This is an archive of the discontinued LLVM Phabricator instance.

No longer place const volatile global variables in a read only section
Changes PlannedPublic

Authored by aaron.ballman on Aug 2 2022, 12:47 PM.

Details

Summary

The C standard hints in a footnote attached to C17 6.7.3p5 that const objects which are not volatile can be placed in read-only memory, which implies that volatile objects can never be placed there. GCC appears to be following that behavior: https://godbolt.org/z/9WEq18TWz and this changes Clang to behave the same.

Fixes #56468

Diff Detail

Event Timeline

aaron.ballman created this revision.Aug 2 2022, 12:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2022, 12:47 PM
aaron.ballman requested review of this revision.Aug 2 2022, 12:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2022, 12:47 PM

The code changes look perfectly fine to me, but I'm hopeful someone else has something to say about how acceptable/ABI related this is.

I think this is fine for the ABI; the section is generally a definition-specific property and doesn't affect use sites.

Do we also need to check for volatile fields of records?

nickdesaulniers accepted this revision.Aug 2 2022, 1:22 PM

Thanks for the patch. I've asked some of my colleagues who work on libabigail their thoughts on the implications of this change. Due to timezones, it may take some time to hear back. Mind holding this for 24hr and I'll update this with the feedback, if any?

This revision is now accepted and ready to land.Aug 2 2022, 1:22 PM

I think this is fine for the ABI; the section is generally a definition-specific property and doesn't affect use sites.

Do we also need to check for volatile fields of records?

GCC doesn't seem to do anything special there as best I can tell: https://godbolt.org/z/67eovqoG8

Thanks for the patch. I've asked some of my colleagues who work on libabigail their thoughts on the implications of this change. Due to timezones, it may take some time to hear back. Mind holding this for 24hr and I'll update this with the feedback, if any?

Happy to wait!

Rebased to get precommit CI to look at it.

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.

aaron.ballman planned changes to this revision.Aug 3 2022, 5:34 AM

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.

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.

anakryiko added a comment.EditedAug 3 2022, 9:39 AM

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.

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.

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!

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?

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.

On the one hand, we can certainly just have different behavior on BPF targets if this is a common expectation there.

On the other hand, if there isn't a requirement to put const volatile objects in writable memory, then I'd rather not. If someone has specific expectations about the section a symbol ends up in that don't match the standard interpretation in the language, they really ought to enforce their expectations with a section attribute. The use case of volatile memory-mapped I/O needs something like that anyway (or something even worse).