[llvm] 4159fd8 - [OMPIRBuilder] Avoid crash in BasicBlock::splice. (#154987)

[via llvm-commits]

Author: Abid Qadeer
Date: 2025-08-29T16:56:26+01:00
New Revision: 4159fd819a89cb18e03a91576372ae257ee5fbc7

URL: https://github.com/llvm/llvm-project/commit/4159fd819a89cb18e03a91576372ae257ee5fbc7
DIFF: https://github.com/llvm/llvm-project/commit/4159fd819a89cb18e03a91576372ae257ee5fbc7.diff

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

Calling `BasicBlock::splice` in `spliceBB` when both `Old` and `New` are
empty is a `nop` currently but it can cause a crash once debug records
are used instead of debug intrinsics. This PR makes the call conditional
on at least one of `Old` or `New` being non-empty.

Consider the following mlir:
```
omp.target map_entries() {
  llvm.intr.dbg.declare ...
  llvm.intr.dbg.declare ...
  omp.teams ...
  ...
}
```

Current code would translate llvm.intr Ops to llvm intrinsics. Old is
the BasicBlock where they were get inserted and it will have 2 llvm
debug intrinsics by the time the implementation of `omp.teams` starts.
This implementation creates many BasicBlocks by calling `splitBB`. The
`New` is the just created BasicBlock which is empty.

In the new scheme (using debug records), there will be no instruction in
the `Old` BB after llvm.intr Ops get translated but just 2 trailing
debug records. So both `Old` and `New` are empty. When control reaches
`BasicBlock::splice`, it calls `spliceDebugInfoEmptyBlock`. This
function expects that in this case (`Src` is empty but has trailing
debug records), the `ToIt` is valid and it can call `adoptDbgRecords` on
it. This assumption is not true in this case as `New` is empty and
`ToIt` is pointing to end(). The fix is to only call
`BasicBlock::splice` when at least of `Old` or `New` is not empty.

Added: 
    

Modified: 
    llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
    llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index c0e956840f989..e740c2819fec9 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -307,7 +307,19 @@ void llvm::spliceBB(IRBuilderBase::InsertPoint IP, BasicBlock *New,
 
   // Move instructions to new block.
   BasicBlock *Old = IP.getBlock();
-  New->splice(New->begin(), Old, IP.getPoint(), Old->end());
+  // 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, `BasicBlock::splice` crashes.
+  // 2. Even if `New` is not empty, the rationale to move those records to `New`
+  // (in `spliceDebugInfoEmptyBlock`) does not apply here. That function
+  // assumes that `Old` is optimized out and is going away. This is not the case
+  // here. The `Old` block is still being used e.g. a branch instruction is
+  // added to it later in this function.
+  // 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) {
     auto *NewBr = BranchInst::Create(New, Old);

diff  --git a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
index b7a060bb3563d..c13570dc803b3 100644
--- a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -7852,4 +7852,28 @@ TEST_F(OpenMPIRBuilderTest, splitBB) {
     EXPECT_TRUE(DL == AllocaBB->getTerminator()->getStableDebugLoc());
 }
 
+TEST_F(OpenMPIRBuilderTest, spliceBBWithEmptyBB) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.Config.IsTargetDevice = false;
+  OMPBuilder.initialize();
+  F->setName("func");
+  IRBuilder<> Builder(BB);
+
+  // Test calling spliceBB with an empty Block (but having trailing debug
+  // records).
+  DIBuilder DIB(*M);
+  DISubprogram *SP = F->getSubprogram();
+  DIType *VoidPtrTy =
+      DIB.createQualifiedType(dwarf::DW_TAG_pointer_type, nullptr);
+  DILocalVariable *Var = DIB.createParameterVariable(
+      SP, "test", /*ArgNo*/ 1, SP->getFile(), /*LineNo=*/0, VoidPtrTy);
+  DIB.insertDeclare(F->getArg(0), Var, DIB.createExpression(), DL,
+                    Builder.GetInsertPoint());
+  BasicBlock *New = BasicBlock::Create(Ctx, "", F);
+  spliceBB(Builder.saveIP(), New, true, DL);
+  Instruction *Terminator = BB->getTerminator();
+  EXPECT_NE(Terminator, nullptr);
+  EXPECT_FALSE(Terminator->getDbgRecordRange().empty());
+}
+
 } // namespace