[DebugInfo] Merge fragments from SIL with fragments created in IRGen #66448
Conversation
SIL variables can be split by SILSROA into separate allocations, each having op_fragment expressions in debug_value (VarInfo.DIExpr). These allocations can be further split by IRGen (multiple values in Storage argument). These "nested" fragments refer to the same DI variable, so it is important to merge them for the LLVM IR DI expression. The compiler used to ignore fragment expressions from SIL when IRGen fragments were also present. This led to incorrect DI info generation, and for some cases even triggered assertions in LLVM X86 CodeGen: DwarfExpression.cpp:679: void llvm::DwarfExpression::addFragmentOffset(const llvm::DIExpression *): Assertion `FragmentOffset >= OffsetInBits && "overlapping or duplicate fragments"' failed. The patch fixes issue swiftlang#64642. The LIT test is a reduced reproducer from that issue.
|
@swift-ci please test |
|
Tagging @BradLarson |
|
@swift-ci please test |
This reverts commit ab1ce9e. Does not help to fix CI failures.
|
@swift-ci please test |
|
@adrian-prantl ping :) |
lib/IRGen/IRGenDebugInfo.cpp
Outdated
| @@ -215,10 +215,11 @@ class IRGenDebugInfoImpl : public IRGenDebugInfo { | |||
|
|
|||
| /// Return false if we fail to create the right DW_OP_LLVM_fragment operand. | |||
| bool handleFragmentDIExpr(const SILDIExprOperand &CurDIExprOp, | |||
| SmallVectorImpl<uint64_t> &Operands); | |||
| std::pair<unsigned, unsigned> &Fragment); | |||
There was a problem hiding this comment.
What do you think about using an llvm::DIExpression::FragmentInfo here, so it's more clear which is which?
| @@ -0,0 +1,40 @@ | |||
| // RUN: %target-swift-frontend -disable-availability-checking -primary-file %s -emit-sil -O -g | %FileCheck %s --check-prefix CHECK-SIL | |||
| // RUN: %target-swift-frontend -disable-availability-checking -primary-file %s -emit-ir -disable-llvm-optzns -O -g | %FileCheck %s | |||
There was a problem hiding this comment.
It would be wonderful if we could have a test that uses textual SIL as input here, so it isn't depending on specific optimization decisions in the compiler pipeline.
SIL serialization for debug info isn't perfect, so let me know if this isn't an option!
There was a problem hiding this comment.
I reduced the reproducer into a somewhat shorter SIL. It shows the same issue in LLVM IR after IRGen, but it does not cause LLVM CG crash as the original.
lib/IRGen/IRGenDebugInfo.cpp
Outdated
| // When the fragment of the SIL variable is further split into other | ||
| // fragments (PieceFragment), merge them into one DW_OP_LLVM_Fragment | ||
| // expression. | ||
| if (PieceFragmentSize) |
There was a problem hiding this comment.
Should we assert that PieceFragmentSize < VarFragmentSize here?
There was a problem hiding this comment.
This is checked by createFragmentExpression:
assert((OffsetInBits + SizeInBits <= FragmentSizeInBits) &&
"new fragment outside of original fragment");
where FragmentSizeInBits is the fragment expression we're appending to.
|
@swift-ci please test |
Two new tests were added in swiftlang#66448, and they both fail for watchOS: 1. debug_fragment_merge.sil fails on 32 bit architectures because the offset calculation is different for those (fragments of 32 bits, instead of 64). 2. debug_fragment_merge.swift is failing for unknown reasons at this point, there is simply no SIL debug information generated for the variable "data". Since the original patch didn't change SILGen, this is not a regression.
SIL variables can be split by SILSROA into separate allocations, each having op_fragment expressions in debug_value (VarInfo.DIExpr). These allocations can be further split by IRGen (multiple values in Storage argument).
These "nested" fragments refer to the same DI variable, so it is important to merge them for the LLVM IR DI expression. The compiler used to ignore fragment expressions from SIL when IRGen fragments were also present. This led to incorrect DI info generation, and for some cases even triggered assertions in LLVM X86 CodeGen:
DwarfExpression.cpp:679: void llvm::DwarfExpression::addFragmentOffset(const
llvm::DIExpression *): Assertion `FragmentOffset >= OffsetInBits &&
"overlapping or duplicate fragments"' failed.
The patch resolves #64642. The LIT test is a reduced reproducer from that issue.