Skip to content

[RemoveDIs] Fix asan-identified leak in unittest #106723

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

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

OCHyams
Copy link
Contributor

@OCHyams OCHyams commented Aug 30, 2024

Fixes issue found here #106691 (comment)

The issue wasn't in the code change itself, just the unittest; the trailing marker wasn't properly cleaned up.

@llvmbot
Copy link
Member

llvmbot commented Aug 30, 2024

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-debuginfo

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

Fixes issue found here #106691 (comment)

The issue wasn't in the code change itself, just the unittest; the trailing marker wasn't properly cleaned up.


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

1 Files Affected:

  • (modified) llvm/unittests/IR/BasicBlockDbgInfoTest.cpp (+5-7)
diff --git a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
index 5615a4493d20a1..5ce14d3f6b9cef 100644
--- a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
+++ b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
@@ -1569,14 +1569,12 @@ TEST(BasicBlockDbgInfoTest, CloneTrailingRecordsToEmptyBlock) {
   // The trailing records should've been absorbed into NewBB.
   EXPECT_FALSE(BB.getTrailingDbgRecords());
   EXPECT_TRUE(NewBB->getTrailingDbgRecords());
-  if (NewBB->getTrailingDbgRecords()) {
-    EXPECT_EQ(
-        llvm::range_size(NewBB->getTrailingDbgRecords()->getDbgRecordRange()),
-        1u);
+  if (DbgMarker *Trailing = NewBB->getTrailingDbgRecords()) {
+    EXPECT_EQ(llvm::range_size(Trailing->getDbgRecordRange()), 1u);
+    // Drop the trailing records now, to prevent a cleanup assertion.
+    Trailing->eraseFromParent();
+    NewBB->deleteTrailingDbgRecords();
   }
-
-  // Drop the trailing records now, to prevent a cleanup assertion.
-  NewBB->deleteTrailingDbgRecords();
 }
 
 } // End anonymous namespace.

@dklimkin
Copy link
Member

Confirmed this fixes the asan issue. Thanks!

@dklimkin dklimkin merged commit 7ffe67c into llvm:main Aug 30, 2024
9 of 10 checks passed
Copy link
Member

@dklimkin dklimkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@OCHyams OCHyams added this to the LLVM 19.X Release milestone Aug 30, 2024
@OCHyams
Copy link
Contributor Author

OCHyams commented Aug 30, 2024

/cherry-pick 7ffe67c

@llvmbot
Copy link
Member

llvmbot commented Aug 30, 2024

Failed to cherry-pick: 7ffe67c

https://github.com/llvm/llvm-project/actions/runs/10633781920

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

@vitalybuka
Copy link
Collaborator

It would be nice to mention the original patch and issue introduced the leak, #105671, #105571.

@OCHyams
Copy link
Contributor Author

OCHyams commented Sep 2, 2024

It would be nice to mention the original patch and issue introduced the leak

Ah yes I should have done that - sorry for any inconvenience there, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants