[LoadableByAddress] Fix debug info holes when expanding alloc_stack. #15575
Conversation
dcci
force-pushed
the
loadable-debuginfo
branch
from
862a9c6 to
64e8189
Compare
dcci
changed the title
|
@adrian-prantl now with the correct patch, after coffee. |
lib/IRGen/LoadableByAddress.cpp
Outdated
There was a problem hiding this comment.
This looks like it could be a range-based-for.
lib/IRGen/LoadableByAddress.cpp
Outdated
There was a problem hiding this comment.
I think this utility would make sense in SILBasicBlock.h or even as a member function of SILBasicBlock. It's going to be useful for other passes that have similar issues, too. Perhaps call it getScopeOfFirstNonMetaInstruction.
lib/IRGen/LoadableByAddress.cpp
Outdated
There was a problem hiding this comment.
Wouldn't it be better to add a new constructor to SILBuilderWithScope that has this behavior?
/// Creates a new SILBuilder with an insertion point at the beginning of BB and the debug scope from the first non-metainstruction in the BB.
explicit SILBuilderWithScope(SILBasicBlock *BB) {}
lib/IRGen/LoadableByAddress.cpp
Outdated
There was a problem hiding this comment.
Let's rename isMaintenanceInst to isMetaInstruction. This is the same name we are using in MIR.
dcci
force-pushed
the
loadable-debuginfo
branch
from
64e8189 to
0c43de8
Compare
|
@swift-ci please test |
|
@adrian-prantl addressed all your comments. Please take another look. |
|
Build failed |
|
Build failed |
include/swift/SIL/DebugUtils.h
Outdated
There was a problem hiding this comment.
Can you make this a method of SILInstruction?
I also believe that every SILInstruction that implements getVarInfo() should be included in this list (DebugValueAddr, AllocBox, ...) .
|
Sure! |
dcci
changed the title
<rdar://problem/36876376>
dcci
force-pushed
the
loadable-debuginfo
branch
from
0c43de8 to
dfaf133
Compare
|
Done. I think as a follow up I'm going to move |
|
@swift-ci please test and merge |
1 similar comment
|
@swift-ci please test and merge |
This function is clearly returning the opposite of what its name says:
```
const SILDebugScope *SILBasicBlock::getScopeOfFirstNonMetaInstruction() {
for (auto &Inst : *this)
if (Inst.isMetaInstruction())
return Inst.getDebugScope();
return begin()->getDebugScope();
}
```
Looking at the PR history (sadly GH doesn't preserve old versions of the patch...)
swiftlang#15575
There was this snippet of code:
```
// Find the correct debug scope for alloc stack. We want to give to the
// expanded sequence the correct debug scope so we skip over instructions
// that aren't lowered to anything real (e.g. debug_value).
static const SILDebugScope *findAllocStackDebugScope(SILBasicBlock &BB) {
auto It = BB.begin();
while (It != BB.end()) {
if (!isMaintenanceInst(&*It))
```
We don't know what used to be after that line but, based on the comments, it
must have been a `return It->getDebugScope()`.
Adrian then asked the author to make this a different helper function
`getScopeOfFirstNonMetaInstruction`, and the subsequence force-push had the code
we see today. So maybe it was in this conversion that the author made the
mistake?
Fixing the implementation doesn't cause any tests to fail (sadly the original PR
did not add any SIL->SIL tests, which would have been ideal).
This function is clearly returning the opposite of what its name says:
```
const SILDebugScope *SILBasicBlock::getScopeOfFirstNonMetaInstruction() {
for (auto &Inst : *this)
if (Inst.isMetaInstruction())
return Inst.getDebugScope();
return begin()->getDebugScope();
}
```
Looking at the PR history (sadly GH doesn't preserve old versions of the patch...)
swiftlang#15575
There was this snippet of code:
```
// Find the correct debug scope for alloc stack. We want to give to the
// expanded sequence the correct debug scope so we skip over instructions
// that aren't lowered to anything real (e.g. debug_value).
static const SILDebugScope *findAllocStackDebugScope(SILBasicBlock &BB) {
auto It = BB.begin();
while (It != BB.end()) {
if (!isMaintenanceInst(&*It))
```
We don't know what used to be after that line but, based on the comments, it
must have been a `return It->getDebugScope()`.
Adrian then asked the author to make this a different helper function
`getScopeOfFirstNonMetaInstruction`, and the subsequence force-push had the code
we see today. So maybe it was in this conversion that the author made the
mistake?
Fixing the implementation doesn't cause any tests to fail (sadly the original PR
did not add any SIL->SIL tests, which would have been ideal).
3 participants
We should also skip debug info as they don't really are lowered
to anything real. Add an helper so that we can use it in passes
as well.
@adrian-prantl I'm dealing with a bot failure, but I wanted to put this online so you can take a look and let me know if this is fine. I'm going to add a test & run the tests.
This is the last failure we have before enabling the verifier for debug info, so my plan is to flip the switch today (crossing fingers).