Skip to content

[OMPIRBuilder] Avoid crash in BasicBlock::splice. #154987

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.

Sign up for GitHub

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
abidh merged 4 commits into from
Aug 29, 2025
Merged

[OMPIRBuilder] Avoid crash in BasicBlock::splice. #154987

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f8f9835
[OMPIRBuilder] Avoid crash in BasicBlock::splice.
abidh Aug 22, 2025
3654d52
Add a unit test.
abidh Aug 26, 2025
6e02d88
Avoid splice call only when Old is empty.
abidh Aug 29, 2025
70bacd0
Update comments.
abidh Aug 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,14 @@ void llvm::spliceBB(IRBuilderBase::InsertPoint IP, BasicBlock *New,

// Move instructions to new block.
BasicBlock *Old = IP.getBlock();
if (!Old->empty() || !New->empty())
// If the Old block is empty then there are no instructions to move. But in
// the new debug scheme, it could have trailing debug records which will be
// moved to New in spliceDebugInfoEmptyBlock. We dont want that for 2 reasons:
// 1. If New is also empty, it could cause a crash.
Copy link
Member

@Meinersbur Meinersbur Aug 29, 2025

Choose a reason for hiding this comment

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

Suggested change
// 1. If New is also empty, it could cause a crash.
// 1. If New is also empty, `BasicBlock::splice` crashes

[nit] to be specific. If it doesn't crash anymore, we know it has been fixed.

// 2. Even if New is not empty, we want to keep those debug records in the
// Old as that was the behavior with the old scheme (debug intrinsics).
Copy link
Member

@Meinersbur Meinersbur Aug 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
// 2. Even if New is not empty, we want to keep those debug records in the
// Old as that was the behavior with the old scheme (debug intrinsics).
// 2. Even if New is not empty, we want to keep those debug records in the
// Old as that was the behavior with the old scheme (debug intrinsics).

[nit] Just being the legacy scheme doesn't mean it is more correct. Consider something that mentions the rationale from BasicBlock::spliceDebugInfoEmptyBlock:

  // If the source block is completely empty, including no terminator, then
  // transfer any trailing DbgRecords that are still hanging around. This can
  // occur when a block is optimised away and the terminator has been moved
  // somewhere else.

which does not apply here since we still want to use Old BB (like adding a BranchInst with CreateBranch). And/or that those trailing DbgRecords should only exist if Old is empty since there is no other place to attach them.

// So we call BasicBlock::splice only when Old is not empty.
if (!Old->empty())
New->splice(New->begin(), Old, IP.getPoint(), Old->end());

if (CreateBranch) {
Expand Down