Skip to content
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

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

smeenai
Copy link
Collaborator

@smeenai smeenai commented Feb 17, 2024

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.

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.
@llvmbot
Copy link
Member

llvmbot commented Feb 17, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Shoaib Meenai (smeenai)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/82080.diff

2 Files Affected:

  • (modified) llvm/lib/Transforms/IPO/MergeFunctions.cpp (+2)
  • (added) llvm/test/Transforms/MergeFunc/debuginfo-iterators.ll (+54)
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:  }

@smeenai smeenai requested review from jmorse and OCHyams February 17, 2024 00:39
@smeenai
Copy link
Collaborator Author

smeenai commented Feb 17, 2024

The test failure is unrelated and caused by b3050f5 (which was later reverted but my branch doesn't include the revert).

Copy link
Member

@jmorse jmorse left a 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.

@smeenai
Copy link
Collaborator Author

smeenai commented Feb 20, 2024

Yup, the asserts are really helpful to catch downstream issues, thanks!

@smeenai smeenai merged commit d2942a8 into llvm:main Feb 20, 2024
5 of 6 checks passed
@smeenai smeenai deleted the mergefuncs-di branch February 20, 2024 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants