diff mbox

powerpc: Fix compiler warning on some syscalls

Message ID 54AC160F.6030403@linux.vnet.ibm.com
State New
Headers show

Commit Message

Adhemerval Zanella Jan. 6, 2015, 5:06 p.m. UTC
On 06-01-2015 13:55, Paul Eggert wrote:
> 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.
>
Fair enough, is it ok now for push?

--

	* sysdeps/unix/sysv/linux/futimens.c (futimens): Use address of first
	timespec struct member in syscall macro.
	* sysdeps/unix/sysv/linux/utimensat.c (utimensat): Likewise.
	* sysdeps/unix/sysv/linux/futimesat.c (futimesat): Use address of
	first timeval struct member in syscall macro.
	* sysdeps/unix/sysv/linux/utimes.c (__utimeS): Likewise.

--

Comments

Roland McGrath Jan. 7, 2015, 11:31 p.m. UTC | #1
Using &foo[0] is fine, but I think it does need a comment in each place.
It doesn't have to be a full explanation, just something short like:
	/* Avoid implicit array coercion in syscall macros.  */
Otherwise someone will come along at some point and say, "&foo[0] is
equivalent to foo in C, so I'll remove the superfluous operators."
A more full explanation can go with the definition of INLINE_SYSCALL et al.

It wouldn't hurt for at least some of those implementations to add a
__builtin_classify_type check so that any array_type_class arguments to
syscall macros (at least in machine-independent code) barf at compile time
in a way that leads to seeing a comment that explains the situation.


Thanks,
Roland
Adhemerval Zanella Jan. 8, 2015, 1:12 p.m. UTC | #2
On 07-01-2015 21:31, Roland McGrath wrote:
> Using &foo[0] is fine, but I think it does need a comment in each place.
> It doesn't have to be a full explanation, just something short like:
> 	/* Avoid implicit array coercion in syscall macros.  */
> Otherwise someone will come along at some point and say, "&foo[0] is
> equivalent to foo in C, so I'll remove the superfluous operators."
> A more full explanation can go with the definition of INLINE_SYSCALL et al.
>
> It wouldn't hurt for at least some of those implementations to add a
> __builtin_classify_type check so that any array_type_class arguments to
> syscall macros (at least in machine-independent code) barf at compile time
> in a way that leads to seeing a comment that explains the situation.
>
>
> Thanks,
> Roland
>
I have added your comment suggestion and pushed upstream as 
dd6e8af6ba1b8a95a7f1dc7422e5ea4ccc7fbd93.
diff mbox

Patch

diff --git a/sysdeps/unix/sysv/linux/futimens.c b/sysdeps/unix/sysv/linux/futimens.c
index c7d03a8..1dda738f 100644
--- a/sysdeps/unix/sysv/linux/futimens.c
+++ b/sysdeps/unix/sysv/linux/futimens.c
@@ -37,7 +37,7 @@  futimens (int fd, const struct timespec tsp[2])
       __set_errno (EBADF);
       return -1;
     }
-  return INLINE_SYSCALL (utimensat, 4, fd, NULL, tsp, 0);
+  return INLINE_SYSCALL (utimensat, 4, fd, NULL, &tsp[0], 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..e2cba1a 100644
--- a/sysdeps/unix/sysv/linux/futimesat.c
+++ b/sysdeps/unix/sysv/linux/futimesat.c
@@ -28,13 +28,10 @@ 
 /* 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);
+  return INLINE_SYSCALL (futimesat, 3, fd, file, &tvp[0]);
 }
diff --git a/sysdeps/unix/sysv/linux/utimensat.c b/sysdeps/unix/sysv/linux/utimensat.c
index 5a5dec7..24fd13a 100644
--- a/sysdeps/unix/sysv/linux/utimensat.c
+++ b/sysdeps/unix/sysv/linux/utimensat.c
@@ -35,7 +35,7 @@  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);
+  return INLINE_SYSCALL (utimensat, 4, fd, file, &tsp[0], 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..8864e48 100644
--- a/sysdeps/unix/sysv/linux/utimes.c
+++ b/sysdeps/unix/sysv/linux/utimes.c
@@ -29,7 +29,7 @@ 
 int
 __utimes (const char *file, const struct timeval tvp[2])
 {
-  return INLINE_SYSCALL (utimes, 2, file, tvp);
+  return INLINE_SYSCALL (utimes, 2, file, &tvp[0]);
 }
 
 weak_alias (__utimes, utimes)