| Message ID | 54ABDECE.3030306@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jan 06, 2015 at 11:10:38AM -0200, Adhemerval Zanella wrote: > GCC 5.0 emits an warning when using sizeof on array function parameters > and powerpc internal syscall macros add an check for such cases. More > specifically, on powerpc64 and powerpc32 sysdep.h: > > if (__builtin_classify_type (__arg3) != 5 && sizeof (__arg3) > 8) \ > __illegally_sized_syscall_arg3 (); \ > > And for sysdeps/unix/sysv/linux/utimensat.c build GCC emits: > > error: ‘sizeof’ on array function parameter ‘tsp’ will return size of > ‘const struct timespec *’ > > This patch adds explicit casts to struct pointers to avoid the warnings. > > Checked on powerpc64 and powerpc32. Ok to push? > > PS: it turned out my earlier checks were incorrect and I didn't see the > issue when I updated my GCC because I used --disable-werror. > > -- > > * sysdeps/unix/sysv/linux/futimens.c (futimens): Cast timespec struct > argument to pointer. > * sysdeps/unix/sysv/linux/utimensat.c (utimensat): Likewise. > * sysdeps/unix/sysv/linux/futimesat.c (futimesat): Cast timeval struct > argument to pointer. > * sysdeps/unix/sysv/linux/utimes.c (__utimeS): Likewise. > > -- > > diff --git a/sysdeps/unix/sysv/linux/futimens.c b/sysdeps/unix/sysv/linux/futimens.c > index c7d03a8..c0e7219 100644 > --- a/sysdeps/unix/sysv/linux/futimens.c > +++ b/sysdeps/unix/sysv/linux/futimens.c > @@ -37,7 +37,11 @@ futimens (int fd, const struct timespec tsp[2]) > __set_errno (EBADF); > return -1; > } > - return INLINE_SYSCALL (utimensat, 4, fd, NULL, tsp, 0); > + /* Some archs (powerpc) add arguments type and size check using sizeof > + and without a cast the compiler might emit an warning about using > + sizeof on a struct (where the builtin returns the pointer size). */ > + return INLINE_SYSCALL (utimensat, 4, fd, NULL, (const struct timespec*)tsp, > + 0); > #else > __set_errno (ENOSYS); > return -1; > diff --git a/sysdeps/unix/sysv/linux/futimesat.c b/sysdeps/unix/sysv/linux/futimesat.c > index ac96e2a..f7d5645 100644 > --- a/sysdeps/unix/sysv/linux/futimesat.c > +++ b/sysdeps/unix/sysv/linux/futimesat.c > @@ -28,13 +28,13 @@ > /* Change the access time of FILE relative to FD to TVP[0] and > the modification time of FILE to TVP[1]. */ > int > -futimesat (fd, file, tvp) > - int fd; > - const char *file; > - const struct timeval tvp[2]; > +futimesat (int fd, const char *file, const struct timeval tvp[2]) > { > if (file == NULL) > return __futimes (fd, tvp); > > - return INLINE_SYSCALL (futimesat, 3, fd, file, tvp); > + /* Some archs (powerpc) add arguments type and size check using sizeof > + and without a cast the compiler might emit an warning about using > + sizeof on a struct (where the builtin returns the pointer size). */ > + return INLINE_SYSCALL (futimesat, 3, fd, file, (const struct timeval*)tvp); I think it'd be better to just change the function parameter to const struct timeval *tvp; the warning is meant to detect a case when a sizeof is applied to a function parameter declared as an array. Marek
On 06-01-2015 11:18, Marek Polacek wrote: > On Tue, Jan 06, 2015 at 11:10:38AM -0200, Adhemerval Zanella wrote: >> GCC 5.0 emits an warning when using sizeof on array function parameters >> and powerpc internal syscall macros add an check for such cases. More >> specifically, on powerpc64 and powerpc32 sysdep.h: >> >> if (__builtin_classify_type (__arg3) != 5 && sizeof (__arg3) > 8) \ >> __illegally_sized_syscall_arg3 (); \ >> >> And for sysdeps/unix/sysv/linux/utimensat.c build GCC emits: >> >> error: ‘sizeof’ on array function parameter ‘tsp’ will return size of >> ‘const struct timespec *’ >> >> This patch adds explicit casts to struct pointers to avoid the warnings. >> >> Checked on powerpc64 and powerpc32. Ok to push? >> >> PS: it turned out my earlier checks were incorrect and I didn't see the >> issue when I updated my GCC because I used --disable-werror. >> >> -- >> >> * sysdeps/unix/sysv/linux/futimens.c (futimens): Cast timespec struct >> argument to pointer. >> * sysdeps/unix/sysv/linux/utimensat.c (utimensat): Likewise. >> * sysdeps/unix/sysv/linux/futimesat.c (futimesat): Cast timeval struct >> argument to pointer. >> * sysdeps/unix/sysv/linux/utimes.c (__utimeS): Likewise. >> >> -- >> >> diff --git a/sysdeps/unix/sysv/linux/futimens.c b/sysdeps/unix/sysv/linux/futimens.c >> index c7d03a8..c0e7219 100644 >> --- a/sysdeps/unix/sysv/linux/futimens.c >> +++ b/sysdeps/unix/sysv/linux/futimens.c >> @@ -37,7 +37,11 @@ futimens (int fd, const struct timespec tsp[2]) >> __set_errno (EBADF); >> return -1; >> } >> - return INLINE_SYSCALL (utimensat, 4, fd, NULL, tsp, 0); >> + /* Some archs (powerpc) add arguments type and size check using sizeof >> + and without a cast the compiler might emit an warning about using >> + sizeof on a struct (where the builtin returns the pointer size). */ >> + return INLINE_SYSCALL (utimensat, 4, fd, NULL, (const struct timespec*)tsp, >> + 0); >> #else >> __set_errno (ENOSYS); >> return -1; >> diff --git a/sysdeps/unix/sysv/linux/futimesat.c b/sysdeps/unix/sysv/linux/futimesat.c >> index ac96e2a..f7d5645 100644 >> --- a/sysdeps/unix/sysv/linux/futimesat.c >> +++ b/sysdeps/unix/sysv/linux/futimesat.c >> @@ -28,13 +28,13 @@ >> /* Change the access time of FILE relative to FD to TVP[0] and >> the modification time of FILE to TVP[1]. */ >> int >> -futimesat (fd, file, tvp) >> - int fd; >> - const char *file; >> - const struct timeval tvp[2]; >> +futimesat (int fd, const char *file, const struct timeval tvp[2]) >> { >> if (file == NULL) >> return __futimes (fd, tvp); >> >> - return INLINE_SYSCALL (futimesat, 3, fd, file, tvp); >> + /* Some archs (powerpc) add arguments type and size check using sizeof >> + and without a cast the compiler might emit an warning about using >> + sizeof on a struct (where the builtin returns the pointer size). */ >> + return INLINE_SYSCALL (futimesat, 3, fd, file, (const struct timeval*)tvp); > I think it'd be better to just change the function parameter to const > struct timeval *tvp; the warning is meant to detect a case when a sizeof > is applied to a function parameter declared as an array. > > Marek > I personally don't have a preference, this suggestion was given by Joseph in a previous threads requesting for comments. I only see that pushing your suggestion will require more changes (the headers and manual).
On Tue, Jan 06, 2015 at 11:41:48AM -0200, Adhemerval Zanella wrote: > On 06-01-2015 11:18, Marek Polacek wrote: > > On Tue, Jan 06, 2015 at 11:10:38AM -0200, Adhemerval Zanella wrote: > >> GCC 5.0 emits an warning when using sizeof on array function parameters > >> and powerpc internal syscall macros add an check for such cases. More > >> specifically, on powerpc64 and powerpc32 sysdep.h: > >> > >> if (__builtin_classify_type (__arg3) != 5 && sizeof (__arg3) > 8) \ > >> __illegally_sized_syscall_arg3 (); \ > >> > >> And for sysdeps/unix/sysv/linux/utimensat.c build GCC emits: > >> > >> error: ‘sizeof’ on array function parameter ‘tsp’ will return size of > >> ‘const struct timespec *’ > >> > >> This patch adds explicit casts to struct pointers to avoid the warnings. > >> > >> Checked on powerpc64 and powerpc32. Ok to push? > >> > >> PS: it turned out my earlier checks were incorrect and I didn't see the > >> issue when I updated my GCC because I used --disable-werror. > >> > >> -- > >> > >> * sysdeps/unix/sysv/linux/futimens.c (futimens): Cast timespec struct > >> argument to pointer. > >> * sysdeps/unix/sysv/linux/utimensat.c (utimensat): Likewise. > >> * sysdeps/unix/sysv/linux/futimesat.c (futimesat): Cast timeval struct > >> argument to pointer. > >> * sysdeps/unix/sysv/linux/utimes.c (__utimeS): Likewise. > >> > >> -- > >> > >> diff --git a/sysdeps/unix/sysv/linux/futimens.c b/sysdeps/unix/sysv/linux/futimens.c > >> index c7d03a8..c0e7219 100644 > >> --- a/sysdeps/unix/sysv/linux/futimens.c > >> +++ b/sysdeps/unix/sysv/linux/futimens.c > >> @@ -37,7 +37,11 @@ futimens (int fd, const struct timespec tsp[2]) > >> __set_errno (EBADF); > >> return -1; > >> } > >> - return INLINE_SYSCALL (utimensat, 4, fd, NULL, tsp, 0); > >> + /* Some archs (powerpc) add arguments type and size check using sizeof > >> + and without a cast the compiler might emit an warning about using > >> + sizeof on a struct (where the builtin returns the pointer size). */ > >> + return INLINE_SYSCALL (utimensat, 4, fd, NULL, (const struct timespec*)tsp, > >> + 0); > >> #else > >> __set_errno (ENOSYS); > >> return -1; > >> diff --git a/sysdeps/unix/sysv/linux/futimesat.c b/sysdeps/unix/sysv/linux/futimesat.c > >> index ac96e2a..f7d5645 100644 > >> --- a/sysdeps/unix/sysv/linux/futimesat.c > >> +++ b/sysdeps/unix/sysv/linux/futimesat.c > >> @@ -28,13 +28,13 @@ > >> /* Change the access time of FILE relative to FD to TVP[0] and > >> the modification time of FILE to TVP[1]. */ > >> int > >> -futimesat (fd, file, tvp) > >> - int fd; > >> - const char *file; > >> - const struct timeval tvp[2]; > >> +futimesat (int fd, const char *file, const struct timeval tvp[2]) > >> { > >> if (file == NULL) > >> return __futimes (fd, tvp); > >> > >> - return INLINE_SYSCALL (futimesat, 3, fd, file, tvp); > >> + /* Some archs (powerpc) add arguments type and size check using sizeof > >> + and without a cast the compiler might emit an warning about using > >> + sizeof on a struct (where the builtin returns the pointer size). */ > >> + return INLINE_SYSCALL (futimesat, 3, fd, file, (const struct timeval*)tvp); > > I think it'd be better to just change the function parameter to const > > struct timeval *tvp; the warning is meant to detect a case when a sizeof > > is applied to a function parameter declared as an array. > > > > Marek > > > I personally don't have a preference, this suggestion was given by Joseph in a > previous threads requesting for comments. I only see that pushing your suggestion > will require more changes (the headers and manual). Allright, in that case please watch for formatting, (const struct timeval*)tvp should be (const struct timeval *) tvp Marek
Adhemerval Zanella <azanella@linux.vnet.ibm.com> writes: > diff --git a/sysdeps/unix/sysv/linux/futimesat.c b/sysdeps/unix/sysv/linux/futimesat.c > index ac96e2a..f7d5645 100644 > --- a/sysdeps/unix/sysv/linux/futimesat.c > +++ b/sysdeps/unix/sysv/linux/futimesat.c > @@ -28,13 +28,13 @@ > /* Change the access time of FILE relative to FD to TVP[0] and > the modification time of FILE to TVP[1]. */ > int > -futimesat (fd, file, tvp) > - int fd; > - const char *file; > - const struct timeval tvp[2]; > +futimesat (int fd, const char *file, const struct timeval tvp[2]) > { > if (file == NULL) > return __futimes (fd, tvp); > > - return INLINE_SYSCALL (futimesat, 3, fd, file, tvp); > + /* Some archs (powerpc) add arguments type and size check using sizeof > + and without a cast the compiler might emit an warning about using > + sizeof on a struct (where the builtin returns the pointer size). */ > + return INLINE_SYSCALL (futimesat, 3, fd, file, (const struct timeval*)tvp); Does &tvp[0] work instead? Andreas.
On 06-01-2015 12:29, Andreas Schwab wrote: > Adhemerval Zanella <azanella@linux.vnet.ibm.com> writes: > >> diff --git a/sysdeps/unix/sysv/linux/futimesat.c b/sysdeps/unix/sysv/linux/futimesat.c >> index ac96e2a..f7d5645 100644 >> --- a/sysdeps/unix/sysv/linux/futimesat.c >> +++ b/sysdeps/unix/sysv/linux/futimesat.c >> @@ -28,13 +28,13 @@ >> /* Change the access time of FILE relative to FD to TVP[0] and >> the modification time of FILE to TVP[1]. */ >> int >> -futimesat (fd, file, tvp) >> - int fd; >> - const char *file; >> - const struct timeval tvp[2]; >> +futimesat (int fd, const char *file, const struct timeval tvp[2]) >> { >> if (file == NULL) >> return __futimes (fd, tvp); >> >> - return INLINE_SYSCALL (futimesat, 3, fd, file, tvp); >> + /* Some archs (powerpc) add arguments type and size check using sizeof >> + and without a cast the compiler might emit an warning about using >> + sizeof on a struct (where the builtin returns the pointer size). */ >> + return INLINE_SYSCALL (futimesat, 3, fd, file, (const struct timeval*)tvp); > Does &tvp[0] work instead? > > Andreas. > It does and I think using would not require an explicit comment about it usage. Would it be a preferable solution?
On 01/06/2015 07:10 AM, Adhemerval Zanella wrote: >> Does &tvp[0] work instead? >> > >> >Andreas. >> > > It does and I think using would not require an explicit comment about it > usage. Would it be a preferable solution? > Absolutely. Casts are too powerful and can get one into trouble too easily.
diff --git a/sysdeps/unix/sysv/linux/futimens.c b/sysdeps/unix/sysv/linux/futimens.c index c7d03a8..c0e7219 100644 --- a/sysdeps/unix/sysv/linux/futimens.c +++ b/sysdeps/unix/sysv/linux/futimens.c @@ -37,7 +37,11 @@ futimens (int fd, const struct timespec tsp[2]) __set_errno (EBADF); return -1; } - return INLINE_SYSCALL (utimensat, 4, fd, NULL, tsp, 0); + /* Some archs (powerpc) add arguments type and size check using sizeof + and without a cast the compiler might emit an warning about using + sizeof on a struct (where the builtin returns the pointer size). */ + return INLINE_SYSCALL (utimensat, 4, fd, NULL, (const struct timespec*)tsp, + 0); #else __set_errno (ENOSYS); return -1; diff --git a/sysdeps/unix/sysv/linux/futimesat.c b/sysdeps/unix/sysv/linux/futimesat.c index ac96e2a..f7d5645 100644 --- a/sysdeps/unix/sysv/linux/futimesat.c +++ b/sysdeps/unix/sysv/linux/futimesat.c @@ -28,13 +28,13 @@ /* Change the access time of FILE relative to FD to TVP[0] and the modification time of FILE to TVP[1]. */ int -futimesat (fd, file, tvp) - int fd; - const char *file; - const struct timeval tvp[2]; +futimesat (int fd, const char *file, const struct timeval tvp[2]) { if (file == NULL) return __futimes (fd, tvp); - return INLINE_SYSCALL (futimesat, 3, fd, file, tvp); + /* Some archs (powerpc) add arguments type and size check using sizeof + and without a cast the compiler might emit an warning about using + sizeof on a struct (where the builtin returns the pointer size). */ + return INLINE_SYSCALL (futimesat, 3, fd, file, (const struct timeval*)tvp); } diff --git a/sysdeps/unix/sysv/linux/utimensat.c b/sysdeps/unix/sysv/linux/utimensat.c index 5a5dec7..7448688 100644 --- a/sysdeps/unix/sysv/linux/utimensat.c +++ b/sysdeps/unix/sysv/linux/utimensat.c @@ -35,7 +35,11 @@ utimensat (int fd, const char *file, const struct timespec tsp[2], return -1; } #ifdef __NR_utimensat - return INLINE_SYSCALL (utimensat, 4, fd, file, tsp, flags); + /* Some archs (powerpc) add arguments type and size check using sizeof + and without a cast the compiler might emit an warning about using + sizeof on a struct (where the builtin returns the pointer size). */ + return INLINE_SYSCALL (utimensat, 4, fd, file, (const struct timespec*)tsp, + flags); #else __set_errno (ENOSYS); return -1; diff --git a/sysdeps/unix/sysv/linux/utimes.c b/sysdeps/unix/sysv/linux/utimes.c index 5423ff2..57c04f9 100644 --- a/sysdeps/unix/sysv/linux/utimes.c +++ b/sysdeps/unix/sysv/linux/utimes.c @@ -29,7 +29,10 @@ int __utimes (const char *file, const struct timeval tvp[2]) { - return INLINE_SYSCALL (utimes, 2, file, tvp); + /* Some archs (powerpc) add arguments type and size check using sizeof + and without a cast the compiler might emit an warning about using + sizeof on a struct (where the builtin returns the pointer size). */ + return INLINE_SYSCALL (utimes, 2, file, (const struct timeval*)tvp); } weak_alias (__utimes, utimes)
GCC 5.0 emits an warning when using sizeof on array function parameters and powerpc internal syscall macros add an check for such cases. More specifically, on powerpc64 and powerpc32 sysdep.h: if (__builtin_classify_type (__arg3) != 5 && sizeof (__arg3) > 8) \ __illegally_sized_syscall_arg3 (); \ And for sysdeps/unix/sysv/linux/utimensat.c build GCC emits: error: ‘sizeof’ on array function parameter ‘tsp’ will return size of ‘const struct timespec *’ This patch adds explicit casts to struct pointers to avoid the warnings. Checked on powerpc64 and powerpc32. Ok to push? PS: it turned out my earlier checks were incorrect and I didn't see the issue when I updated my GCC because I used --disable-werror. -- * sysdeps/unix/sysv/linux/futimens.c (futimens): Cast timespec struct argument to pointer. * sysdeps/unix/sysv/linux/utimensat.c (utimensat): Likewise. * sysdeps/unix/sysv/linux/futimesat.c (futimesat): Cast timeval struct argument to pointer. * sysdeps/unix/sysv/linux/utimes.c (__utimeS): Likewise. --