| Message ID | CAMe9rOqC+Bar9XVrTutMJuo15qtiwts8AL-UMpaPQPc+WxXqNw@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers |
Received: (qmail 75647 invoked by alias); 13 Oct 2015 13:43:28 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 75637 invoked by uid 89); 13 Oct 2015 13:43:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-ob0-f173.google.com MIME-Version: 1.0 X-Received: by 10.60.36.202 with SMTP id s10mr20233371oej.0.1444743802280; Tue, 13 Oct 2015 06:43:22 -0700 (PDT) In-Reply-To: <561D0388.8030206@redhat.com> References: <20151012231944.GA27441@intel.com> <561D0388.8030206@redhat.com> Date: Tue, 13 Oct 2015 06:43:22 -0700 Message-ID: <CAMe9rOqC+Bar9XVrTutMJuo15qtiwts8AL-UMpaPQPc+WxXqNw@mail.gmail.com> Subject: Re: [PATCH 2/6] Use INLINE_SYSCALL_ERROR_RETURN From: "H.J. Lu" <hjl.tools@gmail.com> To: Florian Weimer <fweimer@redhat.com> Cc: GNU C Library <libc-alpha@sourceware.org> Content-Type: text/plain; charset=UTF-8 |
Commit Message
H.J. Lu
Oct. 13, 2015, 1:43 p.m. UTC
On Tue, Oct 13, 2015 at 6:13 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 10/13/2015 01:19 AM, H.J. Lu wrote: >> diff --git a/sysdeps/unix/sysv/linux/shmat.c b/sysdeps/unix/sysv/linux/shmat.c >> index 94d18d3..3f6388b 100644 >> --- a/sysdeps/unix/sysv/linux/shmat.c >> +++ b/sysdeps/unix/sysv/linux/shmat.c >> @@ -43,10 +43,8 @@ shmat (shmid, shmaddr, shmflg) >> (long int) &raddr, >> (void *) shmaddr); >> if (INTERNAL_SYSCALL_ERROR_P (resultvar, err)) >> - { >> - __set_errno (INTERNAL_SYSCALL_ERRNO (resultvar, err)); >> - return (void *) -1l; >> - } >> + return (void *) INLINE_SYSCALL_ERROR_RETURN (INTERNAL_SYSCALL_ERRNO (resultvar, >> + err)); > > Please put in a cast to ptrdiff_t before the cast to void *. This makes > it more likely that we get the desired sign extension. Or use MAP_FAILED. I am making this change.
Comments
"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));