[DebugInfo] Fix getScopeOfFirstNonMetaInstruction #70882
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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).
|
preset=buildbot,tools=RA,stdlib=DA |
|
@adrian-prantl what do you think? |
|
Discussed offline, we are ok merging this as is. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This function is clearly returning the opposite of what its name says:
Looking at the PR history (sadly GH doesn't preserve old versions of the patch...) #15575
There was this snippet of code:
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).