[DebugInfo] Emit distinct DILocation for different inline instances #66744
Conversation
|
@swift-ci please test |
|
The LIT test is a bit too minimal - it has some undefs in IR, which does not play nicely with optimizations. I cannot reproduce CI failures locally, but the issue is obvious. I'll change the test to avoid undefs, and we can try again. |
asavonic
force-pushed
the
debuginfo-fragment-distinct-inline-loc
branch
from
2751d98 to
43d8b5d
Compare
|
@swift-ci please test |
asavonic
force-pushed
the
debuginfo-fragment-distinct-inline-loc
branch
from
43d8b5d to
820ebe3
Compare
|
@swift-ci please test |
LLVM seems to determine a variable instance as a combination of DILocalVariable and DILocation. Therefore if multiple llvm.dbg.declare have the same variable/location parameters, they are considered to be referencing the same instance of variable. Swift IRGen emits a set of llvm.dbg.declare calls for every variable instance (with unique SILDebugScope), so it is important that these calls have distinct variable/location parameters. Otherwise their DIExpression may be incorrect when treated as referencing the same variable. For example, if they have a DIExpression with fragments, we will see this as multiple declarations of the same fragment. LLVM detects this and crashes with assertion failure: DwarfExpression.cpp:679: void llvm::DwarfExpression::addFragmentOffset(const llvm::DIExpression *): Assertion `FragmentOffset >= OffsetInBits && "overlapping or duplicate fragments"' failed. The patch resolves swiftlang#55703. The LIT test (debug_scope_distinct.swift) is the reproducer from that issue.
asavonic
force-pushed
the
debuginfo-fragment-distinct-inline-loc
branch
from
820ebe3 to
ca6e742
Compare
|
@swift-ci please test |
|
@adrian-prantl ping :) |
|
@adrian-prantl, can you please check if the patch is OK? |
|
@adrian-prantl Gentle ping |
|
(Sorry for the delayed response, I've been on vacation) In the reproducer, the inlined-at locations are all at line 0 and it looks like that is the actual problem. I think it would be okay to emit line 0 inlined-at locations as distinct to avoid this problem, but a better solution would be to ensure the autodifferentiation transformation would assign meaningful line numbers. Inlined functions with no source location at the call site are not a great user experience. Would you mind changing the patch to only apply the distinct locations to line-0 call sites? |
|
Right, it seems that the compiler lost the actual line number somewhere, and it is better to fix this issue. |
Correct. I'd still be fine with a variant of this patch to only make line 0 call site locations distinct, though I'd prefer we could assert that no function call is on line 0.
I don't think that's true. If the same function is inlined multiple times, and locations are tracked correctly, then the inlined call sites would differ and the locations would be distinct. |
This is likely because these calls are artificial, and generated by the compiler to support differentiation.
Without the patch, all it takes to get a collision of I think this is the same problem that LLVM had back in the day: |
|
Okay, if LLVM also generates all inlinedAt locations as distinct, we should match that behavior. One thing I was worried about here was that we would generate distinct inlinedAt information for two instructions originating from the same inlined call, however, these should share the same SILDebugScope on so the caching mechanism should prevent this from happening. LGTM! And thanks for pointing this out. |
|
This is failing on Android armv7, similar to previous tests that were disabled for 32-bit in #66879: @felipepiovezan, can you check if this test is failing on watchos too? |
|
@asavonic This may just be too dependent on the architecture. Can you put a REQUIRES on the test that limits it to 64 bit architectures? |
These tests depend on the target layout, and there were issues reported for Android armv7 (see swiftlang#66744) and watchOS (swiftlang#66879) targets.
|
@finagolfin, @adrian-prantl, thank you for letting me know, I missed the follow-up patch that disabled these tests for watchOS. Can you please check the patch #67699? |
LLVM seems to determine a variable instance as a combination of DILocalVariable
and DILocation. Therefore if multiple llvm.dbg.declare have the same
variable/location parameters, they are considered to be referencing the same
instance of variable.
Swift IRGen emits a set of llvm.dbg.declare calls for every variable
instance (with unique SILDebugScope), so it is important that these calls have
distinct variable/location parameters. Otherwise their DIExpression may be
incorrect when treated as referencing the same variable. For example, if they
have a DIExpression with fragments, we will see this as multiple declarations of
the same fragment. LLVM detects this and crashes with assertion failure:
DwarfExpression.cpp:679: void llvm::DwarfExpression::addFragmentOffset(const
llvm::DIExpression *): Assertion `FragmentOffset >= OffsetInBits &&
"overlapping or duplicate fragments"' failed.
The patch resolves #55703. The LIT test (debug_scope_distinct.swift) is the
reproducer from that issue.