From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x532.google.com (mail-ed1-x532.google.com [IPv6:2a00:1450:4864:20::532]) by sourceware.org (Postfix) with ESMTPS id 7D0E73858D1E for ; Fri, 5 Aug 2022 06:52:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7D0E73858D1E Received: by mail-ed1-x532.google.com with SMTP id z22so2212682edd.6 for ; Thu, 04 Aug 2022 23:52:48 -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=5HQFG6b4hRkr5MHIjXxSOoFabL4JWOMfEEDbpuGX8ME=; b=l95+EVZ5o4EkN6lYo4VaGhPSQHL0BJ+AXeeEObJV9+4lfpArJhfXF3bLGshIga+25+ Ad5qh3/gd/DxaGbb1Nw7fNIHaBxglbstYYLuRmthigClA47F2gJwU7EwRhDXSJYnn+eN gbVFaRf/unA6vIx/JDssgpXhV2r3YRor8eboYLSCw7BugErNZmL8oM2EN79Rpq8HOysX +ToxIDK5k9F/pBFFOrZTdTci3Th7kMMS/WlHRr5nYXECdxg50NbzKnqhWpEiyEQkpuJ6 bhhhJ9lK/j1qEFJIRQNT5+iV1vw3nuW9bNKed1Ow01SR7I4X1Z1yW0CdcnPwDGM+hhuo tweg== X-Gm-Message-State: ACgBeo0yh9lNMXpzto1NC5CWUWwC6qP/NogK5XoAoWp80putZTwEJLEZ 60RSyVfJHl0/7yxVvQViK6/jx66UBnLH6w57Yjc= X-Google-Smtp-Source: AA6agR5VjT1AyMoyC1K/Ci8jIepRV/4xot6J0lOPAFiv/HY92i1mh756KslmARH0M29quhNrHYFoMzGwVWEy2GgLMmI= X-Received: by 2002:aa7:db85:0:b0:43d:609b:2592 with SMTP id u5-20020aa7db85000000b0043d609b2592mr5226694edt.147.1659682366092; Thu, 04 Aug 2022 23:52:46 -0700 (PDT) MIME-Version: 1.0 References: <87sfmbxyyj.fsf@oracle.com> In-Reply-To: <87sfmbxyyj.fsf@oracle.com> From: Richard Biener Date: Fri, 5 Aug 2022 08:52:33 +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.1 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 06:52:50 -0000 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. > Regtested in x86_64-linux-gnu and bpf-unknown-none. > No regressions observed. I think this warrants a testcase. 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 >