Message ID | 555E0986.3010608@arm.com |
---|---|
State | Committed |
Headers |
Received: (qmail 50232 invoked by alias); 21 May 2015 16:36:30 -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 50223 invoked by uid 89); 21 May 2015 16:36:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, SPF_PASS autolearn=ham version=3.3.2 X-HELO: eu-smtp-delivery-143.mimecast.com Message-ID: <555E0986.3010608@arm.com> Date: Thu, 21 May 2015 17:36:22 +0100 From: Szabolcs Nagy <szabolcs.nagy@arm.com> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: GNU C Library <libc-alpha@sourceware.org> CC: Marcus Shawcroft <marcus.shawcroft@arm.com>, Ramana Radhakrishnan <Ramana.Radhakrishnan@arm.com> Subject: [PATCH][AArch64] change greg_t definition in ucontext.h X-MC-Unique: 3OAowPLcTbqBMknFxFiVPQ-1 Content-Type: multipart/mixed; boundary="------------060109080706090505040509" |
Commit Message
Szabolcs Nagy
May 21, 2015, 4:36 p.m. UTC
This change assumes greg_t is not used outside of libc. In glibc it is usually used for defining gregset_t, but on aarch64 elf_greg_t is used for that, greg_t is not used anywhere (so it is meant to be a cosmetic fix). If greg_t is used outside of libc, then this is an ABI change, but typedef int greg_t; is not a useful definition on aarch64. Some targets (powerpc) don't define greg_t at all so removing it should be a valid fix too. It seems the signedness of greg_t varies between targets so i settled for using elf_greg_t (unsigned long) for the typedef to be consistent with the members of gregset_t. Changelog: 2015-05-07 Szabolcs Nagy <szabolcs.nagy@arm.com> * sysdeps/unix/sysv/linux/aarch64/sys/ucontext.h (greg_t): Change the definition to elf_greg_t.
Comments
On 05/21/2015 12:36 PM, Szabolcs Nagy wrote: > This change assumes greg_t is not used outside of libc. > In glibc it is usually used for defining gregset_t, but > on aarch64 elf_greg_t is used for that, greg_t is not > used anywhere (so it is meant to be a cosmetic fix). > > If greg_t is used outside of libc, then this is an ABI > change, but > > typedef int greg_t; > > is not a useful definition on aarch64. > > Some targets (powerpc) don't define greg_t at all so > removing it should be a valid fix too. It seems the > signedness of greg_t varies between targets so i settled > for using elf_greg_t (unsigned long) for the typedef to > be consistent with the members of gregset_t. > > > Changelog: > > 2015-05-07 Szabolcs Nagy <szabolcs.nagy@arm.com> > > * sysdeps/unix/sysv/linux/aarch64/sys/ucontext.h (greg_t): Change the > definition to elf_greg_t. > OK for me. I agree it is not a useful definition for aarch64, however, it is technically an ABI break. As long as you acknowledge that you are breaking the ABI for this type, then you can go ahead, but something in userspace *might* break. The only other component I've seen use greg_t is gdb, but on aarch64 you don't use greg_t, and if you did you would have discovered this was broken earlier. Cheers, Carlos.
On 21/05/15 19:21, Carlos O'Donell wrote: > On 05/21/2015 12:36 PM, Szabolcs Nagy wrote: >> This change assumes greg_t is not used outside of libc. >> In glibc it is usually used for defining gregset_t, but >> on aarch64 elf_greg_t is used for that, greg_t is not >> used anywhere (so it is meant to be a cosmetic fix). >> >> If greg_t is used outside of libc, then this is an ABI >> change, but >> >> typedef int greg_t; >> >> is not a useful definition on aarch64. >> >> Some targets (powerpc) don't define greg_t at all so >> removing it should be a valid fix too. It seems the >> signedness of greg_t varies between targets so i settled >> for using elf_greg_t (unsigned long) for the typedef to >> be consistent with the members of gregset_t. >> >> >> Changelog: >> >> 2015-05-07 Szabolcs Nagy <szabolcs.nagy@arm.com> >> >> * sysdeps/unix/sysv/linux/aarch64/sys/ucontext.h (greg_t): Change the >> definition to elf_greg_t. >> > > OK for me. > > I agree it is not a useful definition for aarch64, > however, it is technically an ABI break. > > As long as you acknowledge that you are breaking > the ABI for this type, then you can go ahead, but > something in userspace *might* break. > i was thinking about it, but i don't see a useful way to annotate this change other than adding a comment. since greg_t is not used in libc, it seems it only breaks code that either uses it in an extern api between modules or the source code somehow depends on it being int. i can include a comment about the abi break after glibc 2.21 next to the typedef. > The only other component I've seen use greg_t is > gdb, but on aarch64 you don't use greg_t, and if > you did you would have discovered this was broken > earlier. the semantics of greg_t is target specific. it seems to be used in a few places (gdb, qemu,..) but always under a target specific ifdef to access greg_t members in mcontext. > Cheers, > Carlos. >
On 05/28/2015 09:37 AM, Szabolcs Nagy wrote: > it seems to be used in a few places (gdb, qemu,..) > but always under a target specific ifdef to access > greg_t members in mcontext. Is it used for AArch64 though? As far as I can tell gdb for AArch64 does not use greg_t. Either way, the present definition is broken, so the user could not have expected it to work in any way, but if the type was embedded into a structure and unused, then you might have an ABI breaking situation. I do not think you are in that case because mcontext on AArch64 doesn't use greg_t. Cheers, Carlos.
On 29 May 2015 at 14:55, Carlos O'Donell <carlos@redhat.com> wrote: > On 05/28/2015 09:37 AM, Szabolcs Nagy wrote: >> it seems to be used in a few places (gdb, qemu,..) >> but always under a target specific ifdef to access >> greg_t members in mcontext. > > Is it used for AArch64 though? As far as I can tell gdb > for AArch64 does not use greg_t. > > Either way, the present definition is broken, so the user > could not have expected it to work in any way, but if the > type was embedded into a structure and unused, then you > might have an ABI breaking situation. I do not think you > are in that case because mcontext on AArch64 doesn't use > greg_t. Hi. The analysis Carlos provided looks sensible to me, go ahead and commit your patch. /Marcus
diff --git a/sysdeps/unix/sysv/linux/aarch64/sys/ucontext.h b/sysdeps/unix/sysv/linux/aarch64/sys/ucontext.h index db7a7c1..e7268ce 100644 --- a/sysdeps/unix/sysv/linux/aarch64/sys/ucontext.h +++ b/sysdeps/unix/sysv/linux/aarch64/sys/ucontext.h @@ -29,7 +29,7 @@ included in <signal.h>. */ #include <bits/sigcontext.h> -typedef int greg_t; +typedef elf_greg_t greg_t; /* Container for all general registers. */ typedef elf_gregset_t gregset_t;