[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:14254
     int SectionFlags = ASTContext::PSF_Read;
-    if (var->getType().isConstQualified()) {
-      if (HasConstInit)
----------------
rnk wrote:
> rsmith wrote:
> > efriedma wrote:
> > > rnk wrote:
> > > > I think this is not compatible with MSVC. MSVC uses simple logic, it doesn't look for mutable: https://gcc.godbolt.org/z/sj6d4saxx
> > > > 
> > > > The const mutable struct appears in the myrdata section in that example.
> > > > 
> > > > I think the solution is to separate the flag logic from the pragma stack selection logic, which has to remain MSVC-compatible.
> > > MSVC apparently looks at whether the variable is marked "const", and nothing else; it doesn't look at mutable, it doesn't look at whether the variable has a constant initializer.  So the current code isn't right either; if we're trying to implement MSVC-compatible logic, we shouldn't check HasConstInit.
> > > 
> > > That said, I'm not sure how precisely/in what modes we want to precisely emulate MSVC.  Probably anything we do here will be confusing.
> > We should at least issue a warning if the sensible logic and the MSVC-compatible calculation differ. @rnk, do you know how important it is to follow the MSVC semantics in this regard?
> I think it depends on whether you think that users are primarily using these pragmas to override the default rdata/bss/data sections without any care about precisely what goes where, or if they are using them to do something finer grained.
> 
> If I had to guess, I'd say it's more likely the former, given that `__declspec(allocate)` and `#pragma(section)` exist to handle cases where users are putting specific globals into specific sections.
> 
> Which, if we follow Richard's suggestion, would mean warning when we put a global marked `const` into a writable section when `ConstSegStack` is non-empty. That seems reasonable. `-Wmicrosoft-const-seg` for the new warning group?
Does the MSVC situation only apply to custom sections? (presumably when not customizing the section, MSVC gets it right and can support a const global with a runtime initializer, mutable member, or mutating dtor?)

I think this code still needs to be modified, since this is the code that drives the error about incompatible sections. So it'll need to behave differently depending on the target platform?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156726