Re: [PATCH v4] x86-64: Allocate state buffer space for RDI, RSI and RBX

["H.J. Lu" <hjl.tools@gmail.com>]

On Mon, Mar 18, 2024 at 3:22 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > _dl_tlsdesc_dynamic preserves RDI, RSI and RBX before realigning stack.
> > After realigning stack, it saves RCX, RDX, R8, R9, R10 and R11.  Define
> > TLSDESC_CALL_REGISTER_SAVE_AREA to allocate space for RDI, RSI and RBX
> > to avoid clobbering saved RDI, RSI and RBX values on stack by xsave to
> > STATE_SAVE_OFFSET(%rsp).
> >
> >    +==================+<- stack frame start aligned at 8 or 16 bytes
>
> It's 8-byte aligned only?  If the caller uses the psABI convention, we
> have %rsp ≡ 8 (mod 16).

It is due to:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58066

Do we need to support older compilers?

>
> >    |                  |<- RDI
> >    |                  |<- RSI
> >    |                  |<- RBX
>
> I would add something like “originally in the red zone” here.

Fixed.

> >    |                  |<- paddings from stack realignment of 64 bytes
> >    |------------------|<- xsave buffer end aligned at 64 bytes
>
>
> >    |                  |<-
> >    |                  |<-
> >    |                  |<-
> >    |------------------|<- xsave buffer start at STATE_SAVE_OFFSET(%rsp)
> >    |                  |<- 8-byte padding
>
> Maybe add “to achieve 64-bit alignment”.

Fixed.

> >    |                  |<- 8-byte padding
> >    |                  |<- R11
> >    |                  |<- R10
> >    |                  |<- R9
> >    |                  |<- R8
> >    |                  |<- RDX
> >    |                  |<- RCX
> >    +==================+<- State buffer start aligned at 64 bytes
>
> Isn't this “%rsp  aligned at 64 bytes”?

Fixed.

> Likewise for the comment in the sources below.

Fixed.

> > diff --git a/sysdeps/x86/sysdep.h b/sysdeps/x86/sysdep.h
> > index db8e576e91..46fcd27345 100644
> > --- a/sysdeps/x86/sysdep.h
> > +++ b/sysdeps/x86/sysdep.h
> > @@ -46,6 +46,34 @@
> >     red-zone into account.  */
> >  # define STATE_SAVE_OFFSET (8 * 7 + 8)
>
> The comment on STATE_SAVE_OFFSET needs updating, too.

Fixed.

> I would like to see comments from Noah or Sunil.  You'll have to
> maintain this, too. 8-)

CCed Noah and Sunil.

> I find the macro consts rather confusing, but maybe that's just me.

Can you suggest a different name?

The v5 patch is at

https://patchwork.sourceware.org/project/glibc/list/?series=31992

Thanks.

-- 
H.J.