diff mbox

powerpc: Fix compiler warning on some syscalls

Message ID 54ABDECE.3030306@linux.vnet.ibm.com
State Committed
Delegated to: Adhemerval Zanella Netto
Headers

Commit Message

Adhemerval Zanella Netto Jan. 6, 2015, 1:10 p.m. UTC 
  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.

--

Comments

Marek Polacek Jan. 6, 2015, 1:18 p.m. UTC | #1 
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
Adhemerval Zanella Netto Jan. 6, 2015, 1:41 p.m. UTC | #2 
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).
Marek Polacek Jan. 6, 2015, 1:50 p.m. UTC | #3 
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
Andreas Schwab Jan. 6, 2015, 2:29 p.m. UTC | #4 
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.
Adhemerval Zanella Netto Jan. 6, 2015, 3:10 p.m. UTC | #5 
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?
Paul Eggert Jan. 6, 2015, 3:55 p.m. UTC | #6 
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 mbox

Patch

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)