From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x1033.google.com (mail-pj1-x1033.google.com [IPv6:2607:f8b0:4864:20::1033]) by sourceware.org (Postfix) with ESMTPS id 73D2F3858D3C for ; Mon, 24 Jan 2022 09:42:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 73D2F3858D3C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=sifive.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=sifive.com Received: by mail-pj1-x1033.google.com with SMTP id s61-20020a17090a69c300b001b4d0427ea2so19955042pjj.4 for ; Mon, 24 Jan 2022 01:42:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sifive.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=vUBg7ur15nRasvFFBQ0Uyxkc6iCwx1zH6VveDNJGxLY=; b=I3G/yr6B2YmXmWX/PPS2I/nGxdaR12ietAcYypDYIvOPsjMA3RB5Op60Q3LwXJcvpl LIwMlf17gN7LiLtiCF5zK+d8F/kMPW67tF2aN8piKaRAbwW4YSaFp9mMvUYbc0gqo/Yr bkbLO5fvArH1rf/As4WGivdwCxc5h7R5lUcbT3fUVUEljKWIhLpxCGIQKbwDk6ry6Uf8 t3uUEPRVao7FsyfcBhqGKbcfjmEniy2rkPAHfwS4wCfCo+vpiQaHIbYw2nBQhN/QX9Fh iocSBkpO1LirQ2L0IT8PiimdiibPHareQYle+G5T4KhS8S1bcKLqECb/eRzghMVxKnSJ WG5Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=vUBg7ur15nRasvFFBQ0Uyxkc6iCwx1zH6VveDNJGxLY=; b=CQFI4xH2PXCtf23ssqaCg2f8s+rOWfxakBwIn3hqG9KE5AwgWXZZWxNUbkxeoa+kfY gh1pLNjkP+C3K5lNCs8e1yz5igOWl9+T2ZSB0MXYwTDvOr7M2KoD9eZ/WqL2eQ/U9Wl/ PLtEBkzYUjCu/JEi8fqA53W7d3haHCuDngRIo4f73DSzGEeS7Noh03zM/TD5ZGbgcivH VZ5Op3wbaE5GleagmIdQjLYGExvc5JcchYsBWN7blRQ0BhgCcgJv5mJyc1awoqN4xKx7 txc7VeripvbfQmKpcuN+1CjjBj5mm2lhYQljywGqTXpBNDr+aiOL7hF/ENE9irW85pnk dJGQ== X-Gm-Message-State: AOAM530Z7v2hemsdf65/HwtauAznQu93P8oRls2ikwlQtOwsjbTGiHkO QHoO0+9q7vOCvGojiJIV+cHsxgWGKZEP5F87e47zVg== X-Google-Smtp-Source: ABdhPJyAkw7QPLNd0xaPqSgZ71CGr4WoPEK7NpmbbH3T426Ho45j6KxqC9XjudFtXNs+JvYdYp/KpeOCr3HAn7TKR9Q= X-Received: by 2002:a17:90b:1b0d:: with SMTP id nu13mr1118773pjb.24.1643017373298; Mon, 24 Jan 2022 01:42:53 -0800 (PST) MIME-Version: 1.0 References: <20220118043159.27521-2-vincent.chen@sifive.com> In-Reply-To: From: Vincent Chen Date: Mon, 24 Jan 2022 17:42:42 +0800 Message-ID: Subject: Re: [PATCH v2 1/2] RISC-V: remove riscv-specific sigcontext.h To: Kito Cheng Cc: Palmer Dabbelt , GNU C Library , Darius Rad , Andrew Waterman , DJ Delorie , Greentime Hu Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 24 Jan 2022 09:42:57 -0000 On Fri, Jan 21, 2022 at 9:29 AM Vincent Chen wrote: > > On Thu, Jan 20, 2022 at 10:47 AM Kito Cheng wrote: > > > > Just provide a tricky workaround from GCC side: > > > > +#ifdef _ASM_RISCV_SIGCONTEXT_H > > +#define SIGCONTEXT_PC(SC) (SC)->sc_regs.pc > > +#else > > +#define SIGCONTEXT_PC(SC) (SC)->gregs[0] > > +#endif > > > > https://gcc.gnu.org/pipermail/gcc-patches/2022-January/588682.html > > > > > > On Thu, Jan 20, 2022 at 10:36 AM Palmer Dabbelt wrote: > > > > > > On Mon, 17 Jan 2022 20:31:58 PST (-0800), vincent.chen@sifive.com wrote: > > > > Remove riscv-specific sigcontext.h so that Glibc can directly use > > > > sigcontext.h provided by the kernel to reduce synchronization work > > > > when new extension support is introduced. > > > > --- > > > > .../unix/sysv/linux/riscv/bits/sigcontext.h | 31 ------------------- > > > > 1 file changed, 31 deletions(-) > > > > delete mode 100644 sysdeps/unix/sysv/linux/riscv/bits/sigcontext.h > > > > > > > > diff --git a/sysdeps/unix/sysv/linux/riscv/bits/sigcontext.h b/sysdeps/unix/sysv/linux/riscv/bits/sigcontext.h > > > > deleted file mode 100644 > > > > index b6e15b5f62..0000000000 > > > > --- a/sysdeps/unix/sysv/linux/riscv/bits/sigcontext.h > > > > +++ /dev/null > > > > @@ -1,31 +0,0 @@ > > > > -/* Machine-dependent signal context structure for Linux. RISC-V version. > > > > - Copyright (C) 1996-2022 Free Software Foundation, Inc. This file is part of the GNU C Library. > > > > - > > > > - The GNU C Library is free software; you can redistribute it and/or > > > > - modify it under the terms of the GNU Lesser General Public > > > > - License as published by the Free Software Foundation; either > > > > - version 2.1 of the License, or (at your option) any later version. > > > > - > > > > - The GNU C Library is distributed in the hope that it will be useful, > > > > - but WITHOUT ANY WARRANTY; without even the implied warranty of > > > > - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > > > - Lesser General Public License for more details. > > > > - > > > > - You should have received a copy of the GNU Lesser General Public > > > > - License along with the GNU C Library. If not, see > > > > - . */ > > > > - > > > > -#ifndef _BITS_SIGCONTEXT_H > > > > -#define _BITS_SIGCONTEXT_H 1 > > > > - > > > > -#if !defined _SIGNAL_H && !defined _SYS_UCONTEXT_H > > > > -# error "Never use directly; include instead." > > > > -#endif > > > > - > > > > -struct sigcontext { > > > > - /* gregs[0] holds the program counter. */ > > > > - unsigned long int gregs[32]; > > > > - unsigned long long int fpregs[66] __attribute__ ((__aligned__ (16))); > > > > -}; > > > > - > > > > -#endif > > > > > > This will definitely break API compatibility (the fields have > > > different names) but should be fine for ABI compatibility. IIUC that's > > > within the rules, but I'm not sure it's a desirable outcome. Probably > > > would have been better to get this right the first time around, but I'm > > > not sure it's worth fixing -- essentially we're making a bunch of users > > > change things so we don't have to. That said, it's pretty ugly to have > > > two different definitions of a structure with the same name and layout. > > > > > > Maybe there's some sort of macro-related trick we can use? ie, provide > > > the current definition unless users opt into the Linux one (presumably > > > so they can talk about the V state). There's going to be some hoops to > > > jump through there to maintain ABI compatibility either way, so it's > > > possible we could tie these two together? > > I can understand what you are worried about. Therefore, I also tried > to build multiple Yocto images, such as core-image-full-cmdline, > core-image-x11, core-image-sato, and core-image-base, to evaluate the > impact. After applying Kito's solution to GCC, I didn't get any build > errors. According to the results, maybe we can have a quick conclusion > about the impact of this patch. > > The new version Glibc is not compatible with the old version GCC (The > old Glibc is still compatible with the new version GCC due to Kito's > patch) > Some public programs that use struct sigcontext are not covered by > this test. (If someone can tell me which program I'm missing, I'm > willing to fix it) > Some in-house programs use struct sigcontext_t to access signal stack. > > IMO, the impact seems not severe at this moment. Directly using the > kernel's sigcontext can get us away from the pain if we need to add > new registers to the signal context for a new extension or vendor > customized extension. > > In addition, I was keeping to find a suitable solution to solve it as > you described. But if I still cannot come up with a solution, do you > mind that bits/sigcontext.h has a duplicate data struct related to V > extension? Thank you I have some findings these days. Please allow me to update some descriptions. For the evaluation of deprecating the Glibc self-defined sigcontext.h (riscv/bits/sigcontext.h), thanks to David Abdurachmanov's suggestion, I used "world" instead of a particular image. It makes me able to build every single package available in all the layers. It effectively extends the test coverage. Fortunately, I didn't encounter any compile errors due to sigcontext.h deprecation. In addition, I found using "anonymous union" may suffice to avoid API breakage. According to Greentime's vector patch(https://lore.kernel.org/lkml/202111112102.qLSAjr3Q-lkp@intel.com/T/), the following is associated modification in Glibc sysdeps/unix/sysv/linux/riscv/bits/sigcontext.h --- a/sysdeps/unix/sysv/linux/riscv/bits/sigcontext.h +++ b/sysdeps/unix/sysv/linux/riscv/bits/sigcontext.h @@ -22,10 +22,22 @@ # error "Never use directly; include instead." #endif +#define sigcontext kernel_sigcontext +#include +#undef sigcontext + struct sigcontext { /* gregs[0] holds the program counter. */ - unsigned long int gregs[32]; - unsigned long long int fpregs[66] __attribute__ ((__aligned__ (16))); + __extension__ union { + unsigned long int gregs[32]; + struct user_regs_struct sc_regs; + }; + __extension__ union { + unsigned long long int fpregs[66] __attribute__ ((__aligned__ (16))); + union __riscv_fp_state sc_fpregs; + }; + __u8 __reserved[4224] __attribute__((__aligned__(16))); }; #endif In this modification, users do not need to specify which sigcontext.h he wants to choose. Through the anonymous union, the current member of struct sigcontext, gregs and fpregs, declared in riscv/bits/sigcontext.h is still available. Besides, by redefining struct sigcontext as struct kernel_sigcontext before including asm/sigcontext.h, riscv/bits/sigcontext.h can directly include asm/sigcontext.h to introduce the register data structure of new extension without duplicating the same definitions in Glibc. The modifications can pass the Glibc test suite without additional errors. By the way, I found that the definition of struct sigcontext in riscv/bits/sigcontext.h and asm/sigcontext.h is a bit different now. The memory size of fpregs[66] in riscv/bits/sigcontext.h equals to __riscv_fp_state sc_fpregs in asm/sigcontext.h, but their memory layout is different. Users cannot always use fpregs[x] to get the associated x-th FPU registers from the signal stack in all situations. It may be a minus case, but it points out the potential synchronization issue. Therefore, if possible, I still hope to deprecate the riscv/bits/sigcontext.h.