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

Eli Friedman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Dec 20 11:48:02 PST 2022


efriedma added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:254
 
+def warn_for_global_ctor_for_dllimport : Warning<
+  "global constructor is generated for dllimport global var %0">,
----------------
Missing code to emit this warning?


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5006
+    if (NeedsGlobalCtor || NeedsGlobalDtor)
+      DelayedCXXInitPosition[D] = ~0U;
+  } else {
----------------
zahiraam wrote:
> Do you agree this should be done only when one of those flags is on?
Yes, that's fine; I wasn't really paying close attention to the exact code.  Just wanted to make the point about the structure of the if statements, and code was the easiest way to explain it.

Maybe the outer if statement should actually be `if (isStaticInit(D, getLangOpts()) && NeedsGlobalCtor) {`.

On a related note, we might want to avoid the name "ctor", in case that accidentally conflicts with some user code; an "__"-prefixed name would be appropriate.


================
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:
> > 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);
> > }
> If you agree with the generated IR for this case, then I will start editing the LIT tests accordingly.
> 
>   // CHECK: @"?b@@3UB@@A" = dso_local global %struct.B undef
>   // CHECK: @llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @dtor, ptr null }]
> 
>   // CHECK: define internal void @dtor()
>   // CHECK: entry:
>   // CHECK:     %0 = call i32 @atexit(ptr @"??__Fb@@YAXXZ")
>   // CHECK:   ret void
> 
>   // CHECk: define linkonce_odr dso_local x86_thiscallcc void @"??1B@@QAE at XZ"
>   // CHECK: entry:
>   // CHECK:  %this.addr = alloca ptr, align 4
>   // CHECK:  store ptr %this, ptr %this.addr, align 4
>   // CHECK:  %this1 = load ptr, ptr %this.addr, align 4
>   // CHECK:  ret void
> 
> 
>   // CHECK: define internal void @"??__Fb@@YAXXZ"() #0 {
>   // CHECK: entry:
>   // CHECK:   call x86_thiscallcc void @"??1B@@QAE at XZ"(ptr @"?b@@3UB@@A")
>   // CHECK:   ret void
> 
> 
>   struct B {
>     constexpr B() {}
>     ~B() {};
>   };
>   constinit B b;
> 
> 
> The ctor with priority 201 is generated only when tryEmitForInitializer returns a nullptr (that's the only time when NeedsGlobalCtor is set to true). Correct?
Right, we only need the priority 201 ctor if tryEmitForInitializer fails, but we need constant initialization anyway to meet the C++ language requirements.

Updated code seems much cleaner.


================
Comment at: clang/test/CodeGenCXX/PR19955.cpp:16
+
+// CHECK: define internal void @ctor()
+// CHECK: entry
----------------
It's easier to debug FileCheck failures if you use CHECK-LABEL for function definitions.

You probably want to use a different prefix that's used for both 32-bit and 64-bit for lines like this.

CHECK-64 isn't used by any FileCheck invocation.


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

https://reviews.llvm.org/D137107



More information about the cfe-commits mailing list