Message ID | CAMe9rOqC+Bar9XVrTutMJuo15qtiwts8AL-UMpaPQPc+WxXqNw@mail.gmail.com |
---|---|
State | New |
Headers | show |
"H.J. Lu" <hjl.tools@gmail.com> writes: > diff --git a/sysdeps/unix/sysv/linux/mmap64.c b/sysdeps/unix/sysv/linux/mmap64.c > index 1c9d3c1..8d5b0a4 100644 > --- a/sysdeps/unix/sysv/linux/mmap64.c > +++ b/sysdeps/unix/sysv/linux/mmap64.c > @@ -46,7 +46,7 @@ __mmap64 (void *addr, size_t len, int prot, int > flags, int fd, off64_t offset) > } > #endif > if (offset & ((1 << page_shift) - 1)) > - return (void *) INLINE_SYSCALL_ERROR_RETURN (EINVAL); > + return (void *) (ptrdiff_t) INLINE_SYSCALL_ERROR_RETURN (EINVAL); That's doesn't look like a good change, since you lose the warning if INLINE_SYSCALL_ERROR_RETURN returns something different from the size of a pointer. Andreas.
On 10/13/2015 03:49 PM, Andreas Schwab wrote: > "H.J. Lu" <hjl.tools@gmail.com> writes: > >> diff --git a/sysdeps/unix/sysv/linux/mmap64.c b/sysdeps/unix/sysv/linux/mmap64.c >> index 1c9d3c1..8d5b0a4 100644 >> --- a/sysdeps/unix/sysv/linux/mmap64.c >> +++ b/sysdeps/unix/sysv/linux/mmap64.c >> @@ -46,7 +46,7 @@ __mmap64 (void *addr, size_t len, int prot, int >> flags, int fd, off64_t offset) >> } >> #endif >> if (offset & ((1 << page_shift) - 1)) >> - return (void *) INLINE_SYSCALL_ERROR_RETURN (EINVAL); >> + return (void *) (ptrdiff_t) INLINE_SYSCALL_ERROR_RETURN (EINVAL); > > That's doesn't look like a good change, since you lose the warning if > INLINE_SYSCALL_ERROR_RETURN returns something different from the size of > a pointer. INLINE_SYSCALL_ERROR_RETURN (EINVAL) is of type int, so we definitely do not want a warning here. The problem here is that the cast is implementation-defined, and I think we want sign extension here (although the manual pages are unclear about this). Florian
On Tue, Oct 13, 2015 at 6:49 AM, Andreas Schwab <schwab@suse.de> wrote: > "H.J. Lu" <hjl.tools@gmail.com> writes: > >> diff --git a/sysdeps/unix/sysv/linux/mmap64.c b/sysdeps/unix/sysv/linux/mmap64.c >> index 1c9d3c1..8d5b0a4 100644 >> --- a/sysdeps/unix/sysv/linux/mmap64.c >> +++ b/sysdeps/unix/sysv/linux/mmap64.c >> @@ -46,7 +46,7 @@ __mmap64 (void *addr, size_t len, int prot, int >> flags, int fd, off64_t offset) >> } >> #endif >> if (offset & ((1 << page_shift) - 1)) >> - return (void *) INLINE_SYSCALL_ERROR_RETURN (EINVAL); >> + return (void *) (ptrdiff_t) INLINE_SYSCALL_ERROR_RETURN (EINVAL); > > That's doesn't look like a good change, since you lose the warning if > INLINE_SYSCALL_ERROR_RETURN returns something different from the size of > a pointer. > INLINE_SYSCALL_ERROR_RETURN always returns -1 and we want to cast -1 to (void *). I tried: [hjl@gnu-tools-1 tmp]$ cat x.c void *p; void foo (void) { p = (void *) -1; } [hjl@gnu-tools-1 tmp]$ gcc -S -O2 x.c [hjl@gnu-tools-1 tmp]$ cat x.s .file "x.c" .section .text.unlikely,"ax",@progbits .LCOLDB0: .text .LHOTB0: .p2align 4,,15 .globl foo .type foo, @function foo: .LFB0: .cfi_startproc movq $-1, p(%rip) ret .cfi_endproc .LFE0: .size foo, .-foo .section .text.unlikely .LCOLDE0: .text .LHOTE0: .comm p,8,8 .ident "GCC: (GNU) 5.2.1 20150929 (Red Hat 5.2.1-3)" .section .note.GNU-stack,"",@progbits [hjl@gnu-tools-1 tmp]$ It looks (ptrdiff_t) isn't needed.
On 10/13/2015 03:56 PM, H.J. Lu wrote:
> It looks (ptrdiff_t) isn't needed.
It's not needed on x86_64, but we don't know if there will be
architectures which will eventually need it (or already do). The
existing code had -1L, so I assumed it was better to play it safe and
mirror that.
Florian
On Tue, Oct 13, 2015 at 6:58 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 10/13/2015 03:56 PM, H.J. Lu wrote: > >> It looks (ptrdiff_t) isn't needed. > > It's not needed on x86_64, but we don't know if there will be > architectures which will eventually need it (or already do). The > existing code had -1L, so I assumed it was better to play it safe and > mirror that. Yes, the cast is also needed on x86-64 if __syscall_error is used: [hjl@gnu-tools-1 tmp]$ cat x.c #include <stddef.h> extern int __syscall_error (void); void *p; void foo (void) { p = (void *) (ptrdiff_t) __syscall_error (); } [hjl@gnu-tools-1 tmp]$
On 10/13/2015 04:00 PM, H.J. Lu wrote: > On Tue, Oct 13, 2015 at 6:58 AM, Florian Weimer <fweimer@redhat.com> wrote: >> On 10/13/2015 03:56 PM, H.J. Lu wrote: >> >>> It looks (ptrdiff_t) isn't needed. >> >> It's not needed on x86_64, but we don't know if there will be >> architectures which will eventually need it (or already do). The >> existing code had -1L, so I assumed it was better to play it safe and >> mirror that. > > Yes, the cast is also needed on x86-64 if __syscall_error is used: > > [hjl@gnu-tools-1 tmp]$ cat x.c > #include <stddef.h> > > extern int __syscall_error (void); > void *p; > > void > foo (void) > { > p = (void *) (ptrdiff_t) __syscall_error (); > } > [hjl@gnu-tools-1 tmp]$ Oh, right. I think it's best not to use the new macro in those two cases. If the cast is not a no-op, it's no longer a tail call, after all. Florian
Florian Weimer <fweimer@redhat.com> writes: > On 10/13/2015 03:49 PM, Andreas Schwab wrote: >> "H.J. Lu" <hjl.tools@gmail.com> writes: >> >>> diff --git a/sysdeps/unix/sysv/linux/mmap64.c b/sysdeps/unix/sysv/linux/mmap64.c >>> index 1c9d3c1..8d5b0a4 100644 >>> --- a/sysdeps/unix/sysv/linux/mmap64.c >>> +++ b/sysdeps/unix/sysv/linux/mmap64.c >>> @@ -46,7 +46,7 @@ __mmap64 (void *addr, size_t len, int prot, int >>> flags, int fd, off64_t offset) >>> } >>> #endif >>> if (offset & ((1 << page_shift) - 1)) >>> - return (void *) INLINE_SYSCALL_ERROR_RETURN (EINVAL); >>> + return (void *) (ptrdiff_t) INLINE_SYSCALL_ERROR_RETURN (EINVAL); >> >> That's doesn't look like a good change, since you lose the warning if >> INLINE_SYSCALL_ERROR_RETURN returns something different from the size of >> a pointer. > > INLINE_SYSCALL_ERROR_RETURN (EINVAL) is of type int, so we definitely do > not want a warning here. INLINE_SYSCALL_ERROR_RETURN should return the same type as INLINE_SYSCALL. Andreas.
"H.J. Lu" <hjl.tools@gmail.com> writes: > On Tue, Oct 13, 2015 at 6:58 AM, Florian Weimer <fweimer@redhat.com> wrote: >> On 10/13/2015 03:56 PM, H.J. Lu wrote: >> >>> It looks (ptrdiff_t) isn't needed. >> >> It's not needed on x86_64, but we don't know if there will be >> architectures which will eventually need it (or already do). The >> existing code had -1L, so I assumed it was better to play it safe and >> mirror that. > > Yes, the cast is also needed on x86-64 if __syscall_error is used: > > [hjl@gnu-tools-1 tmp]$ cat x.c > #include <stddef.h> > > extern int __syscall_error (void); __syscall_error returns long on x86_64. Andreas.
On Tue, Oct 13, 2015 at 7:14 AM, Andreas Schwab <schwab@suse.de> wrote: > "H.J. Lu" <hjl.tools@gmail.com> writes: > >> On Tue, Oct 13, 2015 at 6:58 AM, Florian Weimer <fweimer@redhat.com> wrote: >>> On 10/13/2015 03:56 PM, H.J. Lu wrote: >>> >>>> It looks (ptrdiff_t) isn't needed. >>> >>> It's not needed on x86_64, but we don't know if there will be >>> architectures which will eventually need it (or already do). The >>> existing code had -1L, so I assumed it was better to play it safe and >>> mirror that. >> >> Yes, the cast is also needed on x86-64 if __syscall_error is used: >> >> [hjl@gnu-tools-1 tmp]$ cat x.c >> #include <stddef.h> >> >> extern int __syscall_error (void); > > __syscall_error returns long on x86_64. > In this case, (ptrdiff_t) isn't need.
diff --git a/sysdeps/unix/sysv/linux/mmap64.c b/sysdeps/unix/sysv/linux/mmap64.c index 1c9d3c1..8d5b0a4 100644 --- a/sysdeps/unix/sysv/linux/mmap64.c +++ b/sysdeps/unix/sysv/linux/mmap64.c @@ -46,7 +46,7 @@ __mmap64 (void *addr, size_t len, int prot, int flags, int fd, off64_t offset) } #endif if (offset & ((1 << page_shift) - 1)) - return (void *) INLINE_SYSCALL_ERROR_RETURN (EINVAL); + return (void *) (ptrdiff_t) INLINE_SYSCALL_ERROR_RETURN (EINVAL); void *result; result = (void *) INLINE_SYSCALL (mmap2, 6, addr, diff --git a/sysdeps/unix/sysv/linux/shmat.c b/sysdeps/unix/sysv/linux/shmat.c index 3f6388b..15f269a 100644 --- a/sysdeps/unix/sysv/linux/shmat.c +++ b/sysdeps/unix/sysv/linux/shmat.c @@ -43,8 +43,7 @@ shmat (shmid, shmaddr, shmflg) (long int) &raddr, (void *) shmaddr); if (INTERNAL_SYSCALL_ERROR_P (resultvar, err)) - return (void *) INLINE_SYSCALL_ERROR_RETURN (INTERNAL_SYSCALL_ERRNO (resultvar, - err)); + return (void *) (ptrdiff_t) INLINE_SYSCALL_ERROR_RETURN (INTERNAL_SYSCALL_ERRNO (resultvar, err));