Emit const globals with constexpr destructor as constant LLVM values

[hans]

Summary

This follows 2b4fa53 which made Clang not emit destructor calls for such objects. However, they would still not get emitted as constants since CodeGenModule::isTypeConstant() only checked whether the destructor was trivial, not if it's constexpr.

Fixes Issue #61212

[hans]

+aaron.ballman

[ilya-biryukov]

I don't have a lot of experience in codegen, so will let Aaron and Richard do the review.

However, still wanted to share one observation. The actual check that avoids emitting the destructors for variables seems more involved than just checking if the destructor is constexpr.
Is it safe to rely solely on the type for codegen? E.g. I would have expected checks that look at HasConstantDestruction for variables with non-trivial constructors.

[hans]

In D145369#4181296, @ilya-biryukov wrote:

I don't have a lot of experience in codegen, so will let Aaron and Richard do the review.

Thanks for checking! I don't have a lot of experience here either, so any review is much appreciated.

However, still wanted to share one observation. The actual check that avoids emitting the destructors for variables seems more involved than just checking if the destructor is constexpr.
Is it safe to rely solely on the type for codegen? E.g. I would have expected checks that look at HasConstantDestruction for variables with non-trivial constructors.

Right, codegen is also factoring in the check about emitting destructors when deciding about marking the LLVM value constant or not:

in CodeGenModule::EmitGlobalVarDefinition()

  bool NeedsGlobalDtor =
      !IsDefinitionAvailableExternally &&
      D->needsDestruction(getContext()) == QualType::DK_cxx_destructor;

...

  // If it is safe to mark the global 'constant', do so now.
  GV->setConstant(!NeedsGlobalCtor && !NeedsGlobalDtor &&
                  isTypeConstant(D->getType(), true));

So I *think* this is correct, and that the hasConstexprDestructor() case was just missing from isTypeConstant() because constexpr destructors were added later. But again, not super familiar with this stuff :)

[aaron.ballman]

This seems reasonable to me, but adding the codegen code owners in case they disagree.

[efriedma]

There appear to be 8 different places that call isTypeConstant(Ty, true); I'd like to see testcases for all of them if it's relevant. (I agree with your assessment for global variables, but not sure about the other places.)

[hans]

Thanks for the review!

Added tests to cover other calls that change behavior due to this.

Please take another look.

[efriedma]

clang/test/CodeGenCXX/static-init.cpp

181

I don't see how this works. For a static local variable, in general, we register the destructor with __cxa_atexit, and the destructor can have arbitrary side-effects. For example:

extern void foo();
struct A {
  constexpr A() : x(1) {}
  constexpr ~A() {if (x) foo();}
  int x;
};
const int *f() {
  static const A a{};
  return &a.x;
}

[efriedma]

clang/lib/CodeGen/CodeGenModule.cpp

4344

For the purposes of CodeGen, checking Record->hasConstexprDestructor() isn't really helpful. Even if Record->hasConstexprDestructor() is true, a destructor can still have side-effects. Since the callers need to handle that possibility anyway, you might as well just skip the Record->hasTrivialDestructor() || Record->hasConstexprDestructor() check.

[hans]

clang/lib/CodeGen/CodeGenModule.cpp

4344

Maybe what we want is to also have an ExcludeDtor param that callers which have checked that no dtor call is needed could set?

clang/test/CodeGenCXX/static-init.cpp

181

Hmm, I guess I assumed the destructor being constexpr meant it wouldn't have side effects, which of course isn't necessarily true.. thanks for the example.

[hans]

Passing a ExcludeDtor param.

[efriedma]

LGTM

clang/lib/CodeGen/CGExprAgg.cpp

535 ↗

(On Diff #505466)

I'm not sure why we're querying isTypeConstant here? The global shouldn't get written to in any case. But I guess that's a separate issue.

[hans]

clang/lib/CodeGen/CGExprAgg.cpp

535 ↗

(On Diff #505466)

Yeah, it seems safe to always make the global const. Doing that here: https://reviews.llvm.org/D146211

clang/lib/CodeGen/CodeGenModule.cpp

 bool CodeGenModule::isTypeConstant(QualType Ty, bool ExcludeCtor) {
   if (!Ty.isConstant(Context) && !Ty->isReferenceType())
     return false;
 
   if (Context.getLangOpts().CPlusPlus) {
     if (const CXXRecordDecl *Record
           = Context.getBaseElementType(Ty)->getAsCXXRecordDecl())
       return ExcludeCtor && !Record->hasMutableFields() &&
-             Record->hasTrivialDestructor();
+             (Record->hasTrivialDestructor() ||
+              Record->hasConstexprDestructor());
   }
 
   return true;
 }
 
 /// GetOrCreateLLVMGlobal - If the specified mangled name is not in the module,
 /// create and return an llvm GlobalVariable with the specified type and address
 /// space. If there is something in the module with the specified name, return

clang/test/CodeGenCXX/const-init-cxx11.cpp

 // RUN: %clang_cc1 -no-opaque-pointers -w -fmerge-all-constants -triple x86_64-elf-gnu -emit-llvm -o - %s -std=c++11 | FileCheck %s
+// RUN: %clang_cc1 -no-opaque-pointers -w -fmerge-all-constants -triple x86_64-elf-gnu -emit-llvm -o - %s -std=c++20 | FileCheck -check-prefix=CHECK20 %s
 
 // FIXME: The padding in all these objects should be zero-initialized.
 namespace StructUnion {
   struct A {
     int n;
     double d;
     union U {
       constexpr U(int x) : x(x) {}

 // CHECK: @_ZZN12LocalVarInit3aggEvE1a = internal constant {{.*}} i32 101
 // CHECK: @_ZZN12LocalVarInit4ctorEvE1a = internal constant {{.*}} i32 102
 // CHECK: @__const._ZN12LocalVarInit8mutable_Ev.a = private unnamed_addr constant {{.*}} i32 103
 // CHECK: @_ZGRN33ClassTemplateWithStaticDataMember1SIvE1aE_ = linkonce_odr constant i32 5, comdat
 // CHECK: @_ZN33ClassTemplateWithStaticDataMember3useE ={{.*}} constant i32* @_ZGRN33ClassTemplateWithStaticDataMember1SIvE1aE_
 // CHECK: @_ZGRN39ClassTemplateWithHiddenStaticDataMember1SIvE1aE_ = linkonce_odr hidden constant i32 5, comdat
 // CHECK: @_ZN39ClassTemplateWithHiddenStaticDataMember3useE ={{.*}} constant i32* @_ZGRN39ClassTemplateWithHiddenStaticDataMember1SIvE1aE_
 // CHECK: @_ZGRZN20InlineStaticConstRef3funEvE1i_ = linkonce_odr constant i32 10, comdat
+// CHECK20: @_ZZN12LocalVarInit4dtorEvE1a = internal constant {{.*}} i32 103
 
 // Constant initialization tests go before this point,
 // dynamic initialization tests go after.
 
 // We must emit a constant initializer for NonLiteralConstexpr::ntd, but also
 // emit an initializer to register its destructor.
 // CHECK: define {{.*}}cxx_global_var_init{{.*}}
 // CHECK-NOT: NonLiteralConstexpr
namespace LocalVarInit {
   // CHECK: define {{.*}} @_ZN12LocalVarInit8mutable_Ev
   // CHECK-NOT: call
   // CHECK: call {{.*}}memcpy{{.*}} @__const._ZN12LocalVarInit8mutable_Ev.a
   // CHECK-NOT: call
   // Can't fold return value due to 'mutable'.
   // CHECK-NOT: ret i32 103
   // CHECK: }
   int mutable_() { constexpr Mutable a = { f(103) }; return a.k; }
+
+#if __cplusplus >= 202002L
+  // CHECK20: define {{.*}} @_ZN12LocalVarInit4dtorEv
+  // CHECK20-NOT: call
+  // CHECK20: ret i32 103
+  struct Dtor { constexpr Dtor(int n) : k(n) {} constexpr ~Dtor() {} int k; };
+  int dtor() { constexpr Dtor a = { f(103) }; return a.k; }
+#endif
 }
 
 namespace CrossFuncLabelDiff {
   // Make sure we refuse to constant-fold the variable b.
   constexpr long a(bool x) { return x ? 0 : (long)&&lbl + (0 && ({lbl: 0;})); }
   void test() { static long b = (long)&&lbl - a(false); lbl: return; }
   // CHECK: sub nsw i64 ptrtoint (i8* blockaddress(@_ZN18CrossFuncLabelDiff4testEv, {{.*}}) to i64),
   // CHECK: store i64 {{.*}}, i64* @_ZZN18CrossFuncLabelDiff4testEvE1b, align 8

clang/test/CodeGenCXX/const-init-cxx2a.cpp

 // RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s -std=c++2a | FileCheck %s --implicit-check-not=cxx_global_var_init --implicit-check-not=cxa_atexit
 
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-pch -o %t.pch %s -std=c++2a
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -include-pch %t.pch -x c++ /dev/null -emit-llvm -o - -std=c++2a | FileCheck %s --implicit-check-not=cxx_global_var_init --implicit-check-not=cxa_atexit
 
 // CHECK: @a ={{.*}} global i32 123,
 int a = (delete new int, 123);
 
 struct B {
   constexpr B() {}
   constexpr ~B() { n *= 5; }
   int n = 123;
 };
-// CHECK: @b ={{.*}} global {{.*}} i32 123
+// CHECK: @b ={{.*}} constant {{.*}} i32 123
 extern constexpr B b = B();
 
-// CHECK: @_ZL1c = internal global {{.*}} i32 123
+// CHECK: @_ZL1c = internal constant {{.*}} i32 123
 const B c;
 int use_c() { return c.n; }
 
 struct D {
   int n;
   constexpr ~D() {}
 };
 D d;

clang/test/CodeGenCXX/init-invariant.cpp

-// RUN: %clang_cc1 -triple i686-linux-gnu -emit-llvm %s -disable-llvm-passes -o - | FileCheck %s --check-prefix=CHECK-O0
-// RUN: %clang_cc1 -triple i686-linux-gnu -emit-llvm %s -O1 -disable-llvm-passes -o - | FileCheck %s
+// RUN: %clang_cc1 -triple i686-linux-gnu -std=c++20 -emit-llvm %s -disable-llvm-passes -o - | FileCheck %s --check-prefix=CHECK-O0
+// RUN: %clang_cc1 -triple i686-linux-gnu -std=c++20 -emit-llvm %s -O1 -disable-llvm-passes -o - | FileCheck %s
 
 // Check that we add an llvm.invariant.start.p0i8 to mark when a global becomes
 // read-only. If globalopt can fold the initializer, it will then mark the
 // variable as constant.
 
 // Do not produce markers at -O0.
 // CHECK-O0-NOT: llvm.invariant.start.p0i8
 
 struct A {
   A();
   int n;
 };
 
 // CHECK: @a ={{.*}} global {{.*}} zeroinitializer
 extern const A a = A();
 
+struct A2 {
+  A2();
+  constexpr ~A2() {}
+  int n;
+};
+
+// CHECK: @a2 ={{.*}} global {{.*}} zeroinitializer
+extern const A2 a2 = A2();
+
+
 struct B {
   B();
   mutable int n;
 };
 
 // CHECK: @b ={{.*}} global {{.*}} zeroinitializer
 extern const B b = B();
 

 
 void e() {
   static const A a = A();
 }
 
 // CHECK: call void @_ZN1AC1Ev(ptr noundef {{[^,]*}} @a)
 // CHECK: call {{.*}}@llvm.invariant.start.p0(i64 4, ptr @a)
 
+// CHECK: call void @_ZN2A2C1Ev(ptr noundef {{[^,]*}} @a2)
+// CHECK: call {{.*}}@llvm.invariant.start.p0(i64 4, ptr @a2)
+
 // CHECK: call void @_ZN1BC1Ev(ptr noundef {{[^,]*}} @b)
 // CHECK-NOT: call {{.*}}@llvm.invariant.start.p0(i64 noundef 4, ptr @b)
 
 // CHECK: call void @_ZN1CC1Ev(ptr noundef {{[^,]*}} @c)
 // CHECK-NOT: call {{.*}}@llvm.invariant.start.p0(i64 noundef 4, ptr @c)
 
 // CHECK: call noundef i32 @_Z1fv(
 // CHECK: store {{.*}}, ptr @d
 // CHECK: call {{.*}}@llvm.invariant.start.p0(i64 4, ptr @d)
 
 // CHECK-LABEL: define{{.*}} void @_Z1ev(
 // CHECK: call void @_ZN1AC1Ev(ptr noundef {{[^,]*}} @_ZZ1evE1a)
 // CHECK: call {{.*}}@llvm.invariant.start.p0(i64 4, ptr {{.*}}@_ZZ1evE1a)
 // CHECK-NOT: llvm.invariant.end

clang/test/CodeGenCXX/static-init.cpp

 // RUN: %clang_cc1 %s -triple=x86_64-pc-linuxs -emit-llvm -std=c++98 -o - | FileCheck -check-prefix=CHECK -check-prefix=CHECK98 %s
 // RUN: %clang_cc1 %s -triple=x86_64-pc-linuxs -emit-llvm -std=c++11 -o - | FileCheck -check-prefix=CHECK -check-prefix=CHECK11 %s
+// RUN: %clang_cc1 %s -triple=x86_64-pc-linuxs -emit-llvm -std=c++20 -o - | FileCheck -check-prefix=CHECK -check-prefix=CHECK20 %s
 
 // CHECK: @_ZZ1hvE1i = internal global i32 0, align 4
 // CHECK: @base_req ={{.*}} global [4 x i8] c"foo\00", align 1
 // CHECK: @base_req_uchar ={{.*}} global [4 x i8] c"bar\00", align 1
 
 // CHECK: @_ZZN5test31BC1EvE1u = internal global { i8, [3 x i8] } { i8 97, [3 x i8] undef }, align 4
 
+// CHECK20: @_ZZN5test51fEvE1a = internal constant %"struct.test5::A" { i32 42 }
+
 // CHECK: @_ZZ2h2vE1i = linkonce_odr global i32 0, comdat, align 4
 // CHECK: @_ZGVZ2h2vE1i = linkonce_odr global i64 0, comdat, align 8{{$}}
 // CHECK: @_ZZN5test1L6getvarEiE3var = internal constant [4 x i32] [i32 1, i32 0, i32 2, i32 4], align 16
 // CHECK98: @_ZZN5test414useStaticLocalEvE3obj = linkonce_odr global %"struct.test4::HasVTable" zeroinitializer, comdat, align 8
 // CHECK11: @_ZZN5test414useStaticLocalEvE3obj = linkonce_odr global %"struct.test4::HasVTable" { ptr getelementptr inbounds ({ [3 x ptr] }, ptr @_ZTVN5test49HasVTableE, i32 0, inrange i32 0, i32 2) }, comdat, align 8
 
 struct A {
   A();
inline HasVTable &useStaticLocal() {
   return obj;
 }
 void useit() {
   useStaticLocal();
 }
 // CHECK: define linkonce_odr noundef nonnull align 8 dereferenceable(8) ptr @_ZN5test414useStaticLocalEv()
 // CHECK: ret ptr{{.*}} @_ZZN5test414useStaticLocalEvE3obj
 }
+
+#if __cplusplus >= 202002L
+// A const object with constexpr destructor can be emitted as a constant.
+namespace test5 {
+  struct A {
+    constexpr A(int x) : x_(x) {}
+    constexpr ~A() {}
+    int x_;
+  };
+  const int *f() {
+    static const A a{42};
+    return &a.x_;
+  }
+}
+#endif