[gcc r13-2952] place `const volatile' objects in read-only sections

[gcc r13-2952] place `const volatile' objects in read-only sections

[Jose E. Marchesi]

https://gcc.gnu.org/g:a0aafbc324aa90421f0ce99c6f5bbf64ed163da6

commit r13-2952-ga0aafbc324aa90421f0ce99c6f5bbf64ed163da6
Author: Jose E. Marchesi <jose.marchesi@oracle.com>
Date:   Thu Aug 4 21:16:10 2022 +0200

    place `const volatile' objects in read-only sections
    
    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.

Diff:
---
 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(-)

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 703aba412a6..2a2dd05db1e 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -477,6 +477,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 b0c4d6ae8ed..423f3f91af8 100644
--- a/gcc/varasm.cc
+++ b/gcc/varasm.cc
@@ -6967,7 +6967,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;
     }
@@ -7001,7 +7000,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))))
 	{
@@ -7042,7 +7040,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