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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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


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

https://reviews.llvm.org/D131012

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

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

[Andrii Nakryiko via Phabricator via cfe-commits]

anakryiko added a comment.

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.


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

https://reviews.llvm.org/D131012

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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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?


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

https://reviews.llvm.org/D131012

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

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

[Andrii Nakryiko via Phabricator via cfe-commits]

anakryiko added a comment.

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


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

https://reviews.llvm.org/D131012

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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman planned changes to this revision.
aaron.ballman added a comment.

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.


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

https://reviews.llvm.org/D131012

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

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

[Andrii Nakryiko via Phabricator via cfe-commits]

anakryiko added a comment.

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.


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

https://reviews.llvm.org/D131012

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

[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 `_.
 - Fix `#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 `_.
 
 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 `_.
 - Fix `#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 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131012

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

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers accepted this revision.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131012

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

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131012

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

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131012

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

[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 `_.
+- ``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 `_.
 
 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
+++