From: "Jose E. Marchesi" <jose.marchesi@oracle.com>
To: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH V2] place `const volatile' objects in read-only sections
Date: Thu, 18 Aug 2022 15:02:22 +0200 [thread overview]
Message-ID: <875yipd7s1.fsf@oracle.com> (raw)
In-Reply-To: <87zggiudcr.fsf@oracle.com> (Jose E. Marchesi's message of "Fri, 05 Aug 2022 13:41:24 +0200")
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
next prev parent reply other threads:[~2022-08-18 13:02 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-05 11:41 Jose E. Marchesi
2022-08-18 13:02 ` Jose E. Marchesi [this message]
2022-09-28 0:51 ` Jeff Law
2022-09-28 13:33 ` Koning, Paul
2022-09-29 11:25 ` Jose E. Marchesi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=875yipd7s1.fsf@oracle.com \
--to=jose.marchesi@oracle.com \
--cc=gcc-patches@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).