[PATCH] D100388: [BROKEN][clang] Try to fix thunk function types

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/Basic/Thunk.h:157
+
+  /// Holds a pointer to the overridee(!) method this thunk is for,
+  /// if needed by the ABI to distinguish different thunks with equal
----------------
Isn't this the same as `BaseMethod.getDecl()`? (I'd expect "overridee" and "overridden" to be synonymous, contrasting with the "overrider" which would be the derived-class function.)


================
Comment at: clang/lib/AST/VTableBuilder.cpp:1636-1641
+    GlobalDecl BaseGD;
+    if (isa<CXXDestructorDecl>(MD))
+      BaseGD =
+          GlobalDecl(cast<CXXDestructorDecl>(MD), CXXDtorType::Dtor_Deleting);
+    else
+      BaseGD = MD;
----------------
This looks suspicious: we're going to add two vtable slots, one for the deleting destructor and one for the complete object destructor. It doesn't matter in practice because we never form a thunk for a destructor, but it would seem more correct to pass the `CXXMethodDecl*` into `AddMethod` and have it form the appropriate `GlobalDecl`(s) itself.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:451-452
-  StartThunk(Fn, GD, FnInfo, IsUnprototyped);
-  // Create a scope with an artificial location for the body of this function.
-  auto AL = ApplyDebugLocation::CreateArtificial(*this);
-
----------------
Is this done somewhere else now?


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:465-466
-  // Fix up the function type for an unprototyped musttail call.
-  if (IsUnprototyped)
-    Callee = llvm::ConstantExpr::getBitCast(Callee, Fn->getType());
 
----------------
This bitcast seems to have gone missing.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:485
   if (const CXXDestructorDecl *DD = dyn_cast<CXXDestructorDecl>(MD))
-    MCtx.mangleCXXDtorThunk(DD, GD.getDtorType(), TI.This, Out);
+    MCtx.mangleCXXDtorThunk(DD, TI.BaseMethod.getDtorType(), TI.This, Out);
   else
----------------
When do we get here for a destructor? I can't see any evidence that this is reachable at all: in our testsuite there doesn't seem to be any mention of a destructor thunk mangling.

Generally, though, thunk mangling uses the mangled name of the target of the thunk, which should be `GD.getDtorType()` here. (This might matter if we somehow emitted a thunk from the complete destructor to the base subobject destructor via this function.)


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:495
   bool IsUnprototyped = !CGM.getTypes().isFuncTypeConvertible(
       MD->getType()->castAs<FunctionType>());
   if (!shouldEmitVTableThunk(CGM, MD, IsUnprototyped, ForVTable))
----------------
I think this should probably be checking the base method type, but the difference probably doesn't matter in practice.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:507-510
+  const CGFunctionInfo &BaseFnInfo =
       IsUnprototyped ? CGM.getTypes().arrangeUnprototypedMustTailThunk(MD)
                      : CGM.getTypes().arrangeGlobalDeclaration(GD);
+  llvm::FunctionType *BaseFnTy = CGM.getTypes().GetFunctionType(BaseFnInfo);
----------------
It's confusing to call these `Base*` when (in the vtable thunk case) they're information about the derived-class function. Maybe `Target*`?


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:508
+  const CGFunctionInfo &BaseFnInfo =
       IsUnprototyped ? CGM.getTypes().arrangeUnprototypedMustTailThunk(MD)
                      : CGM.getTypes().arrangeGlobalDeclaration(GD);
----------------
This doesn't look right: the target of the thunk shouldn't itself be treated as a `musttail` thunk. Presumably we should unconditionally use `arrangeGlobalDeclaration(GD)` for the target.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:524
                                      Name.str(), &CGM.getModule());
-    CGM.SetLLVMFunctionAttributes(MD, FnInfo, ThunkFn);
+    CGM.SetLLVMFunctionAttributes(TI.BaseMethod, ThunkFnInfo, ThunkFn);
 
----------------
Hm, I think we probably want to use `MD` rather than `BaseMethod` here. Eg, if the base method is `[[noreturn]]` but the overrider is not, we want a non-`[[noreturn]]` thunk, and conversely if the overrider is `[[noreturn]]` the thunk should be regardless of whether the base method was.

Perhaps there are some attributes for which we want to look at the `BaseMethod` instead. If so, I suppose we'll need to pass both the caller-side declaration and the callee-side declaration to `ConstructAttributeList` (or maybe extend `CGFunctionInfo` to store both) and let it pick which one it wants to inspect for each attribute.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:546
 
-    setThunkProperties(CGM, TI, ThunkFn, ForVTable, GD);
+    setThunkProperties(CGM, TI, ThunkFn, ForVTable, TI.BaseMethod);
     return ThunkFn;
----------------
This should be passing `GD` rather than `BaseMethod`.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:557
 
-  CGM.SetLLVMFunctionAttributesForDefinition(GD.getDecl(), ThunkFn);
+  CGM.SetLLVMFunctionAttributesForDefinition(TI.BaseMethod.getDecl(), ThunkFn);
 
----------------
This should be passing `GD` rather than `BaseMethod`.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:592
 
-  setThunkProperties(CGM, TI, ThunkFn, ForVTable, GD);
+  setThunkProperties(CGM, TI, ThunkFn, ForVTable, TI.BaseMethod);
   return ThunkFn;
----------------
This should be passing `GD` rather than `BaseMethod`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100388