diff mbox series

x86-64: Move nptl/rseq-access.h to nptl/64 [BZ #32543]

Message ID 20250111052042.1878880-1-hjl.tools@gmail.com (mailing list archive)
State Rejected
Headers
Series x86-64: Move nptl/rseq-access.h to nptl/64 [BZ #32543] |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed

Commit Message

H.J. Lu Jan. 11, 2025, 5:20 a.m. UTC 
  commit 494d65129ed5ae1154b75cc189bbdde5e9ecf1df
Author: Michael Jeanson <mjeanson@efficios.com>
Date:   Thu Aug 1 10:35:34 2024 -0400

    nptl: Introduce <rseq-access.h> for RSEQ_* accessors

added things like

       asm volatile ("movl %%fs:%P1(%q2),%0"                                  \
                     : "=r" (__value)                                         \
                     : "i" (offsetof (struct rseq_area, member)),             \
                       "r" (__rseq_offset));

But this doesn't work for x32 when __rseq_offset is negative since the
address is computed as

FS + 32-bit to 64-bit zero extension of __rseq_offset
+ offsetof (struct rseq_area, member)

Move x86_64/nptl/rseq-access.h to x86_64/nptl/64/rseq-access.h so that
sysdeps/nptl/rseq-access.h is used for x32.  This fixes BZ #32543.

Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
---
 sysdeps/x86_64/nptl/{ => 64}/rseq-access.h | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename sysdeps/x86_64/nptl/{ => 64}/rseq-access.h (100%)

Comments

Sam James Jan. 11, 2025, 7:04 a.m. UTC | #1 
"H.J. Lu" <hjl.tools@gmail.com> writes:

> commit 494d65129ed5ae1154b75cc189bbdde5e9ecf1df
> Author: Michael Jeanson <mjeanson@efficios.com>
> Date:   Thu Aug 1 10:35:34 2024 -0400
>
>     nptl: Introduce <rseq-access.h> for RSEQ_* accessors
>
> added things like
>
>        asm volatile ("movl %%fs:%P1(%q2),%0"                                  \
>                      : "=r" (__value)                                         \
>                      : "i" (offsetof (struct rseq_area, member)),             \
>                        "r" (__rseq_offset));
>
> But this doesn't work for x32 when __rseq_offset is negative since the
> address is computed as
>
> FS + 32-bit to 64-bit zero extension of __rseq_offset
> + offsetof (struct rseq_area, member)
>
> Move x86_64/nptl/rseq-access.h to x86_64/nptl/64/rseq-access.h so that
> sysdeps/nptl/rseq-access.h is used for x32.  This fixes BZ #32543.
>
> Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> ---
>  sysdeps/x86_64/nptl/{ => 64}/rseq-access.h | 0
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  rename sysdeps/x86_64/nptl/{ => 64}/rseq-access.h (100%)
>
> diff --git a/sysdeps/x86_64/nptl/rseq-access.h b/sysdeps/x86_64/nptl/64/rseq-access.h
> similarity index 100%
> rename from sysdeps/x86_64/nptl/rseq-access.h
> rename to sysdeps/x86_64/nptl/64/rseq-access.h

OK, thanks.

Michael et. al may have followup comments if they want to provide a
specialised impl for x32 of course.
Florian Weimer Jan. 11, 2025, 9:41 a.m. UTC | #2 
* H. J. Lu:

> commit 494d65129ed5ae1154b75cc189bbdde5e9ecf1df
> Author: Michael Jeanson <mjeanson@efficios.com>
> Date:   Thu Aug 1 10:35:34 2024 -0400
>
>     nptl: Introduce <rseq-access.h> for RSEQ_* accessors
>
> added things like
>
>        asm volatile ("movl %%fs:%P1(%q2),%0"                                  \
>                      : "=r" (__value)                                         \
>                      : "i" (offsetof (struct rseq_area, member)),             \
>                        "r" (__rseq_offset));
>
> But this doesn't work for x32 when __rseq_offset is negative since the
> address is computed as
>
> FS + 32-bit to 64-bit zero extension of __rseq_offset
> + offsetof (struct rseq_area, member)
>
> Move x86_64/nptl/rseq-access.h to x86_64/nptl/64/rseq-access.h so that
> sysdeps/nptl/rseq-access.h is used for x32.  This fixes BZ #32543.

Would this work on x86-64 and x86-64 x32?

       asm volatile ("movl %%fs:%P1(%q2),%0"                                  \
                     : "=r" (__value)                                         \
                     : "i" (offsetof (struct rseq_area, member)),             \
                       "r" ((long long int) __rseq_offset));

Thanks,
Florian
Andreas K. Huettel Jan. 11, 2025, 1:58 p.m. UTC | #3 
Since i386/nptl/rseq-access.h (which has only one ABI) and x86_64/nptl/rseq-access.h
are the only non-generic versions of this file, 

am I assuming correctly that we should *not* expect any surprises from exotic ABI on
other types of machines (say 31bit s390 or mips n32)?

Cheers, 
Andreas

Am Samstag, 11. Januar 2025, 06:20:42 Mitteleuropäische Normalzeit schrieb H.J. Lu:
> commit 494d65129ed5ae1154b75cc189bbdde5e9ecf1df
> Author: Michael Jeanson <mjeanson@efficios.com>
> Date:   Thu Aug 1 10:35:34 2024 -0400
> 
>     nptl: Introduce <rseq-access.h> for RSEQ_* accessors
> 
> added things like
> 
>        asm volatile ("movl %%fs:%P1(%q2),%0"                                  \
>                      : "=r" (__value)                                         \
>                      : "i" (offsetof (struct rseq_area, member)),             \
>                        "r" (__rseq_offset));
> 
> But this doesn't work for x32 when __rseq_offset is negative since the
> address is computed as
> 
> FS + 32-bit to 64-bit zero extension of __rseq_offset
> + offsetof (struct rseq_area, member)
> 
> Move x86_64/nptl/rseq-access.h to x86_64/nptl/64/rseq-access.h so that
> sysdeps/nptl/rseq-access.h is used for x32.  This fixes BZ #32543.
> 
> Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> ---
>  sysdeps/x86_64/nptl/{ => 64}/rseq-access.h | 0
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  rename sysdeps/x86_64/nptl/{ => 64}/rseq-access.h (100%)
> 
> diff --git a/sysdeps/x86_64/nptl/rseq-access.h b/sysdeps/x86_64/nptl/64/rseq-access.h
> similarity index 100%
> rename from sysdeps/x86_64/nptl/rseq-access.h
> rename to sysdeps/x86_64/nptl/64/rseq-access.h
>
Mathieu Desnoyers Jan. 11, 2025, 3:19 p.m. UTC | #4 
On 11-Jan-2025 10:41:10 AM, Florian Weimer wrote:
> * H. J. Lu:
> 
> > commit 494d65129ed5ae1154b75cc189bbdde5e9ecf1df
> > Author: Michael Jeanson <mjeanson@efficios.com>
> > Date:   Thu Aug 1 10:35:34 2024 -0400
> >
> >     nptl: Introduce <rseq-access.h> for RSEQ_* accessors
> >
> > added things like
> >
> >        asm volatile ("movl %%fs:%P1(%q2),%0"                                  \
> >                      : "=r" (__value)                                         \
> >                      : "i" (offsetof (struct rseq_area, member)),             \
> >                        "r" (__rseq_offset));
> >
> > But this doesn't work for x32 when __rseq_offset is negative since the
> > address is computed as
> >
> > FS + 32-bit to 64-bit zero extension of __rseq_offset
> > + offsetof (struct rseq_area, member)
> >
> > Move x86_64/nptl/rseq-access.h to x86_64/nptl/64/rseq-access.h so that
> > sysdeps/nptl/rseq-access.h is used for x32.  This fixes BZ #32543.
> 
> Would this work on x86-64 and x86-64 x32?
> 
>        asm volatile ("movl %%fs:%P1(%q2),%0"                                  \
>                      : "=r" (__value)                                         \
>                      : "i" (offsetof (struct rseq_area, member)),             \
>                        "r" ((long long int) __rseq_offset));

__rseq_offset is a ptrdiff_t, which is indeed an issue specifically on
x32 because ptrdiff_t has size 4 and gets zero-extended to 64-bit.

The fix proposed by Florian is better than using the generic
sysdeps/nptl/rseq-access.h because it benefits from using the
%%fs segment selector prefix on x32 rather than compute the offset
based on __builtin_thread_pointer() with more instructions.

Thanks,

Mathieu
Mathieu Desnoyers Jan. 11, 2025, 3:22 p.m. UTC | #5 
On 11-Jan-2025 02:58:30 PM, Andreas K. Huettel wrote:
> Since i386/nptl/rseq-access.h (which has only one ABI) and x86_64/nptl/rseq-access.h
> are the only non-generic versions of this file, 
> 
> am I assuming correctly that we should *not* expect any surprises from exotic ABI on
> other types of machines (say 31bit s390 or mips n32)?

Correct, all other architectures use
sysdeps/unix/sysv/linux/rseq-internal.h:

static inline struct rseq_area *
RSEQ_SELF (void)
{
  return (struct rseq_area *) ((char *) __thread_pointer () + __rseq_offset);
}

Which should work everywhere.

Thanks,

Mathieu
Mathieu Desnoyers Jan. 11, 2025, 3:23 p.m. UTC | #6 
On 11-Jan-2025 07:04:32 AM, Sam James wrote:
> "H.J. Lu" <hjl.tools@gmail.com> writes:
> 
> > commit 494d65129ed5ae1154b75cc189bbdde5e9ecf1df
> > Author: Michael Jeanson <mjeanson@efficios.com>
> > Date:   Thu Aug 1 10:35:34 2024 -0400
> >
> >     nptl: Introduce <rseq-access.h> for RSEQ_* accessors
> >
> > added things like
> >
> >        asm volatile ("movl %%fs:%P1(%q2),%0"                                  \
> >                      : "=r" (__value)                                         \
> >                      : "i" (offsetof (struct rseq_area, member)),             \
> >                        "r" (__rseq_offset));
> >
> > But this doesn't work for x32 when __rseq_offset is negative since the
> > address is computed as
> >
> > FS + 32-bit to 64-bit zero extension of __rseq_offset
> > + offsetof (struct rseq_area, member)
> >
> > Move x86_64/nptl/rseq-access.h to x86_64/nptl/64/rseq-access.h so that
> > sysdeps/nptl/rseq-access.h is used for x32.  This fixes BZ #32543.
> >
> > Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> > ---
> >  sysdeps/x86_64/nptl/{ => 64}/rseq-access.h | 0
> >  1 file changed, 0 insertions(+), 0 deletions(-)
> >  rename sysdeps/x86_64/nptl/{ => 64}/rseq-access.h (100%)
> >
> > diff --git a/sysdeps/x86_64/nptl/rseq-access.h b/sysdeps/x86_64/nptl/64/rseq-access.h
> > similarity index 100%
> > rename from sysdeps/x86_64/nptl/rseq-access.h
> > rename to sysdeps/x86_64/nptl/64/rseq-access.h
> 
> OK, thanks.
> 
> Michael et. al may have followup comments if they want to provide a
> specialised impl for x32 of course.

The approach proposed by Florian is my favorite (explicitly cast the
input operand to long long int). The same code will then work for both
x86-64 and x32.

Thanks,

Mathieu
Sam James Jan. 12, 2025, 1:45 a.m. UTC | #7 
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:

> On 11-Jan-2025 07:04:32 AM, Sam James wrote:
>> "H.J. Lu" <hjl.tools@gmail.com> writes:
>> 
>> > commit 494d65129ed5ae1154b75cc189bbdde5e9ecf1df
>> > Author: Michael Jeanson <mjeanson@efficios.com>
>> > Date:   Thu Aug 1 10:35:34 2024 -0400
>> >
>> >     nptl: Introduce <rseq-access.h> for RSEQ_* accessors
>> >
>> > added things like
>> >
>> >        asm volatile ("movl %%fs:%P1(%q2),%0"                                  \
>> >                      : "=r" (__value)                                         \
>> >                      : "i" (offsetof (struct rseq_area, member)),             \
>> >                        "r" (__rseq_offset));
>> >
>> > But this doesn't work for x32 when __rseq_offset is negative since the
>> > address is computed as
>> >
>> > FS + 32-bit to 64-bit zero extension of __rseq_offset
>> > + offsetof (struct rseq_area, member)
>> >
>> > Move x86_64/nptl/rseq-access.h to x86_64/nptl/64/rseq-access.h so that
>> > sysdeps/nptl/rseq-access.h is used for x32.  This fixes BZ #32543.
>> >
>> > Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
>> > ---
>> >  sysdeps/x86_64/nptl/{ => 64}/rseq-access.h | 0
>> >  1 file changed, 0 insertions(+), 0 deletions(-)
>> >  rename sysdeps/x86_64/nptl/{ => 64}/rseq-access.h (100%)
>> >
>> > diff --git a/sysdeps/x86_64/nptl/rseq-access.h b/sysdeps/x86_64/nptl/64/rseq-access.h
>> > similarity index 100%
>> > rename from sysdeps/x86_64/nptl/rseq-access.h
>> > rename to sysdeps/x86_64/nptl/64/rseq-access.h
>> 
>> OK, thanks.
>> 
>> Michael et. al may have followup comments if they want to provide a
>> specialised impl for x32 of course.
>
> The approach proposed by Florian is my favorite (explicitly cast the
> input operand to long long int). The same code will then work for both
> x86-64 and x32.

WFM, thanks!

>
> Thanks,
>
> Mathieu
diff mbox series

Patch

diff --git a/sysdeps/x86_64/nptl/rseq-access.h b/sysdeps/x86_64/nptl/64/rseq-access.h
similarity index 100%
rename from sysdeps/x86_64/nptl/rseq-access.h
rename to sysdeps/x86_64/nptl/64/rseq-access.h