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

[Tatsuyuki Ishi <ishitatsuyuki@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3770 bytes --]


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

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.

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.

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