TealDbg: Support for StepOver and refactoring object IDs #3653
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3653 +/- ##
==========================================
- Coverage 49.99% 49.99% -0.01%
==========================================
Files 392 392
Lines 68495 68579 +84
==========================================
+ Hits 34242 34283 +41
- Misses 30497 30537 +40
- Partials 3756 3759 +3
Continue to review full report at Codecov.
|
algochoi
changed the title
cmd/tealdbg/debugger.go
Outdated
| defer s.mu.Unlock() | ||
| // Step over a function call (callsub op). | ||
| if currentOp == "callsub" { | ||
| err := s.setBreakpoint(currentLine + 1) |
There was a problem hiding this comment.
I think the only time it would error would be if callsub was at the end of the program and setBreakpoint() will try to set a breakpoint past the program. In that case, it feels normal to not log something and just continue, but maybe I'm missing a case.
| lines := strings.Split(d.Disassembly, "\n") | ||
| for _, pc := range callstack { | ||
| // The callsub is pc - 3 from the callstack pc | ||
| callsubLineNum := d.PCToLine(pc - 3) |
There was a problem hiding this comment.
I did a bit of hardcoding here (callsub saves pc+3 to the callstack in eval.go) and added a "soft" check to see if the line is a callsub later on so we can try to parse the label to make the Call Stack look nicer in CDT.
| NoBreak bool `json:"nobreak"` | ||
| StepBreak bool `json:"stepbreak"` | ||
| StepOutOver bool `json:"stepover"` |
There was a problem hiding this comment.
nit: It looks like not all the combinations are valid (for example NoBreak=true and StepBreak=true). This is an indication these might be flags and not boolean variables
There was a problem hiding this comment.
@algorandskiy I have a similar feeling though I'm wondering if you feel strongly enough that we ought to hold the PR or if we can accept as is and revisit in a future iteration?
From my side - Given the ground already covered here, I feel like it'd be OK to accept as is.
| // setActiveBreak does not check if the line is a valid value, so it should | ||
| // be called inside the setBreakpoint() in session. | ||
| func (dc *debugConfig) setActiveBreak(line int) { | ||
| dc.ActiveBreak[line] = struct{}{} |
There was a problem hiding this comment.
Andrew, I might have already asked but can't find the answer - what is the point of having ActiveBreak map? It is only set in setActiveBreak. And everywhere we call setBreackpoint the contol state is reset s.debugConfig = makeDebugConfig() leaving only a single entry in ActiveBreak.
There was a problem hiding this comment.
It's only used in the Resume() to set multiple active breakpoints (in case we jump backwards), but you're right that in everywhere else there is only 1 active break.
…_split_test_cases Refactor TestCallStackControl into subtests
|
I've manually tested on the following recursive TEAL program, and step in and step over are working great! My only complaint is that step out is frequently buggy and takes me to the end of the program, though I've not yet debugged why this happens. |
There was a problem hiding this comment.
@algochoi Thanks for your patience working through multiple iterations to enrich tealdbg. ☕
| return nil | ||
| } | ||
|
|
||
| func (s *session) SetBreakpoint(line int) error { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| // Reset all existing flags and breakpoints and set a new bp. |
There was a problem hiding this comment.
why moved the resetting s.debugConfig from SetBreakpoint to setBreakpoint?
There was a problem hiding this comment.
Good catch - I will move this back into setBreakpoint
There was a problem hiding this comment.
Actually, for Resume there can be multiple active breakpoints (code execution can jump backward and break on a previous bp) set so I moved this outside instead - let me know if that makes sense.
| @@ -174,3 +174,27 @@ func TestValueDeltaToValueDelta(t *testing.T) { | |||
| require.Equal(t, base64.StdEncoding.EncodeToString([]byte(vDelta.Bytes)), ans.Bytes) | |||
| require.Equal(t, vDelta.Uint, ans.Uint) | |||
| } | |||
|
|
|||
| func TestParseCallstack(t *testing.T) { | |||
There was a problem hiding this comment.
add a real test for with two nested callstack received from the evaluator via debugger.Update.
There was a problem hiding this comment.
Good point - added unit test below (real test checks line+1 due to pragma)
Summary
This PR contains:
callsubop. Dynamically inspects the callstack to make sure depth is equal to what it entered as. Also added a channel so that the updates can be synchronized within the debugger control (else updates may be received at any time and the state may not be updated before the depth check happens).RunAllin tealdbg to ensure proper group txn execution.This should also close #2289
Test Plan
local_test.goanddebugger_test.go./tealdbg debug -d path-to-file/generated-data/dryrun_subroutine.msgpgloadseems to work in tealdbg if the entire txn group is supplied. e.g. through dryrun:tealdbg debug -d dryrun_grp.msgp.