[i-g-t,2/7] Cast void * pointer used in arithmetic to uint32_t*
diff mbox

Message ID fcdf5cfd9ed825fd090696a8014131cd249d2ed4.1531005542.git.rodrigosiqueiramelo@gmail.com
State New
Headers show

Commit Message

Rodrigo Siqueira July 7, 2018, 11:22 p.m. UTC
This commit fixes the GCC warning:

warning: pointer of type ‘void *’ used in arithmetic [-Wpointer-arith]
     memset(ptr + offsets[0], full_range ? 0x00 : 0x10,
                ^
warning: pointer of type ‘void *’ used in arithmetic [-Wpointer-arith]
     memset(ptr + offsets[1], 0x80,

This commit cast the ptr pointer, which is a void *, to uint32_t * in
the pointer arithmetic operation.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 lib/igt_fb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Arkadiusz Hiler July 10, 2018, 1:58 p.m. UTC | #1
On Sat, Jul 07, 2018 at 08:22:38PM -0300, Rodrigo Siqueira wrote:
> This commit fixes the GCC warning:
> 
> warning: pointer of type ‘void *’ used in arithmetic [-Wpointer-arith]
>      memset(ptr + offsets[0], full_range ? 0x00 : 0x10,
>                 ^
> warning: pointer of type ‘void *’ used in arithmetic [-Wpointer-arith]
>      memset(ptr + offsets[1], 0x80,
> 
> This commit cast the ptr pointer, which is a void *, to uint32_t * in
> the pointer arithmetic operation.

This will change semantics, as according to GNU C standard[1], void
pointers have a size of 1 for all arithmetical purposes.

So you should be using uint8_t (or char) pointer instead.

[1]: http://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html

> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
>  lib/igt_fb.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index ae71d967..ca905038 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -410,9 +410,11 @@ static int create_bo_for_fb(int fd, int width, int height,
>  
>  			switch (format->drm_id) {
>  			case DRM_FORMAT_NV12:
> -				memset(ptr + offsets[0], full_range ? 0x00 : 0x10,
> +				memset(((uint32_t *)ptr) + offsets[0],
> +				       full_range ? 0x00 : 0x10,
>  				       calculated_stride * height);
> -				memset(ptr + offsets[1], 0x80,
> +				memset(((uint32_t *)ptr) + offsets[1],
> +				       0x80,
>  				       calculated_stride * height/2);
>  				break;
>  			case DRM_FORMAT_YUYV:
> -- 
> 2.18.0
>
Chris Wilson July 10, 2018, 2:09 p.m. UTC | #2
Quoting Arkadiusz Hiler (2018-07-10 14:58:49)
> On Sat, Jul 07, 2018 at 08:22:38PM -0300, Rodrigo Siqueira wrote:
> > This commit fixes the GCC warning:
> > 
> > warning: pointer of type ‘void *’ used in arithmetic [-Wpointer-arith]
> >      memset(ptr + offsets[0], full_range ? 0x00 : 0x10,
> >                 ^
> > warning: pointer of type ‘void *’ used in arithmetic [-Wpointer-arith]
> >      memset(ptr + offsets[1], 0x80,
> > 
> > This commit cast the ptr pointer, which is a void *, to uint32_t * in
> > the pointer arithmetic operation.
> 
> This will change semantics, as according to GNU C standard[1], void
> pointers have a size of 1 for all arithmetical purposes.
> 
> So you should be using uint8_t (or char) pointer instead.

Please just fix the compiler flags, we want close compatibility with the
kernel coding standards which explicitly allow void arithmetic for the
simplicity it lends to writing code.
-Chris
Arkadiusz Hiler July 10, 2018, 2:38 p.m. UTC | #3
On Tue, Jul 10, 2018 at 03:09:25PM +0100, Chris Wilson wrote:
> Quoting Arkadiusz Hiler (2018-07-10 14:58:49)
> > On Sat, Jul 07, 2018 at 08:22:38PM -0300, Rodrigo Siqueira wrote:
> > > This commit fixes the GCC warning:
> > > 
> > > warning: pointer of type ‘void *’ used in arithmetic [-Wpointer-arith]
> > >      memset(ptr + offsets[0], full_range ? 0x00 : 0x10,
> > >                 ^
> > > warning: pointer of type ‘void *’ used in arithmetic [-Wpointer-arith]
> > >      memset(ptr + offsets[1], 0x80,
> > > 
> > > This commit cast the ptr pointer, which is a void *, to uint32_t * in
> > > the pointer arithmetic operation.
> > 
> > This will change semantics, as according to GNU C standard[1], void
> > pointers have a size of 1 for all arithmetical purposes.
> > 
> > So you should be using uint8_t (or char) pointer instead.
> 
> Please just fix the compiler flags, we want close compatibility with the
> kernel coding standards which explicitly allow void arithmetic for the
> simplicity it lends to writing code.
> -Chris

Fair point.

We don't rise the error with meson, so it is not a change in the gcc
defaults. Somehow autotools manage to end up adding -Wpointer-arith to
BASE_CFLAGS.

I don't think we should invest time into making autotools behave, since
it's going to be dropped completely. Hopefully this will happen sooner
than later.
Chris Wilson July 10, 2018, 2:43 p.m. UTC | #4
Quoting Arkadiusz Hiler (2018-07-10 15:38:26)
> On Tue, Jul 10, 2018 at 03:09:25PM +0100, Chris Wilson wrote:
> > Quoting Arkadiusz Hiler (2018-07-10 14:58:49)
> > > On Sat, Jul 07, 2018 at 08:22:38PM -0300, Rodrigo Siqueira wrote:
> > > > This commit fixes the GCC warning:
> > > > 
> > > > warning: pointer of type ‘void *’ used in arithmetic [-Wpointer-arith]
> > > >      memset(ptr + offsets[0], full_range ? 0x00 : 0x10,
> > > >                 ^
> > > > warning: pointer of type ‘void *’ used in arithmetic [-Wpointer-arith]
> > > >      memset(ptr + offsets[1], 0x80,
> > > > 
> > > > This commit cast the ptr pointer, which is a void *, to uint32_t * in
> > > > the pointer arithmetic operation.
> > > 
> > > This will change semantics, as according to GNU C standard[1], void
> > > pointers have a size of 1 for all arithmetical purposes.
> > > 
> > > So you should be using uint8_t (or char) pointer instead.
> > 
> > Please just fix the compiler flags, we want close compatibility with the
> > kernel coding standards which explicitly allow void arithmetic for the
> > simplicity it lends to writing code.
> > -Chris
> 
> Fair point.
> 
> We don't rise the error with meson, so it is not a change in the gcc
> defaults. Somehow autotools manage to end up adding -Wpointer-arith to
> BASE_CFLAGS.

iirc, it's pulled in from xorg-macros. Maybe just tack a
-Wnopointer-arith onto the end, or sed away.
-Chris
Rodrigo Siqueira July 11, 2018, 1:58 a.m. UTC | #5
Hi,

Thanks for the feedback,

I'm just a little bit confused about the next step here. Is it worth to
fix this patch and the others? I got confused since you said that IGT
will remove autotools and some of the warnings here does not appear on
meson.

Best Regards
Rodrigo Siqueira

On 07/10, Arkadiusz Hiler wrote:
> On Tue, Jul 10, 2018 at 03:09:25PM +0100, Chris Wilson wrote:
> > Quoting Arkadiusz Hiler (2018-07-10 14:58:49)
> > > On Sat, Jul 07, 2018 at 08:22:38PM -0300, Rodrigo Siqueira wrote:
> > > > This commit fixes the GCC warning:
> > > > 
> > > > warning: pointer of type ‘void *’ used in arithmetic [-Wpointer-arith]
> > > >      memset(ptr + offsets[0], full_range ? 0x00 : 0x10,
> > > >                 ^
> > > > warning: pointer of type ‘void *’ used in arithmetic [-Wpointer-arith]
> > > >      memset(ptr + offsets[1], 0x80,
> > > > 
> > > > This commit cast the ptr pointer, which is a void *, to uint32_t * in
> > > > the pointer arithmetic operation.
> > > 
> > > This will change semantics, as according to GNU C standard[1], void
> > > pointers have a size of 1 for all arithmetical purposes.
> > > 
> > > So you should be using uint8_t (or char) pointer instead.
> > 
> > Please just fix the compiler flags, we want close compatibility with the
> > kernel coding standards which explicitly allow void arithmetic for the
> > simplicity it lends to writing code.
> > -Chris
> 
> Fair point.
> 
> We don't rise the error with meson, so it is not a change in the gcc
> defaults. Somehow autotools manage to end up adding -Wpointer-arith to
> BASE_CFLAGS.
> 
> I don't think we should invest time into making autotools behave, since
> it's going to be dropped completely. Hopefully this will happen sooner
> than later.
> 
> -- 
> Cheers,
> Arek

Patch
diff mbox

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index ae71d967..ca905038 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -410,9 +410,11 @@  static int create_bo_for_fb(int fd, int width, int height,
 
 			switch (format->drm_id) {
 			case DRM_FORMAT_NV12:
-				memset(ptr + offsets[0], full_range ? 0x00 : 0x10,
+				memset(((uint32_t *)ptr) + offsets[0],
+				       full_range ? 0x00 : 0x10,
 				       calculated_stride * height);
-				memset(ptr + offsets[1], 0x80,
+				memset(((uint32_t *)ptr) + offsets[1],
+				       0x80,
 				       calculated_stride * height/2);
 				break;
 			case DRM_FORMAT_YUYV: