[M] Change in dart/sdk[main]: [vm] Change how the debugger skips the same position after breakpoints.
Tess Strickland (Gerrit)
Tess Strickland added 1 comment![]( "Open in Gerrit")
Going ahead and sending for you to look at. There is one failure on ARM64 but not on X64, interestingly enough, and it happens with both compiled code and bytecode, which is also interesting as I thought it'd be only compiled code as I'd assume it was due to differences in how compiled code is generated on those two architectures. (At least I assume it's ARM64 vs. X64 since the failures were on Mac ARM64 and the successes were on Linux X64/Windows X64, so also throwing Windows ARM64 at it to see if that succeeds or fails.)
I'll look into it, but in the meantime I think it's worth a pass to see if you have any comments about its current state.
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov voted Code-Review+1![]( "Open in Gerrit")
- Tess Strickland
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tess Strickland (Gerrit)
Tess Strickland added 1 comment![]( "Open in Gerrit")
Going ahead and sending for you to look at. There is one failure on ARM64 but not on X64, interestingly enough, and it happens with both compiled code and bytecode, which is also interesting as I thought it'd be only compiled code as I'd assume it was due to differences in how compiled code is generated on those two architectures. (At least I assume it's ARM64 vs. X64 since the failures were on Mac ARM64 and the successes were on Linux X64/Windows X64, so also throwing Windows ARM64 at it to see if that succeeds or fails.)
I'll look into it, but in the meantime I think it's worth a pass to see if you have any comments about its current state.
For the record, this was coincidence; the real problem is that the last stepping information was set from the breakpoint location after handling the stepping request, but the breakpoint was disabled before the next stepping request which caused a race condition where the breakpoint location may or may not might be invalidated when the last stepping information was set.
That means we need to set the last stepping information _before_ calling `HandleSteppingRequest`, which now we do. This means no clearing the last stepping information as a side effect of handling a stepping request, as it needs to be kept across that call, which was the case previously and fine to go back to.
Related details
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tess Strickland (Gerrit)
Tess Strickland voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
23 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: runtime/vm/debugger.cc
Insertions: 14, Deletions: 6.
The diff is too large to show. Please review the diff.
```
```
The name of the file: runtime/vm/debugger.h
Insertions: 1, Deletions: 0.
The diff is too large to show. Please review the diff.
```
Change information
[vm] Change how the debugger skips the same position after breakpoints.
Previously, the debugger used a boolean "skip next step" field that was
set when a breakpoint was hit. In compiled code, not skipping the next
step would cause a redundant pause to be emitted after a breakpoint, and
so that field caused the next run of PauseStepping to reset it to false
and then continue single stepping without the redundant pause.
However, skipping only the next call to PauseStepping isn't enough
when debugging bytecode. For bytecode, single stepping is performed on
each bytecode instruction and so there may be multiple instructions after a breakpoint until a new token position is reached.
Instead, remove the field and generalize the case being avoided by
recording the token position that should be ignored and single
stepping until the token position changes. This is done by recording
a real last_stepping_pos_ with a last_stepping_fp_ of 0, which now
means to skip any possible pauses until the token position changes.
Doing this means that in each case where there are possible skips to
be performed (skipping the same fp/pos after a pause, skipping the
pos after a breakpoint, skipping the await fp/pos after a resumption),
all such skips are handled via a single unified mechanism.
This CL also reworks the kStepOut behavior so that it steps into
the awaiter closest to the highest debuggable frame on the stack if
there is one, not just the first awaiter even if there are no
debuggable frames between that awaiter and the next.
TEST=pkg/vm_service/test/vm_timeline_flags
- M pkg/vm_service/test/vm_timeline_flags_test.dart
- M runtime/vm/debugger.cc
- M runtime/vm/debugger.h
Code-Review: +1 by Alexander Markov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |