[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13896
+          Diag(var->getLocation(), diag::err_constexpr_var_requires_const_init)
+              << var << Init->getSourceRange();
+      }
----------------
efriedma wrote:
> rnk wrote:
> > zahiraam wrote:
> > > efriedma wrote:
> > > > zahiraam wrote:
> > > > > efriedma wrote:
> > > > > > zahiraam wrote:
> > > > > > > efriedma wrote:
> > > > > > > > I don't understand why this diagnostic is necessary.
> > > > > > > Now that the DLLImport variable can be a constant, HasConstInit is returning true (it was before returning false) and when the variable is global a diagnostic should be reported. 
> > > > > > > 
> > > > > > > This:
> > > > > > >   extern int _declspec(dllimport) val;
> > > > > > >   constexpr int& val_ref = val;
> > > > > > > 
> > > > > > > should report a diagnostic, but this:
> > > > > > > 
> > > > > > >   int foo() { 
> > > > > > >      extern int _declspec(dllimport) val; 
> > > > > > >      constexpr int& val_ref = val;
> > > > > > >   }
> > > > > > > 
> > > > > > >  shouldn't report a diagnostic.
> > > > > > MSVC doesn't report a diagnostic for either of your examples?
> > > > > So, these shouldn't fail anymore? 
> > > > > https://github.com/llvm/llvm-project/blob/main/clang/test/SemaCXX/PR19955.cpp#L5
> > > > > https://github.com/llvm/llvm-project/blob/main/clang/test/SemaCXX/PR19955.cpp#L8
> > > > > https://github.com/llvm/llvm-project/blob/main/clang/test/SemaCXX/dllimport-constexpr.cpp#L44
> > > > Right, they shouldn't fail.
> > > > 
> > > > My perspective is that since we're able to give those the expected semantics, we should just do that.  The only reason that "dllimport" is relevant at all is that there isn't a relocation for it, but our plan is to cover that up with a runtime constructor. So the user shouldn't need to know there's trickery behind the scenes unless they disassemble the binary.
> > > Got it! Thanks. Will remove the diagnostic here then. 
> > I worry that some of our users really do care quite a lot about constant initialization. I think emitting a special runtime initializer may be our best option (mingw does it through other means), but this is likely to be disruptive. Chrome has code which runs before CRT initialization to do some early sandboxing. ASan also competes to initialize shadow memory as early as possible (search compiler-rt for `.CRT`).
> > 
> > I worry that these users will think, "well, I've used [[constinit]] and constexpr, so this global should *definitely* be initialized during startup, and I won't have to worry about init order fiasco problems!", and then they will be surprised to find that this is not the case. Should we care? What can we do about this, realistically? Leaving behind this portability pothole of "dlimport symbols just aren't constexpr" seems worse. Our current behavior may be unduly influenced by my particular values: I think constant initialization is really important, I really care a lot about whether things are relocated or initialized at runtime.
> > 
> > I don't see any good solutions, so please carry on, but if you have any good ideas, please share. :)
> For the obscure cases where the user actually cares about that sort of thing, we can make -Wglobal-constructors trigger, I guess?  I'm not sure what else we could do that doesn't break the feature.
> 
> ------
> 
> It might be worth trying to reach out to Microsoft to figure out a common design for the runtime initialization thing.  MSVC appears to emit broken code in certain cases at the moment.  For example, in the following, "z" is correctly initialized at runtime, but "z2" emits a broken direct reference.
> 
> ```
> _declspec(dllimport) int x;
> constexpr int *y = &x;
> int *z = &x;
> int *z2 = y;
> ```
Thank you for the feedback.

I think these are really corner cases, right? Then maybe it's an acceptable solution to use the -Wglobal-constructors? I suggest we proceed with these changes?

@efriedma the one thing that's missing is the priority in which these constructors are initialized. You said in an earlier comment that they should have higher priority than normal global constructors. What priority should be given to them?  I see this PrioritizedCXXGlobalInits being filled in EmitCXXGlobalVarDeclInitFunc. Or should it be through AddGlobalCtor?  And what priority should be given to them? Should it be hard coded (horrible!)? Or calculated using OrderGlobalInitsOrStermFinalizers?


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

https://reviews.llvm.org/D137107