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