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

88–89

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

91

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

7

You mention ARM but don't test ARM here.

[dschuff]

• Clean up condition, add ARM to test

[jfb]

lib/CodeGen/CGDeclCXX.cpp

92

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

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

97

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

98

|| !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

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

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

97

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

98

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

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

100

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 :(

lib/CodeGen/CGCXXABI.h

public:
   /// non-virtually for destructors).
   ///
   /// There currently is no way to indicate if a destructor returns 'this'
   /// when called virtually, and code generation does not support the case.
   virtual bool HasThisReturn(GlobalDecl GD) const { return false; }
 
   virtual bool hasMostDerivedReturn(GlobalDecl GD) const { return false; }
 
+  /// Returns true if the target allows calling a function through a pointer
+  /// with a different signature than the actual function (or equivalently,
+  /// bitcasting a function or function pointer to a different function type).
+  /// In principle in the most general case this could depend on the target, the
+  /// calling convention, and the actual types of the arguments and return
+  /// value. Here it just means whether the signature mismatch could *ever* be
+  /// allowed; in other words, does the target do strict checking of signatures
+  /// for all calls.
+  virtual bool canCallMismatchedFunctionType() const { return true; }
+
   /// If the C++ ABI requires the given type be returned in a particular way,
   /// this method sets RetAI and returns true.
   virtual bool classifyReturnType(CGFunctionInfo &FI) const = 0;
 
   /// Specify how one should pass an argument of a record type.
   enum RecordArgABI {
     /// Pass it using the normal C aggregate rules for the ABI, potentially
     /// introducing extra copies and passing some or all of it in registers.

lib/CodeGen/CGDeclCXX.cpp

static void EmitDeclDestroy(CodeGenFunction &CGF, const VarDecl &D,
   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.
-  const CXXRecordDecl *record = nullptr;
-  if (dtorKind == QualType::DK_cxx_destructor &&
-      (record = type->getAsCXXRecordDecl())) {
-    assert(!record->hasTrivialDestructor());
-    CXXDestructorDecl *dtor = record->getDestructor();
+  // Special-case non-array C++ destructors, if they have the right signature.
+  // Under some ABIs, destructors return this instead of void, and cannot be
+  // passed directly to __cxa_atexit if the target does not allow this mismatch.
+  const CXXRecordDecl *Record = type->getAsCXXRecordDecl();
+  bool CanRegisterDestructor =
+      Record && (!CGM.getCXXABI().HasThisReturn(
+                     GlobalDecl(Record->getDestructor(), Dtor_Complete)) ||
+                 CGM.getCXXABI().canCallMismatchedFunctionType());
+  // If __cxa_atexit is disabled via a flag, a different helper function is
+  // generated elsewhere which uses atexit instead, and it takes the destructor
+  // directly.
+  bool UsingExternalHelper = !CGM.getCodeGenOpts().CXAAtExit;
+  if (Record && (CanRegisterDestructor || UsingExternalHelper)) {
+    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.
   } else {
     function = CodeGenFunction(CGM)

lib/CodeGen/ItaniumCXXABI.cpp

explicit WebAssemblyCXXABI(CodeGen::CodeGenModule &CGM)
                       /*UseARMGuardVarABI=*/true) {}
 
 private:
   bool HasThisReturn(GlobalDecl GD) const override {
     return isa<CXXConstructorDecl>(GD.getDecl()) ||
            (isa<CXXDestructorDecl>(GD.getDecl()) &&
             GD.getDtorType() != Dtor_Deleting);
   }
+  bool canCallMismatchedFunctionType() const override { return false; }
 };
 }
 
 CodeGen::CGCXXABI *CodeGen::CreateItaniumCXXABI(CodeGenModule &CGM) {
   switch (CGM.getTarget().getCXXABI().getKind()) {
   // For IR-generation purposes, there's no significant difference
   // between the ARM and iOS ABIs.
   case TargetCXXABI::GenericARM:

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
+// RUN: %clang_cc1 %s -triple=armv7-apple-darwin9 -emit-llvm -o - | FileCheck --check-prefix=ARM %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)
+
+// ARM destructors return this, but can be registered directly with __cxa_atexit
+// because the calling conventions tolerate the mismatch.
+// ARM: define internal void @__cxx_global_var_init()
+// ARM-NEXT: entry:
+// ARM-NEXT:   %0 = call i32 @__cxa_atexit(void (i8*)* bitcast (%class.Foo* (%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)