Open nikic opened 1 year ago
@llvm/issue-subscribers-debuginfo
The problem here is the two dbg.declares sort-of overlapping (one describes the location of a field in the source variable, the other describes the location of an entire variable). I don't believe we can have a well defined answer to what that means: variables can't have two locations for any part of them(DWARF can express it, but I don't believe LLVM is designed to handle that). Adding a fragment to the second dbg.declare that overlaps with the first causes a different assertion to fire for similar reasons.
IMO the solution is:
@cuviper Do you happen to still have the original, unreduced bitcode for this?
I do -- I just added it to bugzilla.
The IR that rustc produces reduces to something like this: https://gist.github.com/nikic/efc3134a0aee3cb9f71601e356c5065b This doesn't trigger an assert by itself, but does after -passes=sroa
.
However that input still has two dbg.declares on different allocas with the same DILocalVariable, so I guess that's still an invalid input, just not in a way that crashes llc.
Here's a simple example where rustc assigns the same DILocalVariable to multiple allocas: https://rust.godbolt.org/z/M4cPKhWaE
The IR contains something like this:
call void @llvm.dbg.declare(metadata ptr %s.dbg.spill, metadata !22, metadata !DIExpression()), !dbg !30
call void @llvm.dbg.declare(metadata ptr %s.dbg.spill, metadata !23, metadata !DIExpression()), !dbg !31
call void @llvm.dbg.declare(metadata ptr %s.dbg.spill, metadata !32, metadata !DIExpression()), !dbg !37
call void @llvm.dbg.declare(metadata ptr %slice.dbg.spill, metadata !23, metadata !DIExpression()), !dbg !31
call void @llvm.dbg.declare(metadata ptr %slice.dbg.spill, metadata !32, metadata !DIExpression()), !dbg !37
This looks pretty nonsensical to me.
On the producer side, this is now filed at https://github.com/rust-lang/rust/issues/115113
But on the LLVM side, it would still be nice to catch this in the IR verifier.
@jmorse Could you please clarify what the precise requirements here are? Just having multiple dbg.declares for the same DILocalVariable seems like something that should be fine when inlining is involved, as you can legitimately have multiple copies of the same variable. That behavior exists in clang as well (https://clang.godbolt.org/z/1v1orvxzW with assignment tracking disabled for more direct comparison).
So I'm not entirely clear at which point "multiple overlapping dbg.declares" becomes invalid. Is it just the case where different fragments are involved?
(The debuginfo generated by rustc is wrong in any case, I'm just trying to understand what the verifier check would actually do.)
fyi @jmorse is away at the moment.
Just having multiple dbg.declares for the same DILocalVariable seems like something that should be fine when inlining is involved, as you can legitimately have multiple copies of the same variable
In that case the DILocation attached to the debugging intrinsic has an InlinedAt field which is another DILocation that describes the original call site. After inlining that debugging intrinsic a second time its InlinedAt DILocation will have an InlinedAt field too for that inlined call site.
Unlike most DILocations, InlinedAt DILocations are created as distinct (rather than uniqued) metadata.
We generally identify a variable in LLVM as a tuple of the (DILocalVariable, FragmentInfo from the DIExpression, InlinedAt from the DILocation).
We generally identify a variable in LLVM as a tuple of the (DILocalVariable, FragmentInfo from the DIExpression, InlinedAt from the DILocation).
Thanks! This is the bit I was missing.
It looks like it's not possible to verify that there are no repeated DebugVariables, because these may be introduced by transforms that perform code cloning (e.g. LoopRotate, LoopUnswitch, etc). This is discussed in https://github.com/llvm/llvm-project/issues/32504.
So I guess the only thing we can verify is that all fragments for a given (DILocalVariable, InlinedAt) are non-overlapping (excluding exact overlaps).
Testing this patch: https://gist.github.com/nikic/d8c328c4fe13df2d71ac468923b6cec9
Results in a few test failures:
LLVM :: DebugInfo/AArch64/frameindices.ll
LLVM :: DebugInfo/X86/pieces-3.ll
LLVM :: DebugInfo/invalid-overlap.ll
LLVM :: Transforms/Inline/alloca-dbgdeclare.ll
Annoyingly, this doesn't fail verification on the IR originally produced by rustc, because it doesn't specify fragments, so it looks like the variables are exactly the same. Possibly the way to catch that would be to check whether the size implied by the DILocalVariable is consistent with the size of the alloca.
I've put up https://reviews.llvm.org/D158743 with a sanity check that the fragment can fit into the alloca, but still needs some test fixes.
This doesn't catch the case that actually asserts, but does catch the invalid debuginfo produced by SROA in Rust.
Has this been fixed in main?
@tstellar The invalid debug info generated by rustc has been fixed, no changes on the LLVM side were necessary. I'm keeping this open to track work on additional verifier checks, which is still ongoing (about to be reverted due to #68929).
From https://bugzilla.redhat.com/show_bug.cgi?id=2226564:
Results in: