[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:
> efriedma wrote:
> > dblaikie wrote:
> > > efriedma wrote:
> > > > dblaikie wrote:
> > > > > 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?
> > > > Yes, the MSVC situation is specifically if you specify `#pragma const_seg`; without the pragma, it does what you'd expect.
> > > Went with the "let's do the thing that the user probably wants, but isn't what MSVC does, and warn when that difference comes up" - if that's OK with everyone.
> > > 
> > > (always open to wordsmithing the warning - and if we want to, can go to the extra layer and specifically diagnose which reason (mutable members, non-const init) - and I can't quite figure out the best phrasing to say "we're putting it in section X insetad of section Y, because Z, but Microsoft would use X because A" or something... it's all a bit of a mouthful)
> > Describing which reason actually applies would make the warning a lot easier to read.
> That is true, but I think very few people will see this diagnostic. I'm not sure it's worth the added code complexity to implement that improvement.
Updated with a specific diagnosis. Phrasing still feels a bit awkward/I'm open to wordsmithing suggestions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156726