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

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

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.

> 
> 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.
> 

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.
> 
>>>    | (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