ping
> [Changes from V1: > - Added a test.] > > It is common for C BPF programs to use variables that are implicitly > set by the BPF loader and run-time. 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, that 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, long ago. 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 personally 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 that the reflectors don't think > footnote 135 has any 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. > > - 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 at this point is a). > Therefore this patch. > > Regtested in x86_64-linux-gnu and bpf-unknown-none. > No regressions observed. > > 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/testsuite/ChangeLog: > > PR middle-end/25521 > * lib/target-supports.exp (check_effective_target_elf): Define. > * gcc.dg/pr25521.c: New test. > --- > gcc/testsuite/gcc.dg/pr25521.c | 10 ++++++++++ > gcc/testsuite/lib/target-supports.exp | 10 ++++++++++ > gcc/varasm.cc | 3 --- > 3 files changed, 20 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/pr25521.c > > diff --git a/gcc/testsuite/gcc.dg/pr25521.c b/gcc/testsuite/gcc.dg/pr25521.c > new file mode 100644 > index 00000000000..74fe2ae6626 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/pr25521.c > @@ -0,0 +1,10 @@ > +/* PR middle-end/25521 - place `const volatile' objects in read-only > + sections. > + > + { dg-require-effective-target elf } > + { dg-do compile } */ > + > +const volatile int foo = 30; > + > + > +/* { dg-final { scan-assembler "\\.rodata" } } */ > diff --git a/gcc/testsuite/lib/target-supports.exp > b/gcc/testsuite/lib/target-supports.exp > index 04a2a8e8659..c663d59264b 100644 > --- a/gcc/testsuite/lib/target-supports.exp > +++ b/gcc/testsuite/lib/target-supports.exp > @@ -483,6 +483,16 @@ proc check_effective_target_alias { } { > } > } > > +# Returns 1 if the target uses the ELF object format, 0 otherwise. > + > +proc check_effective_target_elf { } { > + if { [gcc_target_object_format] == "elf" } { > + return 1; > + } else { > + return 0; > + } > +} > + > # Returns 1 if the target toolchain supports ifunc, 0 otherwise. > > proc check_ifunc_available { } { > 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
