Re: [PATCH] place `const volatile' objects in read-only sections

["Jose E. Marchesi" <jose.marchesi@oracle.com>]

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?

> 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
>>