Re: [PATCH v3] RISC-V: Implement TLS Descriptors.

[Fangrui Song <maskray@google.com>]

On Thu, Sep 14, 2023 at 5:09 AM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 14/09/23 05:39, Tatsuyuki Ishi wrote:
> >
> >> On Sep 14, 2023, at 4:14, Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote:
> >>
> >> How did you actually build glibc? I saw multiple build issues with default
> >> configuration and even with --disable-werror, so I am doubtful that this
> >> patch was really proper tested. Please ensure that have-mtls-dialect-gnu2
> >> is set to 'yes' on config.make so the tests are actually run.
> >
> > I’m sorry I’ve made multiple mistakes here. There were actually two prerequisite commits but I’ve forgot to include them in the patch series. This will be included in v4.
> >
> > I used [1] to build a full toolchain and it defaulted to --disable-werror. I’ve manually enabled -Werror and fixed all compiler warnings in v4.
>
> For patch development I would advise strongly to use --disable-werror,
> the patchwork hasn't flag it because we do not build/run on riscv.
>
> >
> > As for have-mtls-dialect-gnu2, RISC-V will use AArch64-style flags (-mtls-dialect={trad,desc}), not gnu2. However, I have configured my GCC fork with --with-tls=desc and all compilation is done with TLSDESC by default for my testing.
>
> So I take that the default would be still the -mtls-dialect=trad. In
> this case I would suggest to change the have-mtls-dialect-gnu2 to be
> enabled for -mtls-dialect=desc as well, the elf tests are generic and
> should exercise both mode in a default configuration.

Agree. We need to test both -mtls-dialect=trad and -mtls-dialect=desc.
We will likely need a variable like have-mtls-dialect-desc...

> >
> > I assumed most testing was done through GCC’s testsuite, and I’ve got GCC’s testsuite to the point of no regression, however I was wrong and there are more in glibc’s testsuite. For v4 I’ve ran all tests in glibc/elf/, and all but two tests for TLS on static executables are passing. More info on my plan for fixing that in v4.
> >

Latest lld (https://github.com/llvm/llvm-project/pull/79239) supports
TLSDESC and it optimizes TLSDESC to LE/IE regardless of R_RISCV_RELAX,
so it is compatible with the static executable tests.

Reviewers can create /usr/local/bin/ld as a symlink to the latest lld
and do a GCC build:)

On the LLVM side, the RISC-V TLSDESC work (LLVM, Clang, lld) has been
completed today.
https://maskray.me/blog/2024-01-23-riscv-tlsdesc-works
Clang cannot build glibc, but there may be something to use Clang to
build just the elf/ tests :) ?

> I am not sure about gcc testsuite, but it would be good RISCV to check
> for both mode and not only stress the compiler default. Unfortunately
> not all ports follow this.
>
> >>
> >>> # RISC-V's assembler also needs to know about PIC as it changes the definition
> >>> # of some assembler macros.
> >>> ASFLAGS-.os += $(pic-ccflag)
> >>> diff --git a/sysdeps/riscv/dl-lookupcfg.h b/sysdeps/riscv/dl-lookupcfg.h
> >>> new file mode 100644
> >>> index 0000000000..d7fe73636b
> >>> --- /dev/null
> >>> +++ b/sysdeps/riscv/dl-lookupcfg.h
> >>> @@ -0,0 +1,27 @@
> >>> +/* Configuration of lookup functions.
> >>> +   Copyright (C) 2006-2023 Free Software Foundation, Inc.
> >>
> >> I think it should be only 2023 for new code.
> >
> > Ack, all copyright headers for new files will be 2023 only in v4.

Perhaps 2024 for v5 :)

> >>>    | (ELF_RTYPE_CLASS_COPY * ((type) == R_RISCV_COPY)))
> >>>
> >>> /* Return nonzero iff ELF header is compatible with the running host.  */
> >>> @@ -219,6 +221,32 @@ elf_machine_rela (struct link_map *map, struct r_scope_elem *scope[],
> >>> }
> >>>       break;
> >>>
> >>> +    case R_RISCV_TLSDESC:
> >>> +      struct tlsdesc *td = (struct tlsdesc *) addr_field;
> >>> +      if (sym == NULL)
> >>> +{
> >>> +  td->entry = _dl_tlsdesc_undefweak;
> >>
> >>
> >> This triggers multiple compiler warnings:
> >>
> >> ../sysdeps/riscv/dl-machine.h: In function ‘elf_machine_rela’:
> >> ../sysdeps/riscv/dl-machine.h:228:21: error: assignment to ‘ptrdiff_t (*)(struct tlsdesc *)’ {aka ‘long int (*)(struct tlsdesc *)’} from incompatible pointer type ‘long unsigned int (*)(struct tlsdesc *)’ [-Werror=incompatible-pointer-types]
> >>  228 |           td->entry = _dl_tlsdesc_undefweak;
> >>      |                     ^
> >> ../sysdeps/riscv/dl-machine.h:244:25: error: assignment to ‘ptrdiff_t (*)(struct tlsdesc *)’ {aka ‘long int (*)(struct tlsdesc *)’} from incompatible pointer type ‘long unsigned int (*)(struct tlsdesc *)’ [-Werror=incompatible-pointer-types]
> >>  244 |               td->entry = _dl_tlsdesc_return;
> >>      |                         ^
> >>
> >> Because you declare _dl_tlsdesc_undefweak as:
> >>
> >>  unsigned long _dl_tlsdesc_dynamic (struct tlsdesc *);
> >>
> >> But the 'entry' at tlsdesc as:
> >>
> >>   ptrdiff_t (*entry) (struct tlsdesc *);
> >>
> >> Based on TLSDESC ABI I think using a unsigned as return value is wrong here.
> >
> > I am opting to not using ptrdiff_t because the offset can be larger than PTRDIFF_MAX. Using unsigned arithmetic avoids the signed overflow concern.
> >
> > The descriptor signature is also defined in the RISC-V psABI as returning unsigned long for the same reason.
> >
> > [1]: https://github.com/riscv-collab/riscv-gnu-toolchain
> >
>
> The C standard specified that subtracting two points should be signed
> integer type, which maps to ptrdiff_t on most C and POSIX interfaces.
> The malloc would fail with requests larger than ptrdiff_t (check
> 9bf8e29ca136094f73f69f725f15c51facc97206 and BZ#23741) and gcc might
> generate wrong optimizations in such cases (we still allow values
> larger than ptrdiff_t for mmap, but some other libc like bionic and
> musl will fail even for mmap).
>
> Although the required code that operates with tlsdesc uses a
> non-standard ABI, meaning most code would either use assembly or C with
> some extra care, I really don't see a compelling reason to have RISCV
> deviates from other ports that implemented TLSDESC.
>
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=23741
> [2] https://gcc.gnu.org/bugzilla//show_bug.cgi?id=67999



-- 
宋方睿