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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 449405.
aaron.ballman added a comment.

Rebased to get precommit CI to look at it.


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

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
@@ -54,6 +54,9 @@
   `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
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^



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
@@ -54,6 +54,9 @@
   `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
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits