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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman created this revision.
aaron.ballman added reviewers: nickdesaulniers, erichkeane, rjmccall, efriedma.
Herald added a project: All.
aaron.ballman requested review of this revision.
Herald added a project: clang.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131012

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGenCXX/const-global-linkage.cpp


Index: clang/test/CodeGenCXX/const-global-linkage.cpp
===================================================================
--- clang/test/CodeGenCXX/const-global-linkage.cpp
+++ clang/test/CodeGenCXX/const-global-linkage.cpp
@@ -4,7 +4,7 @@
 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; }
 
@@ -12,6 +12,6 @@
 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; }
Index: clang/lib/CodeGen/CodeGenModule.cpp
===================================================================
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -4165,6 +4165,15 @@
   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())
Index: clang/docs/ReleaseNotes.rst
===================================================================
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -52,6 +52,9 @@
 - 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>`_.
+- ``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
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D131012.449390.patch
Type: text/x-patch
Size: 2380 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220802/e0d25ca1/attachment.bin>