rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.
Thanks! My concerns are addressed, but please confirm that Eli's are too.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:14254
int SectionFlags = ASTContext::PSF_Read;
- if (var->getType().isConstQualified()) {
- if (HasConstInit)
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156726/new/
https://reviews.llvm.org/D156726
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
