-
Notifications
You must be signed in to change notification settings - Fork 13.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MergeFunctions] Fix thunks for non-instruction debug info #82080
Conversation
When MergeFunctions creates new thunk functions, it needs to copy over the debug info format kind from the original function, otherwise we'll mix debug info formats and run into assertions. This was exposed by a downstream change that runs MergeFunctions before inlining, which caused assertions when inlining attempted to inline thunks created by merging, and the added test covers both scenarios where merging creates thunks.
@llvm/pr-subscribers-llvm-transforms Author: Shoaib Meenai (smeenai) ChangesWhen MergeFunctions creates new thunk functions, it needs to copy over Full diff: https://github.com/llvm/llvm-project/pull/82080.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/MergeFunctions.cpp b/llvm/lib/Transforms/IPO/MergeFunctions.cpp
index 4af9b2206745e2..a704694198b898 100644
--- a/llvm/lib/Transforms/IPO/MergeFunctions.cpp
+++ b/llvm/lib/Transforms/IPO/MergeFunctions.cpp
@@ -747,6 +747,7 @@ void MergeFunctions::writeThunk(Function *F, Function *G) {
NewG = Function::Create(G->getFunctionType(), G->getLinkage(),
G->getAddressSpace(), "", G->getParent());
NewG->setComdat(G->getComdat());
+ NewG->IsNewDbgInfoFormat = G->IsNewDbgInfoFormat;
BB = BasicBlock::Create(F->getContext(), "", NewG);
}
@@ -874,6 +875,7 @@ void MergeFunctions::mergeTwoFunctions(Function *F, Function *G) {
F->getAddressSpace(), "", F->getParent());
NewF->copyAttributesFrom(F);
NewF->takeName(F);
+ NewF->IsNewDbgInfoFormat = F->IsNewDbgInfoFormat;
// Ensure CFI type metadata is propagated to the new function.
copyMetadataIfPresent(F, NewF, "type");
copyMetadataIfPresent(F, NewF, "kcfi_type");
diff --git a/llvm/test/Transforms/MergeFunc/debuginfo-iterators.ll b/llvm/test/Transforms/MergeFunc/debuginfo-iterators.ll
new file mode 100644
index 00000000000000..7f259159ce9d33
--- /dev/null
+++ b/llvm/test/Transforms/MergeFunc/debuginfo-iterators.ll
@@ -0,0 +1,54 @@
+;; Ensure that the MergeFunctions pass creates thunks with the appropriate debug
+;; info format set (which would otherwise assert when inlining those thunks).
+; RUN: opt -S -passes=mergefunc,inline --try-experimental-debuginfo-iterators < %s | FileCheck %s
+
+declare void @f1()
+declare void @f2()
+
+define void @f3() {
+ call void @f1()
+ call void @f2()
+ ret void
+}
+
+;; MergeFunctions will replace f4 with a thunk that calls f3. Inlining will
+;; inline f3 into that thunk, which would assert if the thunk had the incorrect
+;; debug info format.
+define void @f4() {
+ call void @f1()
+ call void @f2()
+ ret void
+}
+
+; CHECK-LABEL: define void @f4() {
+; CHECK-NEXT: call void @f1()
+; CHECK-NEXT: call void @f2()
+; CHECK-NEXT: ret void
+; CHECK-NEXT: }
+
+;; Both of these are interposable, so MergeFunctions will create a common thunk
+;; that both will call. Inlining will inline that thunk back, which would assert
+;; if the thunk had the incorrect debug info format.
+define weak void @f5() {
+ call void @f2()
+ call void @f1()
+ ret void
+}
+
+define weak void @f6() {
+ call void @f2()
+ call void @f1()
+ ret void
+}
+
+; CHECK-LABEL: define weak void @f6() {
+; CHECK-NEXT: call void @f2()
+; CHECK-NEXT: call void @f1()
+; CHECK-NEXT: ret void
+; CHECK-NEXT: }
+
+; CHECK-LABEL: define weak void @f5() {
+; CHECK-NEXT: call void @f2()
+; CHECK-NEXT: call void @f1()
+; CHECK-NEXT: ret void
+; CHECK-NEXT: }
|
The test failure is unrelated and caused by b3050f5 (which was later reverted but my branch doesn't include the revert). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; sorry this is generating noise, but it's saved great confusion from mixing formats elsewhere.
Yup, the asserts are really helpful to catch downstream issues, thanks! |
When MergeFunctions creates new thunk functions, it needs to copy over
the debug info format kind from the original function, otherwise we'll
mix debug info formats and run into assertions. This was exposed by a
downstream change that runs MergeFunctions before inlining, which caused
assertions when inlining attempted to inline thunks created by merging,
and the added test covers both scenarios where merging creates thunks.