Message ID | 20211215231229.434034-1-aurelien@aurel32.net |
---|---|
State | New |
Headers | show |
Series | riscv: align stack before calling _dl_init [BZ #28703] | expand |
Even though sp should be sufficiently aligned upon program entry, the _dl_skip_args jig can misalign the stack. So, LGTM. On Wed, Dec 15, 2021 at 3:12 PM Aurelien Jarno <aurelien@aurel32.net> wrote: > > Align the stack pointer to 128 bits during the call to _dl_init() as > specified by the RISC-V ABI [1]. This fixes the elf/tst-align2 test. > > Fixes bug 28703. > > [1] https://github.com/riscv-non-isa/riscv-elf-psabi-doc > --- > sysdeps/riscv/dl-machine.h | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h > index ce2b3c3875..f457a41c70 100644 > --- a/sysdeps/riscv/dl-machine.h > +++ b/sysdeps/riscv/dl-machine.h > @@ -125,8 +125,14 @@ elf_machine_dynamic (void) > sll a3, a1, " STRINGXP (PTRLOG) "\n\ > add a3, a3, a2\n\ > add a3, a3, " STRINGXP (SZREG) "\n\ > + # Stash the stack pointer in s1.\n\ > + mv s1, sp\n\ > + # Align stack to 128 bits for the _dl_init call.\n\ > + andi sp, sp,-16\n\ > # Call the function to run the initializers.\n\ > jal _dl_init\n\ > + # Restore the stack pointer for _start.\n\ > + mv sp, s1\n\ > # Pass our finalizer function to _start.\n\ > lla a0, _dl_fini\n\ > # Jump to the user entry point.\n\ > -- > 2.33.0 >
* Aurelien Jarno: > + # Align stack to 128 bits for the _dl_init call.\n\ > + andi sp, sp,-16\n\ > # Call the function to run the initializers.\n\ > jal _dl_init\n\ How is that extra alignment observable to user code? Thanks, Florian
On 2021-12-16 07:51, Florian Weimer wrote: > * Aurelien Jarno: > > > + # Align stack to 128 bits for the _dl_init call.\n\ > > + andi sp, sp,-16\n\ > > # Call the function to run the initializers.\n\ > > jal _dl_init\n\ > > How is that extra alignment observable to user code? This extra alignment is visible to the constructors, called from _dl_init. This is how this is tested in elf/tst-align2.c, however the issue wasn't detected due to BZ #27901.
On Dez 16 2021, Aurelien Jarno wrote: > diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h > index ce2b3c3875..f457a41c70 100644 > --- a/sysdeps/riscv/dl-machine.h > +++ b/sysdeps/riscv/dl-machine.h > @@ -125,8 +125,14 @@ elf_machine_dynamic (void) > sll a3, a1, " STRINGXP (PTRLOG) "\n\ > add a3, a3, a2\n\ > add a3, a3, " STRINGXP (SZREG) "\n\ > + # Stash the stack pointer in s1.\n\ > + mv s1, sp\n\ > + # Align stack to 128 bits for the _dl_init call.\n\ > + andi sp, sp,-16\n\ > # Call the function to run the initializers.\n\ > jal _dl_init\n\ > + # Restore the stack pointer for _start.\n\ > + mv sp, s1\n\ > # Pass our finalizer function to _start.\n\ > lla a0, _dl_fini\n\ > # Jump to the user entry point.\n\ Shouldn't the adjustment already be made after the _dl_skip_args fixup, permanently?
On 2021-12-16 09:48, Andreas Schwab wrote: > On Dez 16 2021, Aurelien Jarno wrote: > > > diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h > > index ce2b3c3875..f457a41c70 100644 > > --- a/sysdeps/riscv/dl-machine.h > > +++ b/sysdeps/riscv/dl-machine.h > > @@ -125,8 +125,14 @@ elf_machine_dynamic (void) > > sll a3, a1, " STRINGXP (PTRLOG) "\n\ > > add a3, a3, a2\n\ > > add a3, a3, " STRINGXP (SZREG) "\n\ > > + # Stash the stack pointer in s1.\n\ > > + mv s1, sp\n\ > > + # Align stack to 128 bits for the _dl_init call.\n\ > > + andi sp, sp,-16\n\ > > # Call the function to run the initializers.\n\ > > jal _dl_init\n\ > > + # Restore the stack pointer for _start.\n\ > > + mv sp, s1\n\ > > # Pass our finalizer function to _start.\n\ > > lla a0, _dl_fini\n\ > > # Jump to the user entry point.\n\ > > Shouldn't the adjustment already be made after the _dl_skip_args fixup, > permanently? I have basically followed what is done on i386, mips or x86_64. I can give a try to keep the alignment permanent and see if things still work.
* Aurelien Jarno: > On 2021-12-16 07:51, Florian Weimer wrote: >> * Aurelien Jarno: >> >> > + # Align stack to 128 bits for the _dl_init call.\n\ >> > + andi sp, sp,-16\n\ >> > # Call the function to run the initializers.\n\ >> > jal _dl_init\n\ >> >> How is that extra alignment observable to user code? > > This extra alignment is visible to the constructors, called from > _dl_init. This is how this is tested in elf/tst-align2.c, however the > issue wasn't detected due to BZ #27901. Ah, sorry, I misread “128 bits” as “128 bytes”. 16-byte alignment will be preserved by the compiler. Thanks, Florian
On Dez 16 2021, Aurelien Jarno wrote: > On 2021-12-16 09:48, Andreas Schwab wrote: >> On Dez 16 2021, Aurelien Jarno wrote: >> >> > diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h >> > index ce2b3c3875..f457a41c70 100644 >> > --- a/sysdeps/riscv/dl-machine.h >> > +++ b/sysdeps/riscv/dl-machine.h >> > @@ -125,8 +125,14 @@ elf_machine_dynamic (void) >> > sll a3, a1, " STRINGXP (PTRLOG) "\n\ >> > add a3, a3, a2\n\ >> > add a3, a3, " STRINGXP (SZREG) "\n\ >> > + # Stash the stack pointer in s1.\n\ >> > + mv s1, sp\n\ >> > + # Align stack to 128 bits for the _dl_init call.\n\ >> > + andi sp, sp,-16\n\ >> > # Call the function to run the initializers.\n\ >> > jal _dl_init\n\ >> > + # Restore the stack pointer for _start.\n\ >> > + mv sp, s1\n\ >> > # Pass our finalizer function to _start.\n\ >> > lla a0, _dl_fini\n\ >> > # Jump to the user entry point.\n\ >> >> Shouldn't the adjustment already be made after the _dl_skip_args fixup, >> permanently? > > I have basically followed what is done on i386, mips or x86_64. I can > give a try to keep the alignment permanent and see if things still work. It looks like your version is ok, since _start is doing its own alignment (and it needs sp to point to argc).
On 2021-12-16 11:42, Florian Weimer via Libc-alpha wrote: > * Aurelien Jarno: > > > On 2021-12-16 07:51, Florian Weimer wrote: > >> * Aurelien Jarno: > >> > >> > + # Align stack to 128 bits for the _dl_init call.\n\ > >> > + andi sp, sp,-16\n\ > >> > # Call the function to run the initializers.\n\ > >> > jal _dl_init\n\ > >> > >> How is that extra alignment observable to user code? > > > > This extra alignment is visible to the constructors, called from > > _dl_init. This is how this is tested in elf/tst-align2.c, however the > > issue wasn't detected due to BZ #27901. > > Ah, sorry, I misread “128 bits” as “128 bytes”. 16-byte alignment will > be preserved by the compiler. I agree that's a bit unusual to use bits instead of bytes there, but I used the same terminology as in the RISC-V ABI.
diff --git a/sysdeps/riscv/dl-machine.h b/sysdeps/riscv/dl-machine.h index ce2b3c3875..f457a41c70 100644 --- a/sysdeps/riscv/dl-machine.h +++ b/sysdeps/riscv/dl-machine.h @@ -125,8 +125,14 @@ elf_machine_dynamic (void) sll a3, a1, " STRINGXP (PTRLOG) "\n\ add a3, a3, a2\n\ add a3, a3, " STRINGXP (SZREG) "\n\ + # Stash the stack pointer in s1.\n\ + mv s1, sp\n\ + # Align stack to 128 bits for the _dl_init call.\n\ + andi sp, sp,-16\n\ # Call the function to run the initializers.\n\ jal _dl_init\n\ + # Restore the stack pointer for _start.\n\ + mv sp, s1\n\ # Pass our finalizer function to _start.\n\ lla a0, _dl_fini\n\ # Jump to the user entry point.\n\