[PATCH] D131012: No longer place const volatile global variables in a read only section

Wed Aug 3 10:45:23 PDT 2022

anakryiko added a comment.

In D131012#3697000 <https://reviews.llvm.org/D131012#3697000>, @aaron.ballman wrote:

> In D131012#3696810 <https://reviews.llvm.org/D131012#3696810>, @anakryiko wrote:
>
>> In D131012#3696327 <https://reviews.llvm.org/D131012#3696327>, @aaron.ballman wrote:
>>
>>> In D131012#3695110 <https://reviews.llvm.org/D131012#3695110>, @anakryiko wrote:
>>>
>>>> This will severly break BPF users, as we have a heavy reliance on `const volatile` variables being allocated to `.rodata`, which are passed to BPF verifier in Linux kernel as read-only values. This is critical for BPF Complie Once - Run Everywhere approach of writing portable BPF applications. We've been relying on this behavior for years now and changes this will break many existing BPF applications.
>>>
>>> Thank you for this feedback! I guess I'm a bit surprised given the contents from the issue seem to imply that BPF needed Clang's behavior to change: `Note that this is causing practical difficulties: the kernel's bpftool is generating different skeleton headers for BPF code compiled from LLVM and GCC, because the names of the containing sections get reflected.`
>>
>> GCC folks are trying to make their BPF backend usable. But instead of working with community to make sure they do things the same way that Clang does (which for years now is de facto standard for compiling BPF code and we rely on this behavior heavily in libbpf and other BPF loader libraries), they instead work against BPF community and try to modify/adjust/break the world around them, instead of working with us to make GCC-BPF compatible with Clang.
>
> Ah, that's background information I was unaware of. Thank you for sharing that perspective!

And thank you for holding of on this one and trying to clarify this with WG14!

>>> That said, I'm asking on the WG14 reflectors whether there's a normative requirement here or not, so I'm marking this as having planned changes until I hear back from WG14 and until we've resolved whether the changes will fix code vs break code (or both!) so we don't accidentally land this.
>>
>> Thanks! But note that `const volatile` being in `.rodata` is a very explicit expectation in BPF world, so changing that to `.data` will immediately break a bunch of different BPF applications that rely on this for BPF CO-RE (Compile Once - Run Everywhere) for guarding BPF code that shouldn't work on old kernels. .rodata is reported to BPF verifier as fixed, read-only, known values and BPF verifier is using those values for control flow analysis and dead code elimination. This is crucial feature.
>
> Yes, but if the standard has a normative requirement in this area, we'd still have to consider how to move forward in a conforming C mode (I'd guess the most clear path would be a compiler flag to get the old behavior back again for those who need it). But that said...
>
> The responses I've been getting back on the WG14 reflectors are not indicating that there's any normative requirement. It sounds like the sentiment is generally that the footnote is trying to tell you that objects which are `const` qualified but not `volatile` qualified can be assumed to not change value. Based on that sentiment from the committee, we're under no obligation to make a change here for conformance to C.
>
> Where we go from here regarding https://github.com/llvm/llvm-project/issues/56468 is something we still need to work out. From what I'm hearing, it sounds like changing Clang will break a bunch of currently working, valid code. We obviously want to avoid that. Does anyone have visibility into the GCC community's thoughts and efforts in this space? Like, are they in the same boat where changing their behavior will break a bunch of currently working, valid code (outside of BPF) as well?

I don't know how hard it is, but at least for GCC's BPF backend they'd need to emit `const volatile` into .rodata to be BPF CO-RE-compatible. Let's continue discussion on Github issue, James and Jose both seem to be involved in this from GCC side, so you already have relevant people there.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131012/new/

https://reviews.llvm.org/D131012