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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:14254
     int SectionFlags = ASTContext::PSF_Read;
-    if (var->getType().isConstQualified()) {
-      if (HasConstInit)
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156726