Re: [PATCH v6 0/3] RISC-V: Implement TLS Descriptors.

[Tatsuyuki Ishi <ishitatsuyuki@gmail.com>]

> On May 1, 2024, at 3:33, Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote:
> 
> On 30/04/24 14:05, Palmer Dabbelt wrote:
>> On Thu, 28 Mar 2024 23:18:31 PDT (-0700), ishitatsuyuki@gmail.com wrote:
>>> This patchset implements TLS Descriptors (TLSDESC) for RISC-V targets, per
>>> the ratified specification at [1].
>>> 
>>> Partial support for TLSDESC is in binutils trunk. In [2] there is
>>> additional support for relaxation which is required to pass
>>> tst-tls1-static. GCC needs to be patched with [3].
>>> 
>>> Passes binutils and gcc tests for rv32gcv and rv64gcv. For glibc I only
>>> tested elf/ as other subsystems sometimes doesn't work well under qemu.
>>> 
>>> This contribution is made on behalf of Blue Whale Systems, which has
>>> copyright assignment on file with the FSF.
>>> 
>>> v2: Fix end-of-file newlines.
>>> v3: Fix segfaulting on the slow path of TLSDESC resolver.
>>>     Fix handling of lazy relocations.
>>> v4: Fix compiler warnings.
>>>     Fix fast path stack alignment pointed out by Andrew.
>>>     Fix style issues pointed out by Adhemerval.
>>>     Include 2 missing prerequisite commits.
>>>     Update localplt list.
>>> v5: Add stubs for save/restore of vector registers.
>>>     Add comments and reordered sections for readability in the TLSDESC assembly.
>>>     Add clobber test sequence for tst-gnu2-tls2.
>>> v6: Fix R_RISCV_NUM.
>>>     Elaborate on the include guard commit (thanks Fangrui).
>>> 
>>> [1]: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/373
>>> [2]: https://inbox.sourceware.org/binutils/20240220175556.304692-1-ishitatsuyuki@gmail.com/
>>> [3]: https://inbox.sourceware.org/gcc-patches/20240329055239.29719-1-ishitatsuyuki@gmail.com/
>>> 
>>> Tatsuyuki Ishi (3):
>>>   RISC-V: Add include guard for dl-tls.h.
>>>   RISC-V: Add TLSDESC reloc definitions.
>>>   RISC-V: Implement TLS Descriptors.
>>> 
>>>  elf/elf.h                                   |   7 +-
>>>  sysdeps/riscv/Makefile                      |  10 +
>>>  sysdeps/riscv/dl-lookupcfg.h                |  27 ++
>>>  sysdeps/riscv/dl-machine.h                  |  50 +++-
>>>  sysdeps/riscv/dl-tls.h                      |   4 +
>>>  sysdeps/riscv/dl-tlsdesc.S                  | 269 ++++++++++++++++++++
>>>  sysdeps/riscv/dl-tlsdesc.h                  |  48 ++++
>>>  sysdeps/riscv/linkmap.h                     |   1 +
>>>  sysdeps/riscv/preconfigure                  |   1 +
>>>  sysdeps/riscv/tlsdesc.c                     |  38 +++
>>>  sysdeps/riscv/tlsdesc.sym                   |  19 ++
>>>  sysdeps/riscv/tst-gnu2-tls2.c               |  33 +++
>>>  sysdeps/unix/sysv/linux/riscv/localplt.data |   2 +
>>>  13 files changed, 507 insertions(+), 2 deletions(-)
>>>  create mode 100644 sysdeps/riscv/dl-lookupcfg.h
>>>  create mode 100644 sysdeps/riscv/dl-tlsdesc.S
>>>  create mode 100644 sysdeps/riscv/dl-tlsdesc.h
>>>  create mode 100644 sysdeps/riscv/tlsdesc.c
>>>  create mode 100644 sysdeps/riscv/tlsdesc.sym
>>>  create mode 100644 sysdeps/riscv/tst-gnu2-tls2.c
>> 
>> This generally LGTM, but IIUC we're waiting on a glibc-wide decision as to what the stable ABIs should be around TLS before moving forward.
> 
> If I recall correctly, my latest remarks for this patch was:
> 
> 1. Missing gcc/binutils upstream support (usually we wait the patches 
>   to be upstream on the required projects)

For a status update, binutils have partial TLSDESC support (without relaxation) committed and GCC have all the required patches committed. This means tst-tls-static-1 will still fail with upstream toolchain, though.

> 2. Call the internal __tls_get_addr on _dl_tlsdesc_dynamic to avoid
>   the PLT call.

This was a trivial change, and I will include it in the next version.

> 3. Decide how RISCV would like to handle different ABI constraint for
>   the _dl_tlsdesc_dynamic, now that you are moving to support ifunc
>   and thus allowing a more generic glibc build.  You can either make
>   the _dl_tlsdesc_dynamic with multiple ifunc variants (as x86 does),
>   or check the chip support through the riscv_hwprobe on the function
>   itself and only save/restore the support ABI register.

Note that we don’t really have any of the cpu_features scaffolding in RISC-V, so going down the IFUNC route seems a lot more annoying. I’m working on adding a call to riscv_hwprobe in _dl_tlsdesc_dynamic. I need some more time to figure out how to configure user mode QEMU to test all the code paths.

As for TLS ABI decision, it would be good if we decide on a calling convention, but shipping a stub that saves / restores all vector registers should also be OK (which is what the current revision do).

Tatsuyuki