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

[aaron.ballman]

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

[erichkeane]

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.

[rjmccall]

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]

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?

[aaron.ballman]

In D131012#3694502, @rjmccall wrote:

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

In D131012#3694588, @nickdesaulniers wrote:

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!

[aaron.ballman]

Rebased to get precommit CI to look at it.

[anakryiko]

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]

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

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]

In D131012#3696327, @aaron.ballman wrote:

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

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.

[aaron.ballman]

In D131012#3696810, @anakryiko wrote:

In D131012#3696327, @aaron.ballman wrote:

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

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?

[anakryiko]

In D131012#3697000, @aaron.ballman wrote:

In D131012#3696810, @anakryiko wrote:

In D131012#3696327, @aaron.ballman wrote:

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

[rjmccall]

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

clang/docs/ReleaseNotes.rst

 
 Bug Fixes
 ---------
 - Fixes an accepts-invalid bug in C when using a ``_Noreturn`` function
   specifier on something other than a function declaration. This fixes
   `Issue 56800 <https://github.com/llvm/llvm-project/issues/56800>`_.
 - Fix `#56772 <https://github.com/llvm/llvm-project/issues/56772>`_ - invalid
   destructor names were incorrectly accepted on template classes.
+- ``const volatile`` global variables are no longer placed in a read-only
+  section, but are instead treated as a non-const global. This fixes
+  `Issue 56468 <https://github.com/llvm/llvm-project/issues/56468>`_.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Clang will now correctly diagnose as ill-formed a constant expression where an
   enum without a fixed underlying type is set to a value outside the range of
   the enumeration's values. Fixes
   `Issue 50055: <https://github.com/llvm/llvm-project/issues/50055>`_.
 

clang/lib/CodeGen/CodeGenModule.cpp

 ///
 /// If ExcludeCtor is true, the duration when the object's constructor runs
 /// will not be considered. The caller will need to verify that the object is
 /// not written to during its construction.
 bool CodeGenModule::isTypeConstant(QualType Ty, bool ExcludeCtor) {
   if (!Ty.isConstant(Context) && !Ty->isReferenceType())
     return false;
 
+  // If the type is also marked as volatile, it should not be treated as a
+  // constant according to C17 6.7.3p5 and footnote 133, though there does not
+  // appear to be a normative requirement. That said, GCC does not put the
+  // object into a read only section, and we're following suit. C++ says even
+  // less about this situation, but [decl.type.cv]p6 implies that when it comes
+  // to volatile, C++ follows C's lead, so we'll do the same here.
+  if (Ty.isVolatileQualified())
+    return false;
+
   if (Context.getLangOpts().CPlusPlus) {
     if (const CXXRecordDecl *Record
           = Context.getBaseElementType(Ty)->getAsCXXRecordDecl())
       return ExcludeCtor && !Record->hasMutableFields() &&
              Record->hasTrivialDestructor();
   }
 
   return true;

clang/test/CodeGenCXX/const-global-linkage.cpp

 // RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm -o - %s | FileCheck %s
 
 const int x = 10;
 const int y = 20;
 const volatile int z = 30;
 // CHECK-NOT: @x
-// CHECK: @z = {{(dso_local )?}}constant i32 30
+// CHECK: @z = {{(dso_local )?}}global i32 30
 // CHECK: @_ZL1y = internal constant i32 20
 const int& b() { return y; }
 
 const char z1[] = "asdf";
 const char z2[] = "zxcv";
 const volatile char z3[] = "zxcv";
 // CHECK-NOT: @z1
-// CHECK: @z3 = {{(dso_local )?}}constant
+// CHECK: @z3 = {{(dso_local )?}}global
 // CHECK: @_ZL2z2 = internal constant
 const char* b2() { return z2; }