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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
