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

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

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)

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

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.