Skip to content

Conversation

@felipepiovezan
Copy link
Contributor

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...) #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).
@felipepiovezan
Copy link
Contributor Author

preset=buildbot,tools=RA,stdlib=DA
@swift-ci please test

@felipepiovezan
Copy link
Contributor Author

@adrian-prantl what do you think?

@felipepiovezan
Copy link
Contributor Author

Discussed offline, we are ok merging this as is.

@felipepiovezan felipepiovezan merged commit d0344e5 into swiftlang:main Jan 17, 2024
@felipepiovezan felipepiovezan deleted the felipe/fix_getscopeoffirstnonmeta branch January 17, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant