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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits