Do not register incompatible C++ destructors with __cxa_atexit

[dschuff]

Summary

For a static object with a nontrivial destructor, clang generates an
initializer function (__cxx_global_var_init) which registers that
object's destructor using __cxa_atexit. However some ABIs (ARM,
WebAssembly) use destructors that return 'this' instead of having void
return (which does not match the signature of function pointers passed
to __cxa_atexit). WebAssembly requires that all function signatures
of indirect calls exactly match the called function, so it raises an error
when the destructors are called.

This patch introduces CGCXXABI::canCalMismatchedFunctionType()
to specify whether this mismatch is allowed, and disables the direct
registration of destructors for ABIs that have this-returning destructors
on platforms which cannot tolerate the mismatch.

[dschuff]

Will want a reviewer who's involved with ARM too, still looking.

[jfb]

lib/CodeGen/CGDeclCXX.cpp

94 ↗

(On Diff #54228)

This assignment is looking pretty ugly now, would be better to move out of line.

97 ↗

(On Diff #54228)

Could you also pull out these conditions into their own bool? I find the ands and ors hard to follow!

test/CodeGenCXX/static-destructor.cpp

6 ↗

(On Diff #54228)

You mention ARM but don't test ARM here.

[dschuff]

• Clean up condition, add ARM to test

[jfb]

lib/CodeGen/CGDeclCXX.cpp

92 ↗

(On Diff #54260)

Can you do that unconditionally? Or do you have to test for dtorKind == QualType::DK_cxx_destructor first? It looks like the function already does a dyncast_or_null so the previous code was doing a check it didn't need to.

95 ↗

(On Diff #54260)

Comment on why you can't register dtor when has this return (ABI mismatch).

97 ↗

(On Diff #54260)

Do you need to check Record here? Doesn't it also imply dtorKind == QualType::DK_cxx_destructor?

98 ↗

(On Diff #54260)

|| !CGM.getCodeGenOpts().CXAAtExit seems wrong here. Shouldn't the condition be CanRegisterDestructor && CGM.getCodeGenOpts().CXAAtExit? I may be misunderstanding...

[dschuff]

• Clarify condition, remove redundant check

lib/CodeGen/CGDeclCXX.cpp

92 ↗

(On Diff #54260)

Yes, that's why I made it unconditional. And actually I just noticed that the switch statement above returns from the function if dtorKind != DK_cxx_destructor anyway, so I removed the check here too.

95 ↗

(On Diff #54260)

Doesn't the new text in the comment right above on line 98 cover that?

97 ↗

(On Diff #54260)

Yes, there can be a case where Record is nullptr and CGM.getCodeGenOpts().CXAAtExit is false.

98 ↗

(On Diff #54260)

I split and expanded the comment and made a variable with a name. See if it makes more sense now.

[jfb]

lgtm besides two nits. Would be good to get a review from @t.p.northover or someone from ARM.

lib/CodeGen/CGDeclCXX.cpp

95 ↗

(On Diff #54264)

Connecting the dots between "the right signature" and "has this return" isn't obvious IMO.

100 ↗

(On Diff #54264)

clang-format

[dschuff]

• Clean up condition, add ARM to test
• Clarify condition, remove redundant check
• more cleanup

[dschuff]

any comments from @t.p.northover or perhaps @rengolin ?
(This does have a potential new cost on ARM, as it requires one thunk (which is just a tail call) per static initializer on ARM and WebAssembly)

[t.p.northover]

I'm opposed to the changes on ARM. It would be undefined behaviour if the user wrote it at the C level, but not LLVM IR I think, which means Clang is perfectly entitled to emit calls like that.

Tim.

[jfb]

@t.p.northover: would an acceptable solution be to add a new trait such as CGM.getCXXABI().CanCallMismatchedFunctionType() or something of the sort? ARM would set it to true, wasm to false.

[rnk]

+1 for canCallMismatchedFunctionType.

I guess there's a more general issue here which is that LLVM appears to be more permissive about prototype mismatch than wasm. Specifically, I'm thinking about how swifterror relies on being able to pass extra parameters to a function that doesn't expect them and not get UB. I guess as long as LLVM doesn't introduce any prototype mismatch during optimization, that's OK, but this seems like it would be a real problem if we ever wanted to port swift to wasm.

[t.p.northover]

Yep, that sounds about perfect to me. Exactly what the different CXXABI classes are for, really.

Tim.

[jfb]

I guess there's a more general issue here which is that LLVM appears to be more permissive about prototype mismatch than wasm. Specifically, I'm thinking about how swifterror relies on being able to pass extra parameters to a function that doesn't expect them and not get UB. I guess as long as LLVM doesn't introduce any prototype mismatch during optimization, that's OK, but this seems like it would be a real problem if we ever wanted to port swift to wasm.

Yeah @dschuff, @sunfish and I were also talking about __cxa_pure_virtual being in the same signature-type-mismatch category. I wonder if this also makes CFI sad, /cc @pcc.

[dschuff]

• Introduce CGCXXABI::canCallMismatchedFunctionType

[dschuff]

Thanks for the feedback, PTAL

[dschuff]

Ping?

[jfb]

lgtm

[rnk]

lgtm

[dschuff]

Well, I forgot to squash my local commits before landing, so this is r269085 thru r269089 :(

cfe/trunk/lib/CodeGen/CGDeclCXX.cpp

case QualType::DK_objc_weak_lifetime:
     // We don't care about releasing objects during process teardown.
     assert(!D.getTLSKind() && "should have rejected this");
     return;
   }
 
   llvm::Constant *function;
   llvm::Constant *argument;
 
-  // Special-case non-array C++ destructors, where there's a function
-  // with the right signature that we can just call.
+  // Special-case non-array C++ destructors, if they have the right signature
+  // that can be directly registered with __cxa_atexit. If __cxa_atexit is
+  // disabled via a flag, a different helper function is generated anyway.
   const CXXRecordDecl *record = nullptr;
   if (dtorKind == QualType::DK_cxx_destructor &&
-      (record = type->getAsCXXRecordDecl())) {
+      (record = type->getAsCXXRecordDecl()) &&
+      (!CGM.getCXXABI().HasThisReturn(
+          GlobalDecl(record->getDestructor(), Dtor_Complete)) ||
+       !CGM.getCodeGenOpts().CXAAtExit)) {
     assert(!record->hasTrivialDestructor());
     CXXDestructorDecl *dtor = record->getDestructor();
 
     function = CGM.getAddrOfCXXStructor(dtor, StructorType::Complete);
     argument = llvm::ConstantExpr::getBitCast(
         addr.getPointer(), CGF.getTypes().ConvertType(type)->getPointerTo());
 
   // Otherwise, the standard logic requires a helper function.

cfe/trunk/test/CodeGenCXX/runtimecc.cpp

struct A {
     double d;
     A();
     ~A();
   };
 
   A global;
 // CHECK-LABEL:    define internal void @__cxx_global_var_init()
 // CHECK:      call [[A]]* @_ZN5test01AC1Ev([[A]]* @_ZN5test06globalE)
-// CHECK-NEXT: call i32 @__cxa_atexit(void (i8*)* bitcast ([[A]]* ([[A]]*)* @_ZN5test01AD1Ev to void (i8*)*), i8* bitcast ([[A]]* @_ZN5test06globalE to i8*), i8* @__dso_handle) [[NOUNWIND:#[0-9]+]]
+// CHECK-NEXT: call i32 @__cxa_atexit(void (i8*)* @__cxx_global_array_dtor, i8* null, i8* @__dso_handle) [[NOUNWIND:#[0-9]+]]
 // CHECK-NEXT: ret void
+
+// CHECK-LABEL: define internal void @__cxx_global_array_dtor(i8*)
+// CHECK: call [[A]]* @_ZN5test01AD1Ev([[A]]* @_ZN5test06globalE)
+// CHECK-NEXT: ret void
+
 }
 
 // CHECK: declare i32 @__cxa_atexit(void (i8*)*, i8*, i8*) [[NOUNWIND]]
 
 namespace test1 {
   void test() {
     throw 0;
   }

cfe/trunk/test/CodeGenCXX/static-destructor.cpp

+// RUN: %clang_cc1 %s -triple=x86_64-pc-linux -emit-llvm -o - | FileCheck --check-prefix=X86 %s
+// RUN: %clang_cc1 %s -triple=wasm32 -emit-llvm -o - | FileCheck --check-prefix=WASM %s
+
+// Test that destructors are not passed directly to __cxa_atexit when their
+// signatures do not match the type of its first argument.
+// e.g. ARM and WebAssembly have destructors that return this instead of void.
+
+
+class Foo {
+ public:
+  ~Foo() {
+  }
+};
+
+Foo global;
+
+// X86 destructors have void return, and are registered directly with __cxa_atexit.
+// X86: define internal void @__cxx_global_var_init()
+// X86-NEXT: entry:
+// X86-NEXT:   %0 = call i32 @__cxa_atexit(void (i8*)* bitcast (void (%class.Foo*)* @_ZN3FooD1Ev to void (i8*)*), i8* getelementptr inbounds (%class.Foo, %class.Foo* @global, i32 0, i32 0), i8* @__dso_handle)
+
+// Wasm destructors return this, and use a wrapper function, which is registered
+// with __cxa_atexit.
+// WASM: define internal void @__cxx_global_var_init()
+// WASM-NEXT: entry:
+// WASM-NEXT: %0 = call i32 @__cxa_atexit(void (i8*)* @__cxx_global_array_dtor, i8* null, i8* @__dso_handle)
+
+// WASM: define internal void @__cxx_global_array_dtor(i8*)
+// WASM: %call = call %class.Foo* @_ZN3FooD1Ev(%class.Foo* @global)