TealDbg: Support for StepOver and refactoring object IDs #3653

algochoi · 2022-02-17T22:24:54Z

Summary

This PR contains:

Support for stepping over subroutines when there is a callsub op. 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).
Refactors object IDs in a separate file. We may want to map out the actual addresses to objects in the future.
Adds a unit test for RunAll in tealdbg to ensure proper group txn execution.
Include and expose the call stack in CDT.

This should also close #2289

Test Plan

Unit tests in local_test.go and debugger_test.go
Ran tealdbg on contracts with subroutines. Example dryrun here, and can be run with ./tealdbg debug -d path-to-file/generated-data/dryrun_subroutine.msgp

gload seems to work in tealdbg if the entire txn group is supplied. e.g. through dryrun: tealdbg debug -d dryrun_grp.msgp.

codecov-commenter · 2022-02-17T23:49:17Z

Codecov Report

Merging #3653 (f42f312) into master (32cfc42) will decrease coverage by 0.00%.
The diff coverage is 71.79%.

@@            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

Impacted Files	Coverage Δ
cmd/tealdbg/cdtState.go	`84.47% <0.00%> (-0.15%)`	⬇️
cmd/tealdbg/webdbg.go	`57.79% <0.00%> (-4.71%)`	⬇️
cmd/tealdbg/cdtSession.go	`66.74% <57.57%> (-1.14%)`	⬇️
cmd/tealdbg/debugger.go	`73.60% <86.88%> (+2.17%)`	⬆️
data/transactions/logic/debugger.go	`61.46% <100.00%> (+4.76%)`	⬆️
ledger/blockqueue.go	`82.18% <0.00%> (-2.88%)`	⬇️
network/wsPeer.go	`65.83% <0.00%> (-2.78%)`	⬇️
data/abi/abi_type.go	`87.67% <0.00%> (-0.95%)`	⬇️
ledger/tracker.go	`70.81% <0.00%> (-0.86%)`	⬇️
ledger/acctupdates.go	`68.51% <0.00%> (-0.66%)`	⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32cfc42...f42f312. Read the comment docs.

…ped txns

algorandskiy

Looks good, couple comments

algorandskiy · 2022-03-11T16:58:16Z

cmd/tealdbg/debugger.go

+		defer s.mu.Unlock()
+		// Step over a function call (callsub op).
+		if currentOp == "callsub" {
+			err := s.setBreakpoint(currentLine + 1)


log the error?

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.

cmd/tealdbg/local_test.go

cmd/tealdbg/debugger.go

cmd/tealdbg/local.go

algochoi · 2022-03-16T18:39:23Z

data/transactions/logic/debugger.go

+	lines := strings.Split(d.Disassembly, "\n")
+	for _, pc := range callstack {
+		// The callsub is pc - 3 from the callstack pc
+		callsubLineNum := d.PCToLine(pc - 3)


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.

data/transactions/logic/debugger.go

cmd/tealdbg/local_test.go

algorandskiy · 2022-03-25T19:27:15Z

cmd/tealdbg/debugger.go

+	NoBreak     bool `json:"nobreak"`
+	StepBreak   bool `json:"stepbreak"`
+	StepOutOver bool `json:"stepover"`


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

@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.

algorandskiy · 2022-03-25T19:34:01Z

cmd/tealdbg/debugger.go

+// 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{}{}


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.

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

jasonpaulos · 2022-03-30T21:32:21Z

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.

#pragma version 6
int 5
callsub factorial
return

factorial:
dup
int 1
<=
bz recursive_step
pop
int 1
retsub
recursive_step:
dup
int 1
-
callsub factorial
*
retsub

cmd/tealdbg/debugger.go

…routine

michaeldiamant

@algochoi Thanks for your patience working through multiple iterations to enrich tealdbg. ☕

michaeldiamant

Re-approving after minor change dismissed my prior approval. Still looks good to me.

algorandskiy · 2022-04-11T18:07:22Z

cmd/tealdbg/debugger.go

 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.


why moved the resetting s.debugConfig from SetBreakpoint to setBreakpoint?

Good catch - I will move this back into setBreakpoint

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.

algorandskiy · 2022-04-11T18:15:20Z

data/transactions/logic/debugger_test.go

 	require.Equal(t, vDelta.Uint, ans.Uint)
 }
+
+func TestParseCallstack(t *testing.T) {


add a real test for with two nested callstack received from the evaluator via debugger.Update.

Good point - added unit test below (real test checks line+1 due to pragma)

algochoi added 4 commits February 17, 2022 17:20

Add stepover function for subroutines

bb40245

Move object ID strings to cdtStateObjects.go

e585a7a

Run make sanity

d015136

Use error return for setBreakpoint

852e71e

algochoi added 2 commits February 18, 2022 09:04

Revert accidental change in local state

d939d21

Add better checks to gloadImpl and add a unit test for runAll on grou…

3a55617

…ped txns

algochoi added the Bug-Fix label Feb 23, 2022

algochoi requested review from algorandskiy and jannotti February 23, 2022 18:09

Revert gloadImpl check

07716aa

algochoi added Enhancement and removed Bug-Fix labels Feb 23, 2022

algochoi changed the title ~~TealDbg support for StepOver and refactoring object IDs~~ TealDbg: Support for StepOver and refactoring object IDs Feb 23, 2022

michaeldiamant added the Team Scytale label Mar 10, 2022

algochoi marked this pull request as ready for review March 10, 2022 21:29

algorandskiy reviewed Mar 11, 2022

View reviewed changes

jasonpaulos reviewed Mar 11, 2022

View reviewed changes

cmd/tealdbg/debugger.go Outdated Show resolved Hide resolved

algochoi added 3 commits March 11, 2022 17:13

Merge branch 'master' into algochoi/tealdbg-subroutine

fbfd338

Add program source to tests

9c21330

Add support for call stack on cdt tealdbg

89ecb51

michaeldiamant mentioned this pull request Mar 16, 2022

Merge #3612 into #3653 algochoi/go-algorand#1

Closed

Merge branch 'master' into algochoi/tealdbg-subroutine

c25ba18

algochoi commented Mar 16, 2022

View reviewed changes

cmd/tealdbg/local.go Show resolved Hide resolved

algochoi commented Mar 16, 2022

View reviewed changes

michaeldiamant reviewed Mar 16, 2022

View reviewed changes

data/transactions/logic/debugger.go Show resolved Hide resolved

michaeldiamant reviewed Mar 16, 2022

View reviewed changes

cmd/tealdbg/local_test.go Show resolved Hide resolved

michaeldiamant reviewed Mar 16, 2022

View reviewed changes

cmd/tealdbg/local_test.go Show resolved Hide resolved

algochoi added 2 commits March 16, 2022 19:24

Add parsing test for call stack

c60ed66

Revert a mistake in the test

8cb9870

algochoi added 2 commits March 24, 2022 20:17

Add line check inside the lock

e46f1e3

Minor fixes to testS

f42f312

algorandskiy reviewed Mar 25, 2022

View reviewed changes

michaeldiamant and others added 3 commits March 25, 2022 20:54

Refactor TestCallStackControl into subtests

8717637

Merge pull request #2 from michaeldiamant/algochoi/tealdbg-subroutine…

4ce49dc

…_split_test_cases Refactor TestCallStackControl into subtests

Run gofmt

8307899

michaeldiamant mentioned this pull request Mar 31, 2022

tealdbg: Enforce critical regions for concurrent data access #3850

Open

michaeldiamant reviewed Mar 31, 2022

View reviewed changes

cmd/tealdbg/debugger.go Outdated Show resolved Hide resolved

algochoi added 2 commits March 31, 2022 12:07

Fix stepout semantics and pause debugger only when we are outside sub…

24bdfa5

…routine

Fix completed typo

fa51fff

michaeldiamant previously approved these changes Mar 31, 2022

View reviewed changes

Finish fixing typo

f616437

algochoi dismissed michaeldiamant’s stale review via f616437 March 31, 2022 19:16

michaeldiamant previously approved these changes Apr 1, 2022

View reviewed changes

algorandskiy reviewed Apr 11, 2022

View reviewed changes

algochoi added 2 commits April 11, 2022 21:29

Merge branch 'master' into algochoi/tealdbg-subroutine

7634b63

Add new test for debugger hook

1c9ed1a

algochoi dismissed michaeldiamant’s stale review via 1c9ed1a April 12, 2022 13:08

algochoi requested a review from algorandskiy April 12, 2022 13:26

algorandskiy approved these changes Apr 13, 2022

View reviewed changes

michaeldiamant merged commit b0c551a into algorand:master Apr 13, 2022

egieseke mentioned this pull request Apr 22, 2022

go-algorand 3.6.0-beta Release PR #3909

Closed

Algo-devops-service mentioned this pull request Apr 26, 2022

go-algorand 0.0.0-beta Release PR #3922

Closed

This was referenced Apr 27, 2022

go-algorand 3.6.0-beta Release PR #3923

Merged

go-algorand 3.6.0-beta Release PR #3934

Merged

This was referenced May 2, 2022

go-algorand 3.6.1-beta Release PR #3943

Merged

go-algorand 3.6.2-beta Release PR #3947

Merged

Algo-devops-service mentioned this pull request May 7, 2022

go-algorand 3.6.2-stable Release PR #3960

Merged

algochoi deleted the algochoi/tealdbg-subroutine branch January 12, 2023 16:02

TealDbg: Support for StepOver and refactoring object IDs #3653

TealDbg: Support for StepOver and refactoring object IDs #3653

Uh oh!

Conversation

algochoi commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

codecov-commenter commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

algorandskiy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

algochoi Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonpaulos commented Mar 30, 2022

Uh oh!

Uh oh!

michaeldiamant left a comment

Choose a reason for hiding this comment

Uh oh!

michaeldiamant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

algochoi commented Feb 17, 2022 •

edited

Loading

codecov-commenter commented Feb 17, 2022 •

edited

Loading

algochoi Mar 16, 2022 •

edited

Loading