[WholeProgramDevirt] Add check for AvailableExternal and give up icall.branch.funnel #143468
Conversation
…devirtualization. When a customer class inherits from a libc++ class, and is built with "-flto -fwhole-program-vtables -static-libstdc++ \ -Wl,-plugin-opt=-whole-program-visibility", the libc++ class's vtable is available_externally, meanwhile the customer class vtable is private. And both of them are !vcall_visibility == Linkage Unit. In this case, icall.branch.funnel might be generated. But the icall.branch.funnel would cause crash in LowerTypeTests because available_externally Global_Object is skipped to save and leads to a NULL GlobalTypeMember. Even walking around the crash in LowerTypeTests, it still crashes in SelectionDAGBuilder or VerifierPass, because they ask operands of icall.branch.funnel must be the same GlobalValue. This patch only fix fullLTO mode.
|
@llvm/pr-subscribers-llvm-transforms Author: Tianle Liu (tianleliu) Changes…devirtualization. When a customer class inherits from a libc++ class, and is built with But the icall.branch.funnel would cause crash in LowerTypeTests This patch only fix fullLTO mode. Full diff: https://github.com/llvm/llvm-project/pull/143468.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
index a7d9f3ba24b24..c0ddb50ea83ed 100644
--- a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
+++ b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
@@ -1093,6 +1093,7 @@ bool DevirtModule::tryFindVirtualCallTargets(
std::vector<VirtualCallTarget> &TargetsForSlot,
const std::set<TypeMemberInfo> &TypeMemberInfos, uint64_t ByteOffset,
ModuleSummaryIndex *ExportSummary) {
+ bool hasAvailableExternally = false;
for (const TypeMemberInfo &TM : TypeMemberInfos) {
if (!TM.Bits->GV->isConstant())
return false;
@@ -1103,6 +1104,16 @@ bool DevirtModule::tryFindVirtualCallTargets(
GlobalObject::VCallVisibilityPublic)
return false;
+ // Record if the first GV is AvailableExternally
+ if (TargetsForSlot.empty())
+ hasAvailableExternally = TM.Bits->GV->hasAvailableExternallyLinkage();
+
+ // When the first GV is AvailableExternally, check if all other GVs are
+ // also AvailableExternally. If they are not the same, return false.
+ if (!TargetsForSlot.empty() && hasAvailableExternally &&
+ !TM.Bits->GV->hasAvailableExternallyLinkage())
+ return false;
+
Function *Fn = nullptr;
Constant *C = nullptr;
std::tie(Fn, C) =
diff --git a/llvm/test/Transforms/WholeProgramDevirt/availableexternal-check.ll b/llvm/test/Transforms/WholeProgramDevirt/availableexternal-check.ll
new file mode 100644
index 0000000000000..d72075cdc1bb3
--- /dev/null
+++ b/llvm/test/Transforms/WholeProgramDevirt/availableexternal-check.ll
@@ -0,0 +1,57 @@
+; RUN: opt -S -passes=wholeprogramdevirt -whole-program-visibility %s | FileCheck %s
+
+; This test is reduced from C++ code like this:
+; class A :public std::exception {
+; public:
+; A() {};
+; const char* what () const throw () {return "A";}
+; };
+; long test(std::exception *p) {
+; const char* ch = p->what();
+; return std::strlen(ch);
+; }
+;
+; Build command is "clang++ -O2 -target x86_64-unknown-linux -flto=thin \
+; -fwhole-program-vtables -static-libstdc++ -Wl,-plugin-opt=-whole-program-visibility"
+;
+; _ZTVSt9exception's visibility is 1 (Linkage Unit), and available_externally.
+; But another vtable _ZTV1A.0 is not available_externally.
+; They should not do devirtualization because they are in different linkage type.
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux"
+
+@_ZTVSt9exception = available_externally constant { [5 x ptr] } { [5 x ptr] [ptr null, ptr null, ptr null, ptr null, ptr @_ZNKSt9exception4whatEv] }, !type !0, !type !1, !vcall_visibility !2
+@_ZTV1A.0 = constant [5 x ptr] [ptr null, ptr null, ptr null, ptr null, ptr @_ZNK1A4whatEv], !type !3, !type !4, !type !5, !type !6, !vcall_visibility !2
+
+declare ptr @_ZNKSt9exception4whatEv()
+
+define i64 @_Z4testPSt9exception() {
+ %1 = call i1 @llvm.type.test(ptr null, metadata !"_ZTSSt9exception")
+ tail call void @llvm.assume(i1 %1)
+ %2 = getelementptr i8, ptr null, i64 16
+ %3 = load ptr, ptr %2, align 8
+ %4 = tail call ptr %3(ptr null)
+ ret i64 0
+}
+
+; Function Attrs: nocallback nofree nosync nounwind willreturn memory(inaccessiblemem: write)
+declare void @llvm.assume(i1 noundef) #0
+
+declare ptr @_ZNK1A4whatEv()
+
+; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
+declare i1 @llvm.type.test(ptr, metadata) #1
+
+; CHECK-NOT: call void (...) @llvm.icall.branch.funnel
+
+attributes #0 = { nocallback nofree nosync nounwind willreturn memory(inaccessiblemem: write) }
+attributes #1 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }
+
+!0 = !{i64 16, !"_ZTSSt9exception"}
+!1 = !{i64 32, !"_ZTSMSt9exceptionKDoFPKcvE.virtual"}
+!2 = !{i64 1}
+!3 = !{i32 16, !"_ZTS1A"}
+!4 = !{i32 32, !"_ZTSM1AKDoFPKcvE.virtual"}
+!5 = !{i32 16, !"_ZTSSt9exception"}
+!6 = !{i32 32, !"_ZTSMSt9exceptionKDoFPKcvE.virtual"}
|
| std::vector<VirtualCallTarget> &TargetsForSlot, | ||
| const std::set<TypeMemberInfo> &TypeMemberInfos, uint64_t ByteOffset, | ||
| ModuleSummaryIndex *ExportSummary) { | ||
| bool hasAvailableExternally = false; |
There was a problem hiding this comment.
| bool hasAvailableExternally = false; | |
| bool HasAvailableExternally = false; |
teresajohnson
left a comment
teresajohnson
left a comment
There was a problem hiding this comment.
Hoping @pcc will take a look, but a couple questions after skimming the changes.
Rather than completely shut off devirtualization, would it be better to simply suppress using a branch funnel? I.e. check the GV linkage types of the recorded target GVs in DevirtModule::tryICallBranchFunnel.
Also, I noticed that the branch funnel intrinsic is not documented, so I don't fully understand what this instruction does (will ping #133635).
| if (TargetsForSlot.empty()) | ||
| hasAvailableExternally = TM.Bits->GV->hasAvailableExternallyLinkage(); | ||
|
|
||
| // When the first GV is AvailableExternally, check if all other GVs are |
There was a problem hiding this comment.
Why is it ok if they are all available externally?
There was a problem hiding this comment.
This is a conservative checking that does not stop devirtualization for all available externally.
Because when doing the check of available externally, checking of VCallVisibility != VCallVisibilityPublic is pass, which means all virtual function calls from this available externally vtable are in the current LTO unit. So I guess a icall.branch.funnel whose parameters all relate to available externally vtable is allowed to generated?
There was a problem hiding this comment.
So I guess a icall.branch.funnel whose parameters all relate to available externally vtable is allowed to generated?
If this is a question for me, I don't know the answer. Unfortunately this intrinsic isn't documented so I don't know the semantics. Do you have a test case where all vtables are available externally, and how does that work?
Hi @teresajohnson Thanks for your review! |
They are declarations for the linker but we do have a body in the module if they are available externally. How does this linkage type cause trySingleImplDevirt and tryVirtualConstProp to not work - I don't see them checking isDeclarationForLinker or the linkage type directly. |
|
Hi @teresajohnson Thanks for your review.
I mean vtable's all function's body would be checked. If any of the functions is a declaration, devirtualization of tryVirtualConstProp would drop. In code, checking function by isDeclaration() but not isDeclarationForLinker(). So I guess if function is available_externally, the devirt would not drop. But I am curious what a real case that a external function is regarded as available_externally? You are right for trySingleImplDevirt that it does not check function's body. No matter if vtable is available_externally or not, it would do devirtualization if vtable's function are the same. The example is as below:
I write a all vtables are available externally and it crashes in LowerTypeTests. The crash is because it skips dealing with available externally. And code is at Even I walk around it, there is still linkage type issue that not pass VerifierPass because all available externally always combined to privateLinkage GV at For example: @_ZTVSt9exception = available_externally constant { [5 x ptr] } { [5 x ptr] [ptr null, ptr null, ptr null, ptr null, ptr @_ZNKSt9exception4whatEv] }@_ZTV1A.0 = available_externally constant [5 x ptr] [ptr null, ptr null, ptr null, ptr null, ptr @_ZNK1A4whatEv]tends to combine to: @1 = private constant { { [5 x ptr] }, [24 x i8], [5 x ptr] } { { [5 x ptr] } { [5 x ptr] [ptr null, ptr null, ptr null, ptr null, ptr @_ZNKSt9exception4whatEv] }, [24 x i8] zer oinitializer, [5 x ptr] [ptr null, ptr null, ptr null, ptr null, ptr @_ZNK1A4whatEv] }, align 8@_ZTVSt9exception = available_externally alias { [5 x ptr] }, ptr @1@_ZTV1A.0 = available_externally alias [5 x ptr], getelementptr inbounds ({ { [5 x ptr] }, [24 x i8], [5 x ptr] }, ptr @1, i32 0, i32 2)I am not sure if they are bugs for LowerTypeTests? If assuming LowerTypeTests is right, I guess any available externally for icall.branch.funnel should be stopped. So do you agree that I postpone available externally in tryICallBranchFunnel, and any one of available externally vtable would stop icall.branch.funnel? |
Not sure but we should probably be consistent unless there is a reason not to.
This sounds good, since it is the icall.branch.funnel that has an issue with these vtables, go ahead and put the checks there. I'm not sure if this is a limitation of LowerTypeTests, but in the absence of input from someone more familiar with this intrinsics it makes sense to be conservative in apply that transformation. |
tianleliu
changed the title
teresajohnson
left a comment
teresajohnson
left a comment
There was a problem hiding this comment.
Code change and test look good. In the absence of a review from someone more familiar with this construct, this seems like the right approach to at least work around crashes in LTT. I have a few questions and suggested fixes in the code comment and the PR description for readability and correctness.
| if (!HasNonDevirt) | ||
| return; | ||
|
|
||
| // If any GV is AvailableExternally, drop to generate branch.funnel |
There was a problem hiding this comment.
Change "drop to generate" to "do not generate". Also, please add a note that this avoids a crash in LowerTypeTest due to the GV being dropped resulting in a null GlobalTypeMember.
Not sure what "skipped to save" means - do you mean "is dropped" (e.g. by the ElimAvailExtern pass)? "walking around" should be "working around" "they ask operands ... must be" should be "they ask operands ... to be" (maybe "they expect operands ... to be" would be clearer)
I would think it would also fix split-LTO-unit ThinLTO mode (e.g. "-flto=thin -fsplit-lto-unit" which splits the module into ThinLTO and regular LTO sub modules - is that not the case?) |
The "skip" is not in ElimAvailExtern but LowerTypeTestsModule::lower(). Logic is like this: for (GlobalObject &GO : M.global_objects()) { if (isa(GO) && GO.isDeclarationForLinker()) continue; .... auto *GTM = GlobalTypeMember::create(Alloc, &GO, IsJumpTableCanonical, IsExported, Types); GlobalTypeMembers[&GO] = GTM; .... } when GO.isDeclarationForLinker, it would "continue" and nothing would saved in GlobalTypeMembers[&GO]. So in GTM would be NULL and the NULL would be saved in which would lead crash in https://github.com/llvm/llvm-project/blob/9d491bc602c2d9730cb42fe25f0753471a3af389/llvm/lib/Transforms/IPO/LowerTypeTests.cpp#L2406C7-L2406C34
The experiment of what I "walking around" (or take a detour?) is replacing GO.isDeclarationForLinker() to GO.isDeclaration() to not run "continue" and make GlobalTypeMembers[&GO] is not NULL. It can avoid crash in buildBitSetsFromDisjointSet. But it will meet report_fatal_error in Verifier or SelectionDAGBuilder as below: llvm-project/llvm/lib/IR/Verifier.cpp Lines 946 to 947 in 9d491bc or llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp Lines 7868 to 7869 in 9d491bc |
Yes. The patch also fixes crash for "-flto=thin -fsplit-lto-unit" mode. |
teresajohnson
left a comment
teresajohnson
left a comment
There was a problem hiding this comment.
Thanks for the explanations. lgtm
4 participants
When a customer class inherits from a libc++ class, and is built with
"-flto -fwhole-program-vtables -static-libstdc++
-Wl,-plugin-opt=-whole-program-visibility", the libc++ class's vtable is
available_externally, meanwhile the customer class vtable is private. And
both of them are !vcall_visibility == Linkage Unit.
In this case, icall.branch.funnel might be generated.
But the icall.branch.funnel would cause crash in LowerTypeTests because
available_externally Global_Object's GlobalTypeMember would not be
saved and finally leads to a NULL GlobalTypeMember which causes a crash.
Even saving the available_externally GO's GlobalTypeMember so that it is
not NULL to avoid the crash in LowerTypeTests, it still will crash in
SelectionDAGBuilder or Verifier, because operands linkage type consistency
check of icall.branch.funnel can not pass.
So any one of available externally vtable would stop to generate icall.branch.funnel.
This patch fixes FullLTO mode and split-LTO-unit ThinLTO mode.