Message ID | 20150125215150.GA15033@gmail.com |
---|---|
State | Dropped |
Headers |
Received: (qmail 29613 invoked by alias); 25 Jan 2015 21:53:34 -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 28625 invoked by uid 89); 25 Jan 2015 21:52:58 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pd0-f169.google.com X-Received: by 10.68.135.136 with SMTP id ps8mr29601229pbb.130.1422222712167; Sun, 25 Jan 2015 13:51:52 -0800 (PST) Date: Sun, 25 Jan 2015 13:51:50 -0800 From: "H.J. Lu" <hjl.tools@gmail.com> To: Chris Metcalf <cmetcalf@ezchip.com>, Torvald Riegel <triegel@redhat.com>, Carlos O'Donell <carlos@redhat.com>, GNU C Library <libc-alpha@sourceware.org>, David Miller <davem@davemloft.net>, Richard Henderson <rth@redhat.com>, Mike Frysinger <vapier@gentoo.org>, Andreas Schwab <schwab@suse.de>, "Joseph S. Myers" <joseph@codesourcery.com>, Kaz Kojima <kkojima@rr.iij4u.or.jp>, Thomas Schwinge <thomas@codesourcery.com>, Marcus Shawcroft <marcus.shawcroft@linaro.org>, Chung-Lin Tang <chunglin_tang@mentor.com>, Adhemerval Zanella <azanella@linux.vnet.ibm.com>, Andreas Krebbel <krebbel@linux.vnet.ibm.com> Subject: Re: glibc 2.21 - Machine maintainers, please test your machines. Message-ID: <20150125215150.GA15033@gmail.com> References: <54C2BDD7.7000304@redhat.com> <54C3B6D5.3090308@ezchip.com> <1422119595.29655.42.camel@triegel.csb> <54C5094A.8060300@ezchip.com> <54C51D94.6030007@ezchip.com> <CAMe9rOpOuuC_Bf1eHs9iaiUY6V-fVMHUCKZPAwje_NemBy84wA@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <CAMe9rOpOuuC_Bf1eHs9iaiUY6V-fVMHUCKZPAwje_NemBy84wA@mail.gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) |
Commit Message
H.J. Lu
Jan. 25, 2015, 9:51 p.m. UTC
On Sun, Jan 25, 2015 at 10:12:23AM -0800, H.J. Lu wrote: > On Sun, Jan 25, 2015 at 8:45 AM, Chris Metcalf <cmetcalf@ezchip.com> wrote: > > On 1/25/2015 10:18 AM, Chris Metcalf wrote: > >> > >> I'm now running with the following change, to see if tilegx32 will > >> also pass with it; this implements my suggestion of rounding new_sem to > >> an 8-byte boundary explicitly on ILP32 platforms. > > > > > > With my proposed change, tilegx32 (and tilegx64) pass all the tests. > > Repeated > > here with a suitable ChangeLog. Let me know if this is acceptable to commit > > for 2.21. > > > > 2015-01-25 Chris Metcalf <cmetcalf@ezchip.com> > > > > * sysdeps/nptl/internaltypes.h (to_new_sem): Define. Provides new > > behavior for [__HAVE_64B_ATOMICS && !defined (_LP64)]. > > * nptl/sem_getvalue.c (__new_sem_getvalue): Use to_new_sem. > > * nptl/sem_init.c (__new_sem_init): Likewise. > > * nptl/sem_post.c (__new_sem_post): Likewise. > > * nptl/sem_wait.c (__new_sem_wait): Likewise. > > (__new_sem_trywait): Likewise. > > * nptl/sem_timedwait.c (sem_timedwait): Likewise. > > * nptl/sem_open.c (sem_open): Add comment about to_new_sem. > > > > > > Can you try something similar to siginfo_t in: > > sysdeps/unix/sysv/linux/x86/bits/siginfo.h > > We had a similar problem on x32 before. > > This is for x86. Each target can do something like it if needed. H.J. ---
Comments
On 1/25/2015 4:51 PM, H.J. Lu wrote: > This is for x86. Each target can do something like it if needed. This does change the ABI for existing code, however, which may legitimately have sem_t objects that are only aligned to 4-byte boundaries. Your change is a good idea for new platforms. I suppose we could also do something with symbol versioning to use it for existing 32-bit platforms with 64-bit atomics, too. So there seem to be multiple fixes we can consider. > H.J. > --- > diff --git a/sysdeps/x86/bits/semaphore.h b/sysdeps/x86/bits/semaphore.h > index 18b2b3c..4890e0d 100644 > --- a/sysdeps/x86/bits/semaphore.h > +++ b/sysdeps/x86/bits/semaphore.h > @@ -28,6 +28,11 @@ > # define __SIZEOF_SEM_T 16 > #endif > > +#ifdef __x86_64__ > +# define __SEM_ALIGN_T long long int > +#else > +# define __SEM_ALIGN_T long int > +#endif > > /* Value returned if `sem_open' failed. */ > #define SEM_FAILED ((sem_t *) 0) > @@ -36,5 +41,5 @@ > typedef union > { > char __size[__SIZEOF_SEM_T]; > - long int __align; > + __SEM_ALIGN_T __align; > } sem_t; > >
On Sun, Jan 25, 2015 at 2:10 PM, Chris Metcalf <cmetcalf@ezchip.com> wrote: > On 1/25/2015 4:51 PM, H.J. Lu wrote: > >> This is for x86. Each target can do something like it if needed. > > > This does change the ABI for existing code, however, which may legitimately > have sem_t objects that are only aligned to 4-byte boundaries. Your change > is a good idea for new platforms. I suppose we could also do something > with symbol versioning to use it for existing 32-bit platforms with > 64-bit atomics, too. So there seem to be multiple fixes we can consider. > It doesn't change the size, only increases alignment from 4 bytes to 8 bytes. It may not work on all targets. It works for x32.
On 1/25/2015 6:05 PM, H.J. Lu wrote: > On Sun, Jan 25, 2015 at 2:10 PM, Chris Metcalf <cmetcalf@ezchip.com> wrote: >> On 1/25/2015 4:51 PM, H.J. Lu wrote: >> >>> This is for x86. Each target can do something like it if needed. >> >> This does change the ABI for existing code, however, which may legitimately >> have sem_t objects that are only aligned to 4-byte boundaries. Your change >> is a good idea for new platforms. I suppose we could also do something >> with symbol versioning to use it for existing 32-bit platforms with >> 64-bit atomics, too. So there seem to be multiple fixes we can consider. >> > It doesn't change the size, only increases alignment from 4 bytes to 8 bytes. > It may not work on all targets. It works for x32. The size is not the issue I'm referring to; it's the alignment. Imagine you have some code built against glibc 2.20 or older, where the user has written: sem_t my_sem; Given the 4-byte alignment constraints, this could result in my_sem being aligned to 4 bytes but not to 8 bytes. Now if we were to make the change you propose for glibc 2.21, the user passing this semaphore to sem_post() could be passing a pointer not aligned to 8 bytes, despite the new headers saying that that was required. Bang, bus error -- or on x32, presumably, worse performance even if it performs correctly.
On Sun, Jan 25, 2015 at 4:46 PM, Chris Metcalf <cmetcalf@ezchip.com> wrote: > On 1/25/2015 6:05 PM, H.J. Lu wrote: >> >> On Sun, Jan 25, 2015 at 2:10 PM, Chris Metcalf <cmetcalf@ezchip.com> >> wrote: >>> >>> On 1/25/2015 4:51 PM, H.J. Lu wrote: >>> >>>> This is for x86. Each target can do something like it if needed. >>> >>> >>> This does change the ABI for existing code, however, which may >>> legitimately >>> have sem_t objects that are only aligned to 4-byte boundaries. Your >>> change >>> is a good idea for new platforms. I suppose we could also do something >>> with symbol versioning to use it for existing 32-bit platforms with >>> 64-bit atomics, too. So there seem to be multiple fixes we can consider. >>> >> It doesn't change the size, only increases alignment from 4 bytes to 8 >> bytes. >> It may not work on all targets. It works for x32. > > > The size is not the issue I'm referring to; it's the alignment. > > Imagine you have some code built against glibc 2.20 or older, where the > user has written: > > sem_t my_sem; > > Given the 4-byte alignment constraints, this could result in my_sem > being aligned to 4 bytes but not to 8 bytes. Now if we were to make the > change you propose for glibc 2.21, the user passing this semaphore to > sem_post() could be passing a pointer not aligned to 8 bytes, despite > the new headers saying that that was required. Bang, bus error -- or on > x32, > presumably, worse performance even if it performs correctly. > Realign new_sem requires a few instructions. For x32, the performance gain may be a wash. For x32, I prefer to align sem_t to 8 bytes.
"H.J. Lu" <hjl.tools@gmail.com> writes:
> It doesn't change the size, only increases alignment from 4 bytes to 8 bytes.
Alignment is part of the ABI.
Andreas.
On Mon, Jan 26, 2015 at 1:24 AM, Andreas Schwab <schwab@suse.de> wrote: > "H.J. Lu" <hjl.tools@gmail.com> writes: > >> It doesn't change the size, only increases alignment from 4 bytes to 8 bytes. > > Alignment is part of the ABI. > For x32, we can increase alignment from 4 bytes to 8 bytes without breaking existing binaries.
On Mon, 2015-01-26 at 04:44 -0800, H.J. Lu wrote: > On Mon, Jan 26, 2015 at 1:24 AM, Andreas Schwab <schwab@suse.de> wrote: > > "H.J. Lu" <hjl.tools@gmail.com> writes: > > > >> It doesn't change the size, only increases alignment from 4 bytes to 8 bytes. > > > > Alignment is part of the ABI. > > > > For x32, we can increase alignment from 4 bytes to 8 bytes without breaking > existing binaries. But you'd still have to do the alignment check at runtime, because you can't expect the application to have used the new headers with proper alignment. Or are you saying that the alignment is just a performance thing in the particular case of x32, because the atomics work either way? If so, then using __alignof__(sem_t) in Chris' patch would be good because then to_new_sem would resolve to a noop in x32 with your alignment change applied.
On Mon, Jan 26, 2015 at 4:50 AM, Torvald Riegel <triegel@redhat.com> wrote: > On Mon, 2015-01-26 at 04:44 -0800, H.J. Lu wrote: >> On Mon, Jan 26, 2015 at 1:24 AM, Andreas Schwab <schwab@suse.de> wrote: >> > "H.J. Lu" <hjl.tools@gmail.com> writes: >> > >> >> It doesn't change the size, only increases alignment from 4 bytes to 8 bytes. >> > >> > Alignment is part of the ABI. >> > >> >> For x32, we can increase alignment from 4 bytes to 8 bytes without breaking >> existing binaries. > > But you'd still have to do the alignment check at runtime, because you > can't expect the application to have used the new headers with proper > alignment. Or are you saying that the alignment is just a performance > thing in the particular case of x32, because the atomics work either Yes. > way? If so, then using __alignof__(sem_t) in Chris' patch would be good > because then to_new_sem would resolve to a noop in x32 with your > alignment change applied. I don't want a noop. I want compiler not to generate a noop at all.
On Mon, 2015-01-26 at 05:25 -0800, H.J. Lu wrote: > On Mon, Jan 26, 2015 at 4:50 AM, Torvald Riegel <triegel@redhat.com> wrote: > > way? If so, then using __alignof__(sem_t) in Chris' patch would be good > > because then to_new_sem would resolve to a noop in x32 with your > > alignment change applied. > > I don't want a noop. I want compiler not to generate a noop at all. Yes, that's what I mean. Compiler should figure out that no conversion is necessary and not emit any code for this conversion.
On Mon, Jan 26, 2015 at 5:32 AM, Torvald Riegel <triegel@redhat.com> wrote: > On Mon, 2015-01-26 at 05:25 -0800, H.J. Lu wrote: >> On Mon, Jan 26, 2015 at 4:50 AM, Torvald Riegel <triegel@redhat.com> wrote: >> > way? If so, then using __alignof__(sem_t) in Chris' patch would be good >> > because then to_new_sem would resolve to a noop in x32 with your >> > alignment change applied. >> >> I don't want a noop. I want compiler not to generate a noop at all. > > Yes, that's what I mean. Compiler should figure out that no conversion > is necessary and not emit any code for this conversion. > If it is true, we should drop "&& !defined (_LP64)" in: +#if __HAVE_64B_ATOMICS && !defined (_LP64) +# define to_new_sem(s) ((struct new_sem *)(((uintptr_t)(s) + 4) & -8)) +#else +# define to_new_sem(s) ((struct new_sem *)(s)) +#endif
On Mon, 2015-01-26 at 05:35 -0800, H.J. Lu wrote: > On Mon, Jan 26, 2015 at 5:32 AM, Torvald Riegel <triegel@redhat.com> wrote: > > On Mon, 2015-01-26 at 05:25 -0800, H.J. Lu wrote: > >> On Mon, Jan 26, 2015 at 4:50 AM, Torvald Riegel <triegel@redhat.com> wrote: > >> > way? If so, then using __alignof__(sem_t) in Chris' patch would be good > >> > because then to_new_sem would resolve to a noop in x32 with your > >> > alignment change applied. > >> > >> I don't want a noop. I want compiler not to generate a noop at all. > > > > Yes, that's what I mean. Compiler should figure out that no conversion > > is necessary and not emit any code for this conversion. > > > > If it is true, we should drop "&& !defined (_LP64)" in: > > +#if __HAVE_64B_ATOMICS && !defined (_LP64) > +# define to_new_sem(s) ((struct new_sem *)(((uintptr_t)(s) + 4) & -8)) > +#else > +# define to_new_sem(s) ((struct new_sem *)(s)) > +#endif > Maybe I wasn't clear. I suggest to keep this, but use __alignof__(sem_t) there in such a way that if sem_t is 8-byte aligned, the compiler will figure our at compile time that to_new_sem(s) is essentially just a cast. This way, we can keep this code for all archs including tilegx, yet do not have any overhead for the conversion on archs that have sufficient alignment of sem_t (eg, x32 with your patch). Sounds good?
On Mon, Jan 26, 2015 at 5:39 AM, Torvald Riegel <triegel@redhat.com> wrote: > On Mon, 2015-01-26 at 05:35 -0800, H.J. Lu wrote: >> On Mon, Jan 26, 2015 at 5:32 AM, Torvald Riegel <triegel@redhat.com> wrote: >> > On Mon, 2015-01-26 at 05:25 -0800, H.J. Lu wrote: >> >> On Mon, Jan 26, 2015 at 4:50 AM, Torvald Riegel <triegel@redhat.com> wrote: >> >> > way? If so, then using __alignof__(sem_t) in Chris' patch would be good >> >> > because then to_new_sem would resolve to a noop in x32 with your >> >> > alignment change applied. >> >> >> >> I don't want a noop. I want compiler not to generate a noop at all. >> > >> > Yes, that's what I mean. Compiler should figure out that no conversion >> > is necessary and not emit any code for this conversion. >> > >> >> If it is true, we should drop "&& !defined (_LP64)" in: >> >> +#if __HAVE_64B_ATOMICS && !defined (_LP64) >> +# define to_new_sem(s) ((struct new_sem *)(((uintptr_t)(s) + 4) & -8)) >> +#else >> +# define to_new_sem(s) ((struct new_sem *)(s)) >> +#endif >> > > Maybe I wasn't clear. I suggest to keep this, but use > __alignof__(sem_t) there in such a way that if sem_t is 8-byte aligned, > the compiler will figure our at compile time that to_new_sem(s) is > essentially just a cast. > > This way, we can keep this code for all archs including tilegx, yet do > not have any overhead for the conversion on archs that have sufficient > alignment of sem_t (eg, x32 with your patch). > > Sounds good? > That sounds good to me. Please include my sysdeps/x86/bits/semaphore.h change when you make this change. Thanks.
"H.J. Lu" <hjl.tools@gmail.com> writes: > On Mon, Jan 26, 2015 at 1:24 AM, Andreas Schwab <schwab@suse.de> wrote: >> "H.J. Lu" <hjl.tools@gmail.com> writes: >> >>> It doesn't change the size, only increases alignment from 4 bytes to 8 bytes. >> >> Alignment is part of the ABI. >> > > For x32, we can increase alignment from 4 bytes to 8 bytes without breaking > existing binaries. The compiler may generate code to take advantage of the bigger alignment, which will fail if not fulfilled (this is not just about unaligned accesses). Andreas.
On Mon, Jan 26, 2015 at 5:48 AM, Andreas Schwab <schwab@suse.de> wrote: > "H.J. Lu" <hjl.tools@gmail.com> writes: > >> On Mon, Jan 26, 2015 at 1:24 AM, Andreas Schwab <schwab@suse.de> wrote: >>> "H.J. Lu" <hjl.tools@gmail.com> writes: >>> >>>> It doesn't change the size, only increases alignment from 4 bytes to 8 bytes. >>> >>> Alignment is part of the ABI. >>> >> >> For x32, we can increase alignment from 4 bytes to 8 bytes without breaking >> existing binaries. > > The compiler may generate code to take advantage of the bigger > alignment, which will fail if not fulfilled (this is not just about > unaligned accesses). > Failure shouldn't happen on x32 in this case.
On Mon, 2015-01-26 at 05:50 -0800, H.J. Lu wrote: > On Mon, Jan 26, 2015 at 5:48 AM, Andreas Schwab <schwab@suse.de> wrote: > > "H.J. Lu" <hjl.tools@gmail.com> writes: > > > >> On Mon, Jan 26, 2015 at 1:24 AM, Andreas Schwab <schwab@suse.de> wrote: > >>> "H.J. Lu" <hjl.tools@gmail.com> writes: > >>> > >>>> It doesn't change the size, only increases alignment from 4 bytes to 8 bytes. > >>> > >>> Alignment is part of the ABI. > >>> > >> > >> For x32, we can increase alignment from 4 bytes to 8 bytes without breaking > >> existing binaries. > > > > The compiler may generate code to take advantage of the bigger > > alignment, which will fail if not fulfilled (this is not just about > > unaligned accesses). > > > > Failure shouldn't happen on x32 in this case. > Then please provide a *detailed* comment why this is the case along with the alignment change in x86 semaphore.h. Given that we're discussing whether this is safe or not, I think we should have detailed documentation. And this will also help conclude the discussion.
On Mon, Jan 26, 2015 at 5:51 AM, Torvald Riegel <triegel@redhat.com> wrote: > On Mon, 2015-01-26 at 05:50 -0800, H.J. Lu wrote: >> On Mon, Jan 26, 2015 at 5:48 AM, Andreas Schwab <schwab@suse.de> wrote: >> > "H.J. Lu" <hjl.tools@gmail.com> writes: >> > >> >> On Mon, Jan 26, 2015 at 1:24 AM, Andreas Schwab <schwab@suse.de> wrote: >> >>> "H.J. Lu" <hjl.tools@gmail.com> writes: >> >>> >> >>>> It doesn't change the size, only increases alignment from 4 bytes to 8 bytes. >> >>> >> >>> Alignment is part of the ABI. >> >>> >> >> >> >> For x32, we can increase alignment from 4 bytes to 8 bytes without breaking >> >> existing binaries. >> > >> > The compiler may generate code to take advantage of the bigger >> > alignment, which will fail if not fulfilled (this is not just about >> > unaligned accesses). >> > >> >> Failure shouldn't happen on x32 in this case. >> > > Then please provide a *detailed* comment why this is the case along with > the alignment change in x86 semaphore.h. Given that we're discussing > whether this is safe or not, I think we should have detailed > documentation. And this will also help conclude the discussion. > This the part of x86 specification. From vol 1 of x86 SDM: 4.1.1 Alignment of Words, Doublewords, Quadwords, and Double Quadwords Words, doublewords, and quadwords do not need to be aligned in memory on natural boundaries. The natural boundaries for words, double words, and quadwords are even-numbered addresses, addresses evenly divisible by four, and addresses evenly divisible by eight, respectively. However, to improve the performance of programs, data structures (especially stacks) should be aligned on natural boundaries whenever possible. The reason for this is that the processor requires two memory accesses to make an unaligned memory access; aligned accesses require only one memory access. A word or doubleword operand that crosses a 4-byte boundary or a quadword operand that crosses an 8-byte boundary is considered unaligned and requires two separate memory bus cycles for access. Do I really to quote this?
On Mon, Jan 26, 2015 at 5:58 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Mon, Jan 26, 2015 at 5:51 AM, Torvald Riegel <triegel@redhat.com> wrote: >> On Mon, 2015-01-26 at 05:50 -0800, H.J. Lu wrote: >>> On Mon, Jan 26, 2015 at 5:48 AM, Andreas Schwab <schwab@suse.de> wrote: >>> > "H.J. Lu" <hjl.tools@gmail.com> writes: >>> > >>> >> On Mon, Jan 26, 2015 at 1:24 AM, Andreas Schwab <schwab@suse.de> wrote: >>> >>> "H.J. Lu" <hjl.tools@gmail.com> writes: >>> >>> >>> >>>> It doesn't change the size, only increases alignment from 4 bytes to 8 bytes. >>> >>> >>> >>> Alignment is part of the ABI. >>> >>> >>> >> >>> >> For x32, we can increase alignment from 4 bytes to 8 bytes without breaking >>> >> existing binaries. >>> > >>> > The compiler may generate code to take advantage of the bigger >>> > alignment, which will fail if not fulfilled (this is not just about >>> > unaligned accesses). >>> > >>> >>> Failure shouldn't happen on x32 in this case. >>> >> >> Then please provide a *detailed* comment why this is the case along with >> the alignment change in x86 semaphore.h. Given that we're discussing >> whether this is safe or not, I think we should have detailed >> documentation. And this will also help conclude the discussion. >> > > This the part of x86 specification. From vol 1 of x86 SDM: > > 4.1.1 Alignment of Words, Doublewords, Quadwords, and Double Quadwords > > Words, doublewords, and quadwords do not need to be aligned in memory > on natural boundaries. The natural > boundaries for words, double words, and quadwords are even-numbered > addresses, addresses evenly divisible by > four, and addresses evenly divisible by eight, respectively. However, > to improve the performance of programs, data > structures (especially stacks) should be aligned on natural boundaries > whenever possible. The reason for this is > that the processor requires two memory accesses to make an unaligned > memory access; aligned accesses require > only one memory access. A word or doubleword operand that crosses a > 4-byte boundary or a quadword operand > that crosses an 8-byte boundary is considered unaligned and requires > two separate memory bus cycles for access. > > Do I really to quote this? > You can download x86 SDM from: http://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-vol-1-manual.pdf
On Mon, 2015-01-26 at 05:58 -0800, H.J. Lu wrote: > On Mon, Jan 26, 2015 at 5:51 AM, Torvald Riegel <triegel@redhat.com> wrote: > > On Mon, 2015-01-26 at 05:50 -0800, H.J. Lu wrote: > >> On Mon, Jan 26, 2015 at 5:48 AM, Andreas Schwab <schwab@suse.de> wrote: > >> > "H.J. Lu" <hjl.tools@gmail.com> writes: > >> > > >> >> On Mon, Jan 26, 2015 at 1:24 AM, Andreas Schwab <schwab@suse.de> wrote: > >> >>> "H.J. Lu" <hjl.tools@gmail.com> writes: > >> >>> > >> >>>> It doesn't change the size, only increases alignment from 4 bytes to 8 bytes. > >> >>> > >> >>> Alignment is part of the ABI. > >> >>> > >> >> > >> >> For x32, we can increase alignment from 4 bytes to 8 bytes without breaking > >> >> existing binaries. > >> > > >> > The compiler may generate code to take advantage of the bigger > >> > alignment, which will fail if not fulfilled (this is not just about > >> > unaligned accesses). > >> > > >> > >> Failure shouldn't happen on x32 in this case. > >> > > > > Then please provide a *detailed* comment why this is the case along with > > the alignment change in x86 semaphore.h. Given that we're discussing > > whether this is safe or not, I think we should have detailed > > documentation. And this will also help conclude the discussion. > > > > This the part of x86 specification. From vol 1 of x86 SDM: > > 4.1.1 Alignment of Words, Doublewords, Quadwords, and Double Quadwords > > Words, doublewords, and quadwords do not need to be aligned in memory > on natural boundaries. The natural > boundaries for words, double words, and quadwords are even-numbered > addresses, addresses evenly divisible by > four, and addresses evenly divisible by eight, respectively. However, > to improve the performance of programs, data > structures (especially stacks) should be aligned on natural boundaries > whenever possible. The reason for this is > that the processor requires two memory accesses to make an unaligned > memory access; aligned accesses require > only one memory access. A word or doubleword operand that crosses a > 4-byte boundary or a quadword operand > that crosses an 8-byte boundary is considered unaligned and requires > two separate memory bus cycles for access. > > Do I really to quote this? > It made an experienced glibc developer wonder whether it would work, so personally, I think a comment in the code is useful. If we need to exchange emails about this, it's likely that others will wonder too, so a comment doesn't hurt. For example, I believe that if such a comment would have been part of the patch, we wouldn't need to have to discuss this, because it would have been clear from the start. IMO, we should want to make glibc simple instead of creating puzzles :) For the comment, I think that you could point to just this section, or summarize in a way that clarifies that unaligned is not an issue for x32, or something like that.
On Mon, Jan 26, 2015 at 6:35 AM, Torvald Riegel <triegel@redhat.com> wrote: > On Mon, 2015-01-26 at 05:58 -0800, H.J. Lu wrote: >> On Mon, Jan 26, 2015 at 5:51 AM, Torvald Riegel <triegel@redhat.com> wrote: >> > On Mon, 2015-01-26 at 05:50 -0800, H.J. Lu wrote: >> >> On Mon, Jan 26, 2015 at 5:48 AM, Andreas Schwab <schwab@suse.de> wrote: >> >> > "H.J. Lu" <hjl.tools@gmail.com> writes: >> >> > >> >> >> On Mon, Jan 26, 2015 at 1:24 AM, Andreas Schwab <schwab@suse.de> wrote: >> >> >>> "H.J. Lu" <hjl.tools@gmail.com> writes: >> >> >>> >> >> >>>> It doesn't change the size, only increases alignment from 4 bytes to 8 bytes. >> >> >>> >> >> >>> Alignment is part of the ABI. >> >> >>> >> >> >> >> >> >> For x32, we can increase alignment from 4 bytes to 8 bytes without breaking >> >> >> existing binaries. >> >> > >> >> > The compiler may generate code to take advantage of the bigger >> >> > alignment, which will fail if not fulfilled (this is not just about >> >> > unaligned accesses). >> >> > >> >> >> >> Failure shouldn't happen on x32 in this case. >> >> >> > >> > Then please provide a *detailed* comment why this is the case along with >> > the alignment change in x86 semaphore.h. Given that we're discussing >> > whether this is safe or not, I think we should have detailed >> > documentation. And this will also help conclude the discussion. >> > >> >> This the part of x86 specification. From vol 1 of x86 SDM: >> >> 4.1.1 Alignment of Words, Doublewords, Quadwords, and Double Quadwords >> >> Words, doublewords, and quadwords do not need to be aligned in memory >> on natural boundaries. The natural >> boundaries for words, double words, and quadwords are even-numbered >> addresses, addresses evenly divisible by >> four, and addresses evenly divisible by eight, respectively. However, >> to improve the performance of programs, data >> structures (especially stacks) should be aligned on natural boundaries >> whenever possible. The reason for this is >> that the processor requires two memory accesses to make an unaligned >> memory access; aligned accesses require >> only one memory access. A word or doubleword operand that crosses a >> 4-byte boundary or a quadword operand >> that crosses an 8-byte boundary is considered unaligned and requires >> two separate memory bus cycles for access. >> >> Do I really to quote this? >> > > It made an experienced glibc developer wonder whether it would work, so > personally, I think a comment in the code is useful. If we need to > exchange emails about this, it's likely that others will wonder too, so > a comment doesn't hurt. > For example, I believe that if such a comment would have been part of > the patch, we wouldn't need to have to discuss this, because it would > have been clear from the start. > > IMO, we should want to make glibc simple instead of creating puzzles :) > > For the comment, I think that you could point to just this section, or > summarize in a way that clarifies that unaligned is not an issue for > x32, or something like that. > I will prepare a patch to include some comments.
"H.J. Lu" <hjl.tools@gmail.com> writes: > On Mon, Jan 26, 2015 at 5:48 AM, Andreas Schwab <schwab@suse.de> wrote: >> "H.J. Lu" <hjl.tools@gmail.com> writes: >> >>> On Mon, Jan 26, 2015 at 1:24 AM, Andreas Schwab <schwab@suse.de> wrote: >>>> "H.J. Lu" <hjl.tools@gmail.com> writes: >>>> >>>>> It doesn't change the size, only increases alignment from 4 bytes to 8 bytes. >>>> >>>> Alignment is part of the ABI. >>>> >>> >>> For x32, we can increase alignment from 4 bytes to 8 bytes without breaking >>> existing binaries. >> >> The compiler may generate code to take advantage of the bigger >> alignment, which will fail if not fulfilled (this is not just about >> unaligned accesses). >> > > Failure shouldn't happen on x32 in this case. Which part of "this is not just about unaligned accesses" didn't you understand? Andreas.
On Mon, Jan 26, 2015 at 6:51 AM, Andreas Schwab <schwab@suse.de> wrote: > "H.J. Lu" <hjl.tools@gmail.com> writes: > >> On Mon, Jan 26, 2015 at 5:48 AM, Andreas Schwab <schwab@suse.de> wrote: >>> "H.J. Lu" <hjl.tools@gmail.com> writes: >>> >>>> On Mon, Jan 26, 2015 at 1:24 AM, Andreas Schwab <schwab@suse.de> wrote: >>>>> "H.J. Lu" <hjl.tools@gmail.com> writes: >>>>> >>>>>> It doesn't change the size, only increases alignment from 4 bytes to 8 bytes. >>>>> >>>>> Alignment is part of the ABI. >>>>> >>>> >>>> For x32, we can increase alignment from 4 bytes to 8 bytes without breaking >>>> existing binaries. >>> >>> The compiler may generate code to take advantage of the bigger >>> alignment, which will fail if not fulfilled (this is not just about >>> unaligned accesses). >>> >> >> Failure shouldn't happen on x32 in this case. > > Which part of "this is not just about unaligned accesses" didn't you > understand? > Please elaborate. An example/testcase will be more helpful. Thanks.
The compiler can simplify operations based on the known alignment of objects. This is like any other undefined behaviour. Andreas.
On Mon, 2015-01-26 at 16:03 +0100, Andreas Schwab wrote: > The compiler can simplify operations based on the known alignment of > objects. This is like any other undefined behaviour. Is your concern regarding glibc code (ie, sem_*()) or code outside glibc? For glibc, the accesses which might not be aligned as promised are to either the private field, or in the form of atomic operations. Do you seem examples of badness happening in both cases, or is this more a case of not being able to prove absence of badness (which would be a valid concern too). For code outside of glibc, I'm not sure something can actually happen. If an application picks up the new semaphore.h, sem_t will be 8B-aligned. If not, then not. I don't think this can be partially 8B/4B-aligned, or can it?
Torvald Riegel <triegel@redhat.com> writes: > On Mon, 2015-01-26 at 16:03 +0100, Andreas Schwab wrote: >> The compiler can simplify operations based on the known alignment of >> objects. This is like any other undefined behaviour. > > Is your concern regarding glibc code (ie, sem_*()) or code outside > glibc? It doesn't matter. If you have a type with 8 byte alignment, but the object has only 4 byte alignment you get undefined behaviour. > For code outside of glibc, I'm not sure something can actually happen. > If an application picks up the new semaphore.h, sem_t will be > 8B-aligned. If not, then not. I don't think this can be partially > 8B/4B-aligned, or can it? You are changing the ABI. Anything can happen. Andreas.
[Note that I don't have a strong opinion in either direction regarding this. I just want to make sure there's no misunderstanding here and everyone knows why we decide either way (and agrees with the decision).] On Mon, 2015-01-26 at 16:52 +0100, Andreas Schwab wrote: > Torvald Riegel <triegel@redhat.com> writes: > > > On Mon, 2015-01-26 at 16:03 +0100, Andreas Schwab wrote: > >> The compiler can simplify operations based on the known alignment of > >> objects. This is like any other undefined behaviour. > > > > Is your concern regarding glibc code (ie, sem_*()) or code outside > > glibc? > > It doesn't matter. If you have a type with 8 byte alignment, but the > object has only 4 byte alignment you get undefined behaviour. Well, but we control the glibc code and to some extent it's compilation (if we assume no LTO, at least). It seems HJ is claiming that while his change could be undefined behavior, it is harmless for x32 with the compiler we currently support. > > For code outside of glibc, I'm not sure something can actually happen. > > If an application picks up the new semaphore.h, sem_t will be > > 8B-aligned. If not, then not. I don't think this can be partially > > 8B/4B-aligned, or can it? > > You are changing the ABI. Anything can happen. I agree we change the ABI. Just to clarify though, your concern is thus specifically due to ABI changes of sem_t and what they would do in user programs (e.g., if a sem_t is part of another struct whose alignment changes in return) -- and not regarding how glibc code could fail if presented with a non-8B-aligned sem_t?
Torvald Riegel <triegel@redhat.com> writes: > I agree we change the ABI. Just to clarify though, your concern is thus > specifically due to ABI changes of sem_t and what they would do in user > programs (e.g., if a sem_t is part of another struct whose alignment > changes in return) -- and not regarding how glibc code could fail if > presented with a non-8B-aligned sem_t? Both can fail. Just because it doesn't fail today doesn't mean it won't bite you in the future. Andreas.
On Mon, Jan 26, 2015 at 8:18 AM, Andreas Schwab <schwab@suse.de> wrote: > Torvald Riegel <triegel@redhat.com> writes: > >> I agree we change the ABI. Just to clarify though, your concern is thus >> specifically due to ABI changes of sem_t and what they would do in user >> programs (e.g., if a sem_t is part of another struct whose alignment >> changes in return) -- and not regarding how glibc code could fail if >> presented with a non-8B-aligned sem_t? > > Both can fail. Just because it doesn't fail today doesn't mean it won't > bite you in the future. > The problem is struct foo { int foo; semt_t sem; int bar; } sem/bar have different offsets and struct foo has different sizes when alignment of sem_t is is changed. I withdrew my change to sem_t.
On Mon, 2015-01-26 at 17:18 +0100, Andreas Schwab wrote: > Torvald Riegel <triegel@redhat.com> writes: > > > I agree we change the ABI. Just to clarify though, your concern is thus > > specifically due to ABI changes of sem_t and what they would do in user > > programs (e.g., if a sem_t is part of another struct whose alignment > > changes in return) -- and not regarding how glibc code could fail if > > presented with a non-8B-aligned sem_t? > > Both can fail. Just because it doesn't fail today doesn't mean it won't > bite you in the future. IMHO this conversation would benefit from some more verbosity in your replies. It seems you and HJ disagree, so the only way I see to solve this is to actually talk about it. Anyway, I'll let you and HJ hash this out. In the end, this is primarily about whether x32's sem_t uses a bigger alignment or not.
On 01/25/2015 06:05 PM, H.J. Lu wrote: > On Sun, Jan 25, 2015 at 2:10 PM, Chris Metcalf <cmetcalf@ezchip.com> wrote: >> On 1/25/2015 4:51 PM, H.J. Lu wrote: >> >>> This is for x86. Each target can do something like it if needed. >> >> >> This does change the ABI for existing code, however, which may legitimately >> have sem_t objects that are only aligned to 4-byte boundaries. Your change >> is a good idea for new platforms. I suppose we could also do something >> with symbol versioning to use it for existing 32-bit platforms with >> 64-bit atomics, too. So there seem to be multiple fixes we can consider. >> > > It doesn't change the size, only increases alignment from 4 bytes to 8 bytes. > It may not work on all targets. It works for x32. How can you change the alignment? That will break applications that embedded a sem_t in other structures by changing the size of those structures as they expand to account for the changed object alignment? Cheers, Carlos.
diff --git a/sysdeps/x86/bits/semaphore.h b/sysdeps/x86/bits/semaphore.h index 18b2b3c..4890e0d 100644 --- a/sysdeps/x86/bits/semaphore.h +++ b/sysdeps/x86/bits/semaphore.h @@ -28,6 +28,11 @@ # define __SIZEOF_SEM_T 16 #endif +#ifdef __x86_64__ +# define __SEM_ALIGN_T long long int +#else +# define __SEM_ALIGN_T long int +#endif /* Value returned if `sem_open' failed. */ #define SEM_FAILED ((sem_t *) 0) @@ -36,5 +41,5 @@ typedef union { char __size[__SIZEOF_SEM_T]; - long int __align; + __SEM_ALIGN_T __align; } sem_t;