public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: "Koning, Paul" <Paul.Koning@dell.com>
To: Jeff Law <jeffreyalaw@gmail.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH V2] place `const volatile' objects in read-only sections
Date: Wed, 28 Sep 2022 13:33:04 +0000	[thread overview]
Message-ID: <66F12817-6A11-4B33-A909-7F1230DCBEA2@dell.com> (raw)
In-Reply-To: <3b34b862-c71b-307c-f884-04fad0a3751b@gmail.com>



> On Sep 27, 2022, at 8:51 PM, Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
> 
> On 8/5/22 05:41, Jose E. Marchesi via Gcc-patches wrote:
>> [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'.
>> ...
> 
> The best use I've heard for const volatile is stuff like hardware status registers which are readonly from the standpoint of the compiler, but which are changed by the hardware.   But for those, we're looking for the const to trigger compiler diagnostics if we try to write the value.  The volatile (of course) indicates the value changes behind our back.

I'd go a bit further and say that this is the only use of const volatile that makes any sense.

> What you're trying to do seems to parallel that case reasonably well for the volatile aspect.  You want to force the compiler to read the data for every access.
> 
> Your need for the const is a bit different.  Instead of looking to get a diagnostic out of the compiler if its modified, you need the data to live in .rodata so the BPF verifier knows the compiler/code won't change the value.  Presumably the BPF verifier can't read debug info to determine the const-ness.
> 
> I'm not keen on the behavior change, but nobody else is stepping in to review and I don't have a strong case to reject.  So OK for the trunk.

A const volatile that sits in memory feels like a programmer error.  Instead of worrying about how it's handled, would it not make more sense to tag it with a warning?

	paul


  reply	other threads:[~2022-09-28 13:33 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
2022-09-28  0:51 ` Jeff Law
2022-09-28 13:33   ` Koning, Paul [this message]
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=66F12817-6A11-4B33-A909-7F1230DCBEA2@dell.com \
    --to=paul.koning@dell.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jeffreyalaw@gmail.com \
    /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).