Jump Threading duplicates dbg.declare intrinsics for fragments, bug?

Hi,

I'm hitting an assertion "overlapping or duplicate fragments" in the
DWARF codegen in addFragmentOffset(). This originates from a
duplicated dbg.declare intrinsic, declaring the same fragment twice.
The duplicated call was generated by the jump threading pass.

I have a patch (see below) that removes simply such duplicates, but
I'm not sure whether that is the right approach.

Cheers,
Björn

diff --git a/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
b/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index 499780a173b..308b6bd2b9f 100644
--- a/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -220,6 +220,13 @@ ArrayRef<DbgVariable::FrameIndexExpr>
DbgVariable::getFrameIndexExprs() const {
               return A.Expr->getFragmentInfo()->OffsetInBits <
                      B.Expr->getFragmentInfo()->OffsetInBits;
             });

Hi Björn,

I don't have any answers, just more confusion. Hopefully someone else can bring some light to this.

I'm also interested in dbg.declares and what the rules are regarding them since it's not very clear to me at the moment.

A similar fix as yours was made for duplicate non-fragment dbg.declares in r305244 and there is a bit of discussion about it in https://bugs.llvm.org//show_bug.cgi?id=33157
Maybe you've seen that?

I've run into a case where the inliner leaves two dbg.declares (with different locations since they origin from two different inlined call sites) connected to a single alloca after alloca-merging.

Then the inliner moves (what it thinks is) the one and only dbg.declare to the alloca (and thus leaves the other one in a loop). The help methods replaceDbgDeclareForAlloca()/replaceDbgDeclare()/FindAllocaDbgDeclare() all seems to think there is only one dbg.declare connected to one alloca. At least they all ignore all except the first one found.

Later loop unroll comes and unrolls the loop and then suddenly we have two absolutely identical dbg.declares and the assert in addFragmentOffset() blows. Who's at fault?

There is also an existing testcase that checks that there _are_ indeed two dbg.declares to a single alloca: Transforms/Inline/alloca-dbgdeclare-merge.ll

Regards,
Mikael

Hi Mikael,

A similar fix as yours was made for duplicate non-fragment dbg.declares in
r305244 and there is a bit of discussion about it in
https://bugs.llvm.org//show_bug.cgi?id=33157
Maybe you've seen that?

I saw the code, but not the bug report it originated from. Thanks!

My patch had the same idea of trying not to complicate other passes
to avoid duplicate declarations.

I've run into a case where the inliner leaves two dbg.declares (with
different locations since they origin from two different inlined call sites)
connected to a single alloca after alloca-merging.

Then the inliner moves (what it thinks is) the one and only dbg.declare to
the alloca (and thus leaves the other one in a loop). The help methods
replaceDbgDeclareForAlloca()/replaceDbgDeclare()/FindAllocaDbgDeclare() all
seems to think there is only one dbg.declare connected to one alloca. At
least they all ignore all except the first one found.

Later loop unroll comes and unrolls the loop and then suddenly we have two
absolutely identical dbg.declares and the assert in addFragmentOffset()
blows. Who's at fault?

Sounds like the same issue just with a different pass triggering the problem.

There is also an existing testcase that checks that there _are_ indeed two
dbg.declares to a single alloca:
Transforms/Inline/alloca-dbgdeclare-merge.ll

This is different. The test doesn't check for it, but the declarations actually
describe different vars in different scopes, so that is fine.

Cheers,
Björn

Hi Björn,

I don't have any answers, just more confusion. Hopefully someone else can bring some light to this.

I'm also interested in dbg.declares and what the rules are regarding them since it's not very clear to me at the moment.

A dbg.declare describes a stack-allocated variable. There may only be one dbg.declare per source variable, with the one exception that if the source variable is split up into multiple fragments (such as SROA) there may be one dbg.declare per variable fragment. A dbg.declare has no means of describing liveness of a variable and is always describing the location of the variable for its entire lexical scope.

A similar fix as yours was made for duplicate non-fragment dbg.declares in r305244 and there is a bit of discussion about it in https://bugs.llvm.org//show_bug.cgi?id=33157
Maybe you've seen that?

I've run into a case where the inliner leaves two dbg.declares (with different locations since they origin from two different inlined call sites) connected to a single alloca after alloca-merging.

Then the inliner moves (what it thinks is) the one and only dbg.declare to the alloca (and thus leaves the other one in a loop). The help methods replaceDbgDeclareForAlloca()/replaceDbgDeclare()/FindAllocaDbgDeclare() all seems to think there is only one dbg.declare connected to one alloca. At least they all ignore all except the first one found.

Later loop unroll comes and unrolls the loop and then suddenly we have two absolutely identical dbg.declares and the assert in addFragmentOffset() blows. Who's at fault?

Without having read the code yet, my intuition says that the unroller should not be duplicating dbg.declares, only dbg.values.

-- adrian

I think it's a bug in both places: the backend should tolerate identical,
duplicate dbg.declares, and the loop unroller probably shouldn't duplicate
dbg.declare, since there is no point.

IR is supposed to be duplicatable unless it is marked noduplicate. That was
ultimately the fix we applied for PR33157, right?

I guess that’s fair, yes.

– adrian

Hi all,

Thanks for the answers!

I feel like I've hijacked your thread now though Björn, sorry for that.

But from the answers it sounds like there is agreement that it's reasonable to remove the duplicates as done in Björn's patch?

Hi all,

Thanks for the answers!

I feel like I've hijacked your thread now though Björn, sorry for that.

But from the answers it sounds like there is agreement that it's reasonable to remove the duplicates as done in Björn's patch?

---

A couple of more things around the problem I saw.

> A dbg.declare describes a stack-allocated variable. There may only be > one dbg.declare per source variable, with the one exception that if
> the source variable is split up into multiple fragments (such as SROA)
> there may be one dbg.declare per variable fragment.

How about this:

%b.sroa.4.i = alloca [32 x i32]
call void @llvm.dbg.declare(metadata [32 x i32]* %b.sroa.4.i, metadata !10, metadata !DIExpression(DW_OP_LLVM_fragment, 32, 1024)), !dbg !18
call void @llvm.dbg.declare(metadata [32 x i32]* %b.sroa.4.i, metadata !10, metadata !DIExpression(DW_OP_LLVM_fragment, 32, 1024)), !dbg !20

The dbg.declares are identical except that !18 and !20 have different inlinedAt fields. The above is the result of the alloca-merging done by the inliner. After inlining two calls to the same function it merges two allocas and keeps the dbg.declares from each of them.

Note that two debug intrinsics with different inlinedAt fields are like two separate variables for all that matters. I would be surprised if that was related to you problem. (But, I keep getting surprised every day :slight_smile:

-- adrian

I agree, this seems like a bug. I plan to make this return a list in
https://reviews.llvm.org/D37768.

Great, got it!

Thanks for that and thank you both the the replies!

Regards,
Mikael