* [musl] [PATCH] arm64/sigcontext: Synchronize the type of the __reserved field with the linux kernel.
@ 2021-08-18 22:52 Olivier Galibert
2021-08-19 0:49 ` Rich Felker
0 siblings, 1 reply; 6+ messages in thread
From: Olivier Galibert @ 2021-08-18 22:52 UTC (permalink / raw)
To: musl; +Cc: Olivier Galibert
clang's compiler-rt sanitizer_linux.cpp expects the __reserved field
to be convertible to u8 *. So let's.
---
arch/aarch64/bits/signal.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/aarch64/bits/signal.h b/arch/aarch64/bits/signal.h
index 5098c734..a46997e3 100644
--- a/arch/aarch64/bits/signal.h
+++ b/arch/aarch64/bits/signal.h
@@ -19,7 +19,7 @@ typedef struct sigcontext {
unsigned long fault_address;
unsigned long regs[31];
unsigned long sp, pc, pstate;
- long double __reserved[256];
+ unsigned char __reserved[4096] __attribute__((__aligned__(16)));
} mcontext_t;
#define FPSIMD_MAGIC 0x46508001
--
2.33.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [PATCH] arm64/sigcontext: Synchronize the type of the __reserved field with the linux kernel.
2021-08-18 22:52 [musl] [PATCH] arm64/sigcontext: Synchronize the type of the __reserved field with the linux kernel Olivier Galibert
@ 2021-08-19 0:49 ` Rich Felker
2021-08-19 5:54 ` Florian Weimer
0 siblings, 1 reply; 6+ messages in thread
From: Rich Felker @ 2021-08-19 0:49 UTC (permalink / raw)
To: Olivier Galibert; +Cc: musl
On Thu, Aug 19, 2021 at 12:52:23AM +0200, Olivier Galibert wrote:
> clang's compiler-rt sanitizer_linux.cpp expects the __reserved field
> to be convertible to u8 *. So let's.
> ---
> arch/aarch64/bits/signal.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/aarch64/bits/signal.h b/arch/aarch64/bits/signal.h
> index 5098c734..a46997e3 100644
> --- a/arch/aarch64/bits/signal.h
> +++ b/arch/aarch64/bits/signal.h
> @@ -19,7 +19,7 @@ typedef struct sigcontext {
> unsigned long fault_address;
> unsigned long regs[31];
> unsigned long sp, pc, pstate;
> - long double __reserved[256];
> + unsigned char __reserved[4096] __attribute__((__aligned__(16)));
> } mcontext_t;
>
> #define FPSIMD_MAGIC 0x46508001
The member name __reserved is not API, much less its particular type.
If the sanitizer code is attempting to access it, it's doing something
wrong and that should be investigated and fixed. The choice to use
long double was very intentional so that the struct definition does
not depend on GNUC attributes to have the correct alignment.
Rich
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [PATCH] arm64/sigcontext: Synchronize the type of the __reserved field with the linux kernel.
2021-08-19 0:49 ` Rich Felker
@ 2021-08-19 5:54 ` Florian Weimer
2021-08-19 13:38 ` Rich Felker
0 siblings, 1 reply; 6+ messages in thread
From: Florian Weimer @ 2021-08-19 5:54 UTC (permalink / raw)
To: Rich Felker; +Cc: Olivier Galibert, musl
* Rich Felker:
> On Thu, Aug 19, 2021 at 12:52:23AM +0200, Olivier Galibert wrote:
>> clang's compiler-rt sanitizer_linux.cpp expects the __reserved field
>> to be convertible to u8 *. So let's.
>> ---
>> arch/aarch64/bits/signal.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/aarch64/bits/signal.h b/arch/aarch64/bits/signal.h
>> index 5098c734..a46997e3 100644
>> --- a/arch/aarch64/bits/signal.h
>> +++ b/arch/aarch64/bits/signal.h
>> @@ -19,7 +19,7 @@ typedef struct sigcontext {
>> unsigned long fault_address;
>> unsigned long regs[31];
>> unsigned long sp, pc, pstate;
>> - long double __reserved[256];
>> + unsigned char __reserved[4096] __attribute__((__aligned__(16)));
>> } mcontext_t;
>>
>> #define FPSIMD_MAGIC 0x46508001
>
> The member name __reserved is not API, much less its particular type.
The name is called __reserved, but it is actually part of the API.
We learned this when we tried to rename it:
<https://sourceware.org/bugzilla/show_bug.cgi?id=22742>
The name and its __ prefix are rather unfortunate, but we are stuck with
it.
Thanks,
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [PATCH] arm64/sigcontext: Synchronize the type of the __reserved field with the linux kernel.
2021-08-19 5:54 ` Florian Weimer
@ 2021-08-19 13:38 ` Rich Felker
2021-08-20 6:46 ` Szabolcs Nagy
0 siblings, 1 reply; 6+ messages in thread
From: Rich Felker @ 2021-08-19 13:38 UTC (permalink / raw)
To: Florian Weimer; +Cc: Olivier Galibert, musl
On Thu, Aug 19, 2021 at 07:54:12AM +0200, Florian Weimer wrote:
> * Rich Felker:
>
> > On Thu, Aug 19, 2021 at 12:52:23AM +0200, Olivier Galibert wrote:
> >> clang's compiler-rt sanitizer_linux.cpp expects the __reserved field
> >> to be convertible to u8 *. So let's.
> >> ---
> >> arch/aarch64/bits/signal.h | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/aarch64/bits/signal.h b/arch/aarch64/bits/signal.h
> >> index 5098c734..a46997e3 100644
> >> --- a/arch/aarch64/bits/signal.h
> >> +++ b/arch/aarch64/bits/signal.h
> >> @@ -19,7 +19,7 @@ typedef struct sigcontext {
> >> unsigned long fault_address;
> >> unsigned long regs[31];
> >> unsigned long sp, pc, pstate;
> >> - long double __reserved[256];
> >> + unsigned char __reserved[4096] __attribute__((__aligned__(16)));
> >> } mcontext_t;
> >>
> >> #define FPSIMD_MAGIC 0x46508001
> >
> > The member name __reserved is not API, much less its particular type.
>
> The name is called __reserved, but it is actually part of the API.
> We learned this when we tried to rename it:
>
> <https://sourceware.org/bugzilla/show_bug.cgi?id=22742>
>
> The name and its __ prefix are rather unfortunate, but we are stuck with
> it.
I question the reasoning there. Just because there are users of it
doesn't mean it's API, *especially* if the users are things like
sanitizer lib that regularly poke at internals that are not interface
contracts. Use of a name like __reserved as API is *really* bad since
it could even be something like a macro for an attribute in the
implementation, rather than something available as a member name.
My interpretation was that it's something like the powerpc reserved
space where there's a separate pointer into it, which you're supposed
to access it by. But that doesn't seem to be the case here, so I'm not
sure what the right way to access it is. Do you have a list of
software that's actually poking at it so we can evaluate the situation
better for figuring out what to do?
Rich
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [PATCH] arm64/sigcontext: Synchronize the type of the __reserved field with the linux kernel.
2021-08-19 13:38 ` Rich Felker
@ 2021-08-20 6:46 ` Szabolcs Nagy
2021-11-07 7:07 ` Fangrui Song
0 siblings, 1 reply; 6+ messages in thread
From: Szabolcs Nagy @ 2021-08-20 6:46 UTC (permalink / raw)
To: Rich Felker; +Cc: Florian Weimer, Olivier Galibert, musl
* Rich Felker <dalias@libc.org> [2021-08-19 09:38:41 -0400]:
> On Thu, Aug 19, 2021 at 07:54:12AM +0200, Florian Weimer wrote:
> > * Rich Felker:
> >
> > > On Thu, Aug 19, 2021 at 12:52:23AM +0200, Olivier Galibert wrote:
> > >> clang's compiler-rt sanitizer_linux.cpp expects the __reserved field
> > >> to be convertible to u8 *. So let's.
> > >> ---
> > >> arch/aarch64/bits/signal.h | 2 +-
> > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/arch/aarch64/bits/signal.h b/arch/aarch64/bits/signal.h
> > >> index 5098c734..a46997e3 100644
> > >> --- a/arch/aarch64/bits/signal.h
> > >> +++ b/arch/aarch64/bits/signal.h
> > >> @@ -19,7 +19,7 @@ typedef struct sigcontext {
> > >> unsigned long fault_address;
> > >> unsigned long regs[31];
> > >> unsigned long sp, pc, pstate;
> > >> - long double __reserved[256];
> > >> + unsigned char __reserved[4096] __attribute__((__aligned__(16)));
> > >> } mcontext_t;
> > >>
> > >> #define FPSIMD_MAGIC 0x46508001
> > >
> > > The member name __reserved is not API, much less its particular type.
> >
> > The name is called __reserved, but it is actually part of the API.
> > We learned this when we tried to rename it:
> >
> > <https://sourceware.org/bugzilla/show_bug.cgi?id=22742>
> >
> > The name and its __ prefix are rather unfortunate, but we are stuck with
> > it.
>
> I question the reasoning there. Just because there are users of it
> doesn't mean it's API, *especially* if the users are things like
> sanitizer lib that regularly poke at internals that are not interface
> contracts. Use of a name like __reserved as API is *really* bad since
> it could even be something like a macro for an attribute in the
> implementation, rather than something available as a member name.
>
> My interpretation was that it's something like the powerpc reserved
> space where there's a separate pointer into it, which you're supposed
> to access it by. But that doesn't seem to be the case here, so I'm not
> sure what the right way to access it is. Do you have a list of
> software that's actually poking at it so we can evaluate the situation
> better for figuring out what to do?
__reserved holds sigcontext extensions so i'd expect
code accessing the extended register state from a
signal handler would use it (includes fp/simd regs).
e.g. chromium tests seem to use it:
https://source.chromium.org/search?q=uc_mcontext.__reserved
but the ucontext access code in breakpad is written
in asm so the c struct is not used in actual code.
qemu seems to define its own types. and i don't
immediately know of other tools that poke at
sigcontext.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [PATCH] arm64/sigcontext: Synchronize the type of the __reserved field with the linux kernel.
2021-08-20 6:46 ` Szabolcs Nagy
@ 2021-11-07 7:07 ` Fangrui Song
0 siblings, 0 replies; 6+ messages in thread
From: Fangrui Song @ 2021-11-07 7:07 UTC (permalink / raw)
To: musl, Rich Felker, Florian Weimer, Olivier Galibert
On Thu, Aug 19, 2021 at 11:46 PM Szabolcs Nagy <nsz@port70.net> wrote:
>
> * Rich Felker <dalias@libc.org> [2021-08-19 09:38:41 -0400]:
> > On Thu, Aug 19, 2021 at 07:54:12AM +0200, Florian Weimer wrote:
> > > * Rich Felker:
> > >
> > > > On Thu, Aug 19, 2021 at 12:52:23AM +0200, Olivier Galibert wrote:
> > > >> clang's compiler-rt sanitizer_linux.cpp expects the __reserved field
> > > >> to be convertible to u8 *. So let's.
> > > >> ---
> > > >> arch/aarch64/bits/signal.h | 2 +-
> > > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/arch/aarch64/bits/signal.h b/arch/aarch64/bits/signal.h
> > > >> index 5098c734..a46997e3 100644
> > > >> --- a/arch/aarch64/bits/signal.h
> > > >> +++ b/arch/aarch64/bits/signal.h
> > > >> @@ -19,7 +19,7 @@ typedef struct sigcontext {
> > > >> unsigned long fault_address;
> > > >> unsigned long regs[31];
> > > >> unsigned long sp, pc, pstate;
> > > >> - long double __reserved[256];
> > > >> + unsigned char __reserved[4096] __attribute__((__aligned__(16)));
> > > >> } mcontext_t;
> > > >>
> > > >> #define FPSIMD_MAGIC 0x46508001
> > > >
> > > > The member name __reserved is not API, much less its particular type.
> > >
> > > The name is called __reserved, but it is actually part of the API.
> > > We learned this when we tried to rename it:
> > >
> > > <https://sourceware.org/bugzilla/show_bug.cgi?id=22742>
> > >
> > > The name and its __ prefix are rather unfortunate, but we are stuck with
> > > it.
> >
> > I question the reasoning there. Just because there are users of it
> > doesn't mean it's API, *especially* if the users are things like
> > sanitizer lib that regularly poke at internals that are not interface
> > contracts. Use of a name like __reserved as API is *really* bad since
> > it could even be something like a macro for an attribute in the
> > implementation, rather than something available as a member name.
> >
> > My interpretation was that it's something like the powerpc reserved
> > space where there's a separate pointer into it, which you're supposed
> > to access it by. But that doesn't seem to be the case here, so I'm not
> > sure what the right way to access it is. Do you have a list of
> > software that's actually poking at it so we can evaluate the situation
> > better for figuring out what to do?
>
> __reserved holds sigcontext extensions so i'd expect
> code accessing the extended register state from a
> signal handler would use it (includes fp/simd regs).
>
> e.g. chromium tests seem to use it:
>
> https://source.chromium.org/search?q=uc_mcontext.__reserved
>
> but the ucontext access code in breakpad is written
> in asm so the c struct is not used in actual code.
>
> qemu seems to define its own types. and i don't
> immediately know of other tools that poke at
> sigcontext.
I added a reinterpret_cast in
https://github.com/llvm/llvm-project/commit/70986ea3d6aeacb5d10bfe3b75757611d4e7a379
(I have sufficient compiler-rt commits so they may not blame me)
compiler-rt from llvm-project 14.0.0 will work with the long double version.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-07 7:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-18 22:52 [musl] [PATCH] arm64/sigcontext: Synchronize the type of the __reserved field with the linux kernel Olivier Galibert
2021-08-19 0:49 ` Rich Felker
2021-08-19 5:54 ` Florian Weimer
2021-08-19 13:38 ` Rich Felker
2021-08-20 6:46 ` Szabolcs Nagy
2021-11-07 7:07 ` Fangrui Song
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).