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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4852
+          if (!CD->isTrivial() && !D->getTLSKind())
+            NeedsGlobalCtor = true;
+        }
----------------
zahiraam wrote:
> efriedma wrote:
> > zahiraam wrote:
> > > efriedma wrote:
> > > > I have no idea what this code is supposed to do.
> > > I have removed from this the thread_local variables for being processed the same way. Maybe they should be included, not sure?
> > > 
> > > For the rest is to differentiate between these cases. I found the 2nd test case while doing some unit testing.
> > > 
> > >   struct B {
> > >   constexpr B() {}
> > >   ~B() {};
> > >   };
> > >   constinit B b;
> > > 
> > > 
> > > and
> > > 
> > >   struct A {
> > >     int s;
> > >     ~A();
> > >   };
> > > 
> > >   struct D : A {};
> > >   D d1 = {};
> > > 
> > > I think the second test case is not supposed to EmitDeclInit in EmitCXXGlobalVarDeclInit right? But since now tryEmitForInitializer is returning an Initializer, then NeedsGlobalCtor = true; 
> > > 
> > > Actually, when I removed this code, I have 2 LIT tests failing with the same crash. 
> > > WDYT?
> > > 
> > > 
> > I'm not sure how thread-local var init works on Windows off the top of my head; I'd need to check.
> > 
> > For the given testcases, neither one of those cases should be calling EmitDeclInit; we use constant initialization for both cases, so the global init func should just be registering the destructors.  We shouldn't need to examine the initializer beyond checking whether tryEmitForInitializer succeeds.
> > 
> > Not sure what's crashing from your description.
> Removing this code will still result, in both cases to the call to EmitDeclInit, because the first call to EmitCXXCtorInit is called with PerformInit true. 
> 
> Calling EmitCXXCtorInit with false would result into a ctor function with an empty body for the 1st test case (struct B):
>   define internal void @ctor() #0 {
>   entry:
>     ret void
>   }
> 
> I would have thought that we should have something like this:
> 
>   define internal void @ctor() #0 {
>   entry:
>     %call = call x86_thiscallcc ptr @"??0B@@QAE at XZ"(ptr nonnull align 1 dereferenceable(1) @"?b@@3UB@@A")
>     ret void
>   }
> 
> No?
> because the first call to EmitCXXCtorInit is called with PerformInit true.

That sounds wrong; if tryEmitForInitializer succeeds, we shouldn't perform the init at all.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5009
+  if (isStaticInit(D, getLangOpts()) && NeedsGlobalCtor && NeedsGlobalDtor) {
+    EmitCXXCtorInit(D, GV, true, 201, llvm::StringLiteral("ctor"), false);
+    EmitCXXCtorInit(D, GV, false, 65535, llvm::StringLiteral("dtor"), true);
----------------
zahiraam wrote:
> efriedma wrote:
> > I think you want to use priority 201 whether or not there's a destructor.
> Is that what you mean?
I think it should look something more like this:

```
if (isStaticInit(D, getLangOpts()) {
  if (NeedsGlobalCtor)
    EmitCXXCtorInit(D, GV, true, 201, llvm::StringLiteral("ctor"), false);
  if (NeedsGlobalDtor)
    EmitCXXCtorInit(D, GV, false, 65535, llvm::StringLiteral("dtor"), true);
  DelayedCXXInitPosition[D] = ~0U;
} else {
  EmitCXXGlobalVarDeclInitFunc(D, GV, NeedsGlobalCtor);
}


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

https://reviews.llvm.org/D137107