From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x230.google.com (mail-oi1-x230.google.com [IPv6:2607:f8b0:4864:20::230]) by sourceware.org (Postfix) with ESMTPS id 2CA7F3858CDA for ; Thu, 14 Sep 2023 12:09:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2CA7F3858CDA Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-oi1-x230.google.com with SMTP id 5614622812f47-3ab29af398aso522063b6e.1 for ; Thu, 14 Sep 2023 05:09:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1694693346; x=1695298146; darn=sourceware.org; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to; bh=WaJD3BBW2TPKHjfue7/shsa7OqPt4KePYJbvONgklc4=; b=hMscdWGgDpOlkmH9Ljz+VU9cehTxyK/V7gywM6zFk6kzFFacbwcUntjjzV0EJW/7y2 EAAUYgvCAZ8yDmFVML+Dkm5JeWbdDPDAuwkpO7p+GddvSgthfz8HBicB0KwkaVBEFSDq q1zoFK2LYrZitDQVSeAiVdzJAw28CXrvSa9vts0InSs4eqa2swqMkIpovckFmQP5znPP HaW/UxZhdLb4dx/W7FjgCduKC9d6Nr61W1IhRx1HqyGGcZHmXeU1wfDfB3dejo6SRZaI yRnpLFFAOWG3CW4MWYq2J3wW0IbMadJ883WzsKsUEIl5QyxqhCd8uiPBewQDydMWmV8S 66Eg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694693346; x=1695298146; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=WaJD3BBW2TPKHjfue7/shsa7OqPt4KePYJbvONgklc4=; b=Kus+1VqHITkV2MHd7TUKgBpe/RMC3dGjGJfk30dqLlKtWvJj9siLTPMwWqa7MSkaSR UkJ3IJvUPnaDvnoQf+iVT+q/sWMTO9IryGmCm4Z9wQqRrI2mbi+kEdIZ8av6p9ZQsHAq XpMB7mwh6av8jqK7pBG7G47mN1x5MoZyKJ/K21mN4WHqI02dnT47Sd4EHrY0stZL9znE +FT5orMNjyrcRTNEyjz7VAjNQFxcBvamYiTNV1rtO2haMTvX+bgv2F8ow3CBXFqEA3tC HABTIzLPdI69zv0EZjFOIrZz0aHoT4zGuRNwjy/JG561QQSriN2KvU5TAddesUJRJ0fD OfyQ== X-Gm-Message-State: AOJu0YzLK5x/7Faryi3zPYZBbvvbNGaZWaVAhJwh1qQdXFr82mlSkTMp 9A0Frb/G2pCJFybo41BgcWzEew== X-Google-Smtp-Source: AGHT+IH+4PFIzXVCW04vECn6PMH829pie4MCCKdaIDFbRa6pyvBBCb7NcBvy+7tLr8hBiFEc7xh5FQ== X-Received: by 2002:aca:2112:0:b0:3a3:6cb2:d5bf with SMTP id 18-20020aca2112000000b003a36cb2d5bfmr5101795oiz.4.1694693346395; Thu, 14 Sep 2023 05:09:06 -0700 (PDT) Received: from ?IPV6:2804:1b3:a7c0:91cb:8813:a8ad:85d6:b2a8? ([2804:1b3:a7c0:91cb:8813:a8ad:85d6:b2a8]) by smtp.gmail.com with ESMTPSA id fa12-20020a0568082a4c00b003a4243d034dsm521105oib.17.2023.09.14.05.09.04 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 14 Sep 2023 05:09:05 -0700 (PDT) Message-ID: <3288d163-d41f-cc12-3e13-154af48b7801@linaro.org> Date: Thu, 14 Sep 2023 09:09:01 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.15.0 Subject: Re: [PATCH v3] RISC-V: Implement TLS Descriptors. Content-Language: en-US To: Tatsuyuki Ishi Cc: libc-alpha@sourceware.org, Rui Ueyama , Rui Ueyama , schwab@linux-m68k.org References: <20230817181228.122674-2-ishitatsuyuki@gmail.com> <20230913172633.39229-1-ishitatsuyuki@gmail.com> <93e0270b-bdc8-3443-1d2a-947a31945d99@linaro.org> <9600C568-8A5B-457C-B726-D62D8C1EF226@gmail.com> From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: <9600C568-8A5B-457C-B726-D62D8C1EF226@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 14/09/23 05:39, Tatsuyuki Ishi wrote: > >> On Sep 14, 2023, at 4:14, Adhemerval Zanella Netto 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