From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x629.google.com (mail-ej1-x629.google.com [IPv6:2a00:1450:4864:20::629]) by sourceware.org (Postfix) with ESMTPS id 71FD03858404 for ; Fri, 5 Aug 2022 08:45:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 71FD03858404 Received: by mail-ej1-x629.google.com with SMTP id c24so3770935ejd.11 for ; Fri, 05 Aug 2022 01:45:11 -0700 (PDT) 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=hJ/j4Bk3AaFxQeuDlzZnTmkTzCIl8jKNWXn1F64nPR4=; b=EWjadbyyWNXlkl1SAq+0Q+tShiDlLkFj2vShQyUGuR5YbaxLpYmUeRdFIFfJCDot4r y1bDoq96RMgFXxiO2HaFoFQEqJ4BhvabmDif4/WFIwgIpHQmGY7dYOk4yg6nYMGUyx9q D+Uz8LX9L6sMhpfoQ9JpRI/K+7XJkra9ocWbt2qlUQirnf2byG63IaRFPiYfDPNpWX5T b1qJKVXSXcec4iVaM2h8IuUKaWexQbDZih9wYac8CnnyhAy7j0feCyYg4wObHHAYz8Wg lDRjtZD+BTKMCAe96Qs5tgxv0Hr6GvL1RJIBsD9As2Skx37T8QX3U0+1ZDX2L5vUUk0A WpRQ== X-Gm-Message-State: ACgBeo3iA0h8H5OElZd4LZlo0ygfdzhgHrCNTwDbQSJ3GflZYFUKDmic JSi4sRE6VjX61nBS4A90g5U4QaEr0eMoussJv1U= X-Google-Smtp-Source: AA6agR64fTIRKcK1icwS0KwDDXnqnXTA386IhgLFHlyAqUgI8O/+s7pnVvZjY1D92vCGnQMAEglwTmozQa0UxaH3nKQ= X-Received: by 2002:a17:907:738a:b0:730:7537:848c with SMTP id er10-20020a170907738a00b007307537848cmr4665342ejc.488.1659689110002; Fri, 05 Aug 2022 01:45:10 -0700 (PDT) MIME-Version: 1.0 References: <87sfmbxyyj.fsf@oracle.com> <87h72rumdq.fsf@oracle.com> In-Reply-To: <87h72rumdq.fsf@oracle.com> From: Richard Biener Date: Fri, 5 Aug 2022 10:44:57 +0200 Message-ID: Subject: Re: [PATCH] place `const volatile' objects in read-only sections To: "Jose E. Marchesi" Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Aug 2022 08:45:13 -0000 On Fri, Aug 5, 2022 at 10:30 AM Jose E. Marchesi wrote: > > > Hi Richard. > > > On Fri, Aug 5, 2022 at 3:27 AM Jose E. Marchesi via Gcc-patches > > wrote: > >> > >> > >> Hi people! > >> > >> First of all, a bit of context. > >> > >> It is common for C BPF programs to use variables that are implicitly set > >> by the underlying BPF machinery and not by the program itself. It is > >> also necessary for these variables to be stored in read-only storage so > >> the BPF verifier recognizes them as such. This leads to declarations > >> using both `const' and `volatile' qualifiers, like this: > >> > >> const volatile unsigned char is_allow_list = 0; > >> > >> Where `volatile' is used to avoid the compiler to optimize out the > >> variable, or turn it into a constant, and `const' to make sure it is > >> placed in .rodata. > >> > >> Now, it happens that: > >> > >> - GCC places `const volatile' objects in the .data section, under the > >> assumption that `volatile' somehow voids the `const'. > >> > >> - LLVM places `const volatile' objects in .rodata, under the > >> assumption that `volatile' is orthogonal to `const'. > >> > >> So there is a divergence, and this divergence has practical > >> consequences: it makes BPF programs compiled with GCC to not work > >> properly. > >> > >> When looking into this, I found this bugzilla: > >> > >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=25521 > >> "change semantics of const volatile variables" > >> > >> which was filed back in 2005. This report was already asking to put > >> `const volatile' objects in .rodata, questioning the current behavior. > >> > >> While discussing this in the #gcc IRC channel I was pointed out to the > >> following excerpt from the C18 spec: > >> > >> 6.7.3 Type qualifiers / 5 The properties associated with qualified > >> types are meaningful only for expressions that are > >> lval-values [note 135] > >> > >> > >> 135) The implementation may place a const object that is not > >> volatile in a read-only region of storage. Moreover, the > >> implementation need not allocate storage for such an object if > >> its $ address is never used. > >> > >> This footnote may be interpreted as if const objects that are volatile > >> shouldn't be put in read-only storage. Even if I was not very convinced > >> of that interpretation (see my earlier comment in BZ 25521) I filed the > >> following issue in the LLVM tracker in order to discuss the matter: > >> > >> https://github.com/llvm/llvm-project/issues/56468 > >> > >> As you can see, Aaron Ballman, one of the LLVM hackers, asked the WG14 > >> reflectors about this. He reported back that the reflectors consider > >> footnote 135 has not normative value. > >> > >> So, not having a normative mandate on either direction, there are two > >> options: > >> > >> a) To change GCC to place `const volatile' objects in .rodata instead > >> of .data. > >> > >> b) To change LLVM to place `const volatile' objects in .data instead > >> of .rodata. > >> > >> Considering that: > >> > >> - One target (bpf-unknown-none) breaks with the current GCC behavior. > >> > >> - No target/platform relies on the GCC behavior, that we know. (And it > >> is unlikely there is any, at least for targets also supported by > >> LLVM.) > >> > >> - Changing the LLVM behavior at this point would be very severely > >> traumatic for the BPF people and their users. > >> > >> I think the right thing to do is a). > >> Therefore this patch. > >> > >> A note about the patch itself: > >> > >> I am not that familiar with the middle-end and in this patch I am > >> assuming that a `var|constructor + SIDE_EFFECTS' is the result of > >> `volatile' (or an equivalent language construction) and nothing else. > >> It would be good if some middle-end wizard could confirm this. > > > > Yes, for decls that sounds correct. For a CTOR it just means > > re-evaluation is not safe. > > Thanks for confirming. > > >> Regtested in x86_64-linux-gnu and bpf-unknown-none. > >> No regressions observed. > > > > I think this warrants a testcase. > > Sure, will add one. > What would be the right testsuite? gcc.dg? That sounds good. > > I'm not sure I agree about the whole thing though, I'm leaving it > > to Joseph. > > > >> gcc/ChangeLog: > >> > >> PR middle-end/25521 > >> * varasm.cc (categorize_decl_for_section): Place `const volatile' > >> objects in read-only sections. > >> (default_select_section): Likewise. > >> --- > >> gcc/varasm.cc | 3 --- > >> 1 file changed, 3 deletions(-) > >> > >> diff --git a/gcc/varasm.cc b/gcc/varasm.cc > >> index 4db8506b106..7864db11faf 100644 > >> --- a/gcc/varasm.cc > >> +++ b/gcc/varasm.cc > >> @@ -6971,7 +6971,6 @@ default_select_section (tree decl, int reloc, > >> { > >> if (! ((flag_pic && reloc) > >> || !TREE_READONLY (decl) > >> - || TREE_SIDE_EFFECTS (decl) > >> || !TREE_CONSTANT (decl))) > >> return readonly_data_section; > >> } > >> @@ -7005,7 +7004,6 @@ categorize_decl_for_section (const_tree decl, int reloc) > >> if (bss_initializer_p (decl)) > >> ret = SECCAT_BSS; > >> else if (! TREE_READONLY (decl) > >> - || TREE_SIDE_EFFECTS (decl) > >> || (DECL_INITIAL (decl) > >> && ! TREE_CONSTANT (DECL_INITIAL (decl)))) > >> { > >> @@ -7046,7 +7044,6 @@ categorize_decl_for_section (const_tree decl, int reloc) > >> else if (TREE_CODE (decl) == CONSTRUCTOR) > >> { > >> if ((reloc & targetm.asm_out.reloc_rw_mask ()) > >> - || TREE_SIDE_EFFECTS (decl) > >> || ! TREE_CONSTANT (decl)) > >> ret = SECCAT_DATA; > >> else > >> -- > >> 2.30.2 > >>