[PATCH][AArch64] change greg_t definition in ucontext.h

[PATCH][AArch64] change greg_t definition in ucontext.h

[Szabolcs Nagy]

[-- Attachment #1: Type: text/plain, Size: 801 bytes --]

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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: greg_t.diff --]
[-- Type: text/x-patch; name=greg_t.diff, Size: 470 bytes --]

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;

Re: [PATCH][AArch64] change greg_t definition in ucontext.h

[Carlos O'Donell]

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.

Re: [PATCH][AArch64] change greg_t definition in ucontext.h

[Szabolcs Nagy]

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.
> 

Re: [PATCH][AArch64] change greg_t definition in ucontext.h

[Carlos O'Donell]

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.

Re: [PATCH][AArch64] change greg_t definition in ucontext.h

[Marcus Shawcroft]

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