[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/CodeGen/CodeGenModule.cpp:4852
+          if (!CD->isTrivial() && !D->getTLSKind())
+            NeedsGlobalCtor = true;
+        }
----------------
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?


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

https://reviews.llvm.org/D137107