From: "H.J. Lu" <hjl.tools@gmail.com>
To: Florian Weimer <fweimer@redhat.com>
Cc: libc-alpha@sourceware.org
Subject: Re: [PATCH] x86-64: Stack alignment in _dl_tlsdesc_dynamic and red zone usage (bug 31501)
Date: Sat, 16 Mar 2024 18:19:14 -0700 [thread overview]
Message-ID: <CAMe9rOrsg7NM8tE7LsDF_K0-MTwZCe1-6EnFyFQTPao2PKmHzQ@mail.gmail.com> (raw)
In-Reply-To: <CAMe9rOrLomPisUr3VdYpsH7PcuKG2kg8F5gnv5JepDOpM84BRQ@mail.gmail.com>
On Sat, Mar 16, 2024 at 3:05 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Sat, Mar 16, 2024 at 10:51 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Sat, Mar 16, 2024 at 10:42 AM Florian Weimer <fweimer@redhat.com> wrote:
> > >
> > > * H. J. Lu:
> > >
> > > > Please verify if this is the right testcase.
> > >
> > > Test case works (fails without my fix, succeeds with my fix). Some
> > > comments below.
> > >
> > > > diff --git a/sysdeps/x86_64/tst-gnu2-tls2-x86-64-mod0.S b/sysdeps/x86_64/tst-gnu2-tls2-x86-64-mod0.S
> > > > new file mode 100644
> > > > index 0000000000..8129b28061
> > > > --- /dev/null
> > > > +++ b/sysdeps/x86_64/tst-gnu2-tls2-x86-64-mod0.S
> > > > @@ -0,0 +1,57 @@
> > >
> > > > + .text
> > > > + .p2align 4
> > > > + .globl apply_tls
> > > > + .type apply_tls, @function
> > > > +apply_tls:
> > > > + .cfi_startproc
> > >
> > > Missing CET marker.
> > >
> > > > + subq $24, %rsp
> > > > + .cfi_def_cfa_offset 32
> > > > + movdqu (%rdi), %xmm0
> > > > + movq %fs:40, %rax
> > > > + movq %rax, 8(%rsp)
> > > > + xorl %eax, %eax
> > > > + leaq tls_var0@TLSDESC(%rip), %rax
> > > > + call *tls_var0@TLSCALL(%rax)
> > > > + addq %fs:0, %rax
> > > > + movups %xmm0, (%rax)
> > > > + movdqu 16(%rdi), %xmm1
> > > > + movups %xmm1, 16(%rax)
> > > > + movq 8(%rsp), %rdx
> > > > + subq %fs:40, %rdx
> > > > + jne .L5
> > > > + addq $24, %rsp
> > > > + .cfi_remember_state
> > > > + .cfi_def_cfa_offset 8
> > > > + ret
> > > > +.L5:
> > > > + .cfi_restore_state
> > > > + call __stack_chk_fail@PLT
> > >
> > > Not sure if we need this?
> > >
> > > Maybe add some comment what exactly this subtest is exercising?
> > >
> > > These are present in the other TLS modules as well.
> > >
> > > > diff --git a/sysdeps/x86_64/tst-gnu2-tls2-x86-64-mod1.S b/sysdeps/x86_64/tst-gnu2-tls2-x86-64-mod1.S
> > > > new file mode 100644
> > > > index 0000000000..af4b7ca761
> > > > --- /dev/null
> > > > +++ b/sysdeps/x86_64/tst-gnu2-tls2-x86-64-mod1.S
> > >
> > > > +/* Select an offset which will cause _dl_tlsdesc_dynamic_xsavec to
> > > > + clobber %rbx. */
> > > > +#define OFFSET (56 + 16 + 16 + 16)
> > > > +
> > > > + .text
> > > > + .p2align 4
> > > > + .globl apply_tls
> > > > + .type apply_tls, @function
> > > > +apply_tls:
> > > > + .cfi_startproc
> > > > + pushq %rbp
> > > > + .cfi_def_cfa_offset 16
> > > > + .cfi_offset 6, -16
> > > > + movq %rsp, %rbp
> > > > + .cfi_def_cfa_register 6
> > > > + /* Align stack to 64 bytes. */
> > > > + andq $-64, %rsp
> > > > + pushq %rbx
> > > > + subq $OFFSET, %rsp
> > >
> > > The offset could be loaded from a global variable or something like
> > > that. We should exercise a wide range of stack alignments—the
> > > individual tests are cheap. And maybe check extra registers.
> >
> > I will clean it up with a different fix.
> >
>
> I submitted a patch with a testase:
>
> https://patchwork.sourceware.org/project/glibc/list/?series=31963
>
> My patch allocates 64 more bytes to avoid clobbering saved RDI,
> RSI and RBX values on stack by xsave. It avoids 2 stack
> adjustments. Either my fix or Florian's fix should fix the issue.
> I don't have a strong preference as long as my testcase is
> included.
>
>
I think my testcase may fail on AMD AVX CPUs without the
fix. On Intel AVX CPUs, the state size is 960 bytes. But the
last 128 bytes may be unused.
--
H.J.
next prev parent reply other threads:[~2024-03-17 1:19 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-16 14:32 Florian Weimer
2024-03-16 14:37 ` H.J. Lu
2024-03-16 14:43 ` H.J. Lu
2024-03-16 14:47 ` H.J. Lu
2024-03-16 15:04 ` Florian Weimer
2024-03-16 15:18 ` H.J. Lu
2024-03-16 16:32 ` H.J. Lu
2024-03-16 16:37 ` H.J. Lu
2024-03-16 17:42 ` Florian Weimer
2024-03-16 17:51 ` H.J. Lu
2024-03-16 22:05 ` H.J. Lu
2024-03-17 1:19 ` H.J. Lu [this message]
2024-03-17 3:14 ` H.J. Lu
2024-03-16 14:57 ` Florian Weimer
2024-03-16 14:59 ` H.J. Lu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMe9rOrsg7NM8tE7LsDF_K0-MTwZCe1-6EnFyFQTPao2PKmHzQ@mail.gmail.com \
--to=hjl.tools@gmail.com \
--cc=fweimer@redhat.com \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).