On Fri, Aug 5, 2022 at 10:30 AM Jose E. Marchesi
<jose.march...@oracle.com> wrote:
>
>
> Hi Richard.
>
> > On Fri, Aug 5, 2022 at 3:27 AM Jose E. Marchesi via Gcc-patches
> > <gcc-patches@gcc.gnu.org> 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
> >>
