service/dap: support evaluate requests with expressions and calls #2185

polinasok · 2020-09-28T09:53:31Z

updates #1515

…ring merge

service/dap/server.go

aarzilli · 2020-09-28T12:03:53Z

service/dap/server.go

+			s.sendErrorResponseWithOpts(request.Request, UnableToEvaluateExpression, "Unable to evaluate expression", err.Error(), showErrorToUser)
+			return
+		}
+		retVars := s.debugger.ReturnValues(&prcCfg)


The problem with this is that there's no guarantee that the return values for the call will be available the very next time the debugger stops. It could be that the debugger stopped because it encountered a breakpoint, either inside the function that was called or some other breakpoint in a different goroutine.

I'm not sure what the right way to handle this would be, I think it depends on what the UI of VSCode can do. For example what happens when using VSCode with gdb, doing a function call while a breakpoint is set in the function being called?

I added a draft implementation. See TODO please - I am not fully sure what the cleanest way is to detect if the call completed or not.

Also, I added a unittest for a breakpoint inside of the call. I still need to add a test where the call restarts other goroutines that stop on a breakpoint. In the meantime, I tested this manually with https://github.com/aarzilli/delve_client_testing/blob/master/05nextbp/main.go. It works when one-off calls are issued from the debug console. But if we put a call into a watch panel, then everything gets totally screwed up. The call restarts everything, so before it finishes, another goroutine stops. That triggers watch re-eval, but relative to another goroutine, so it is allowed. This again restarts everything, jumping from goroutine to goroutine, and just keeps going like continue, never really stopping at any breakpoints for longer than a split second.

Should we be disallowing calls as watch expressions to avoid such circular behavior? That would be unfortunate because it could be such a neat way to pretty print most interesting variables in the watch panel and have them automatically update.

See TODO please - I am not fully sure what the cleanest way is to detect if the call completed or not.

You should iterate over the thread list and check if one has the goroutine ID you want and ReturnValues != nil. I don't think the way you are doing this works. First off there's no guarantee that the injected goroutine will make progress before we are interrupted by a simultaneous breakpoint, secondly consider what happens if the goroutine stopped on a breakpoint inside main.f and the user injects a call to main.g which calls main.f again.

Should we be disallowing calls as watch expressions to avoid such circular behavior? That would be unfortunate because it could be such a neat way to pretty print most interesting variables in the watch panel and have them automatically update.

IMHO there's no way to do it cleanly. I'd like to know what VSCode's solution is for C-like languages, maybe there's something I haven't thought of.

there's no guarantee that the injected goroutine will make progress before we are interrupted by a simultaneous breakpoint

It that case won't the injected goroutine no longer be the selected goroutine, even if its position hasn't changed? Won't the selected goroutine match that of the simultaneous breakpoint, which would be different from the goroutine where the call got injected?

Let's say I do loop over all the threads and look for the one with the goroutine id I am interested in and it has non-nil values. In what case, would this not be the selected goroutine? After the injection, are we not guaranteed to go back to the original point on the original goroutine, making it selected again?

Can the goroutine with the injection end up in a different stopped state that does have return values? (Maybe if it stops right after some function call inside of the injection? Does that only happened if we step out?)

consider what happens if the goroutine stopped on a breakpoint inside main.f and the user injects a call to main.g which calls main.f again

Right, good point. Added test and handling for this. Except, I am calling "f" when stopped in "f".

aarzilli · 2020-09-28T12:04:25Z

service/dap/server.go

+			return
+		}
+		retVars := s.debugger.ReturnValues(&prcCfg)
+		if retVars != nil && len(retVars) > 0 {


See DeepSource error.

service/dap/server_test.go

aarzilli · 2020-10-25T14:32:22Z

PS. see the failure of TestScopesAndVariables on windows/go1.15. I think you need to relax that test, it keeps going back and forth.

polinasok · 2020-10-30T17:41:18Z

Don't take a look just yet. I am investigating the failure (everything did pass for me locally). And I still need to update the code to loop over threads like we agreed.

aarzilli · 2020-11-02T13:10:50Z

I think travis is running the tests because of the merge conflict. If you don't know how to get the full log of cirrus-ci, it's here: https://cirrus-ci.com/task/6100324710612992.

polinasok · 2020-11-02T16:46:20Z

It took me a moment to figure out, but I did eventually get to the log by clicking some obscure link on the bottom. Let's see what things look like after the merge.

polinasok · 2020-11-03T10:47:48Z

I think the proc test failures are unrelated.

aarzilli · 2020-11-03T12:23:35Z

service/dap/server.go

+					s.sendErrorResponseWithOpts(request.Request, UnableToEvaluateExpression, "Unable to evaluate expression", err.Error(), showErrorToUser)
+					return
+				}
+				retVars = proct.Common().ReturnValues(prcCfg)


There should be a s.debugger.LockTarget() / s.debugger.UnlockTarget() around this. Also at this point we can exit the loop early with break.

aarzilli · 2020-11-03T12:25:24Z

service/debugger/debugger.go

@@ -1518,6 +1518,16 @@ func (d *Debugger) Recorded() (recorded bool, tracedir string) {
 	return d.target.Recorded()
 }

+// ReturnValues returns the return values of the function we just stepped out of.


No longer used?

Reused for ReturnValues of a given thread.

aarzilli · 2020-11-03T12:27:51Z

service/dap/server.go

+			if t.GoroutineID != stateBeforeCall.SelectedGoroutine.ID {
+				continue
+			}
+			if t.Line == stateBeforeCall.SelectedGoroutine.CurrentLoc.Line && t.ReturnValues != nil {


This if and the one above can be fused into a single one

if t.GoroutineID == stateBeforeCall.SelectedGoroutine.ID && t.Line == stateBeforeCall.SelectedGoroutine.CurrentLoc.Line && t.ReturnValues != nil { ...```

aarzilli

LGTM

…-delve#2185) * Support evaluate request * Fix failing tests * Call support * Remove debugger.CurrentThread() that got accidentally reintroduced during merge * Address review comments * Function to stringify stop reason * Add resetHandlesForStop * Handle stop inside call * More tests * Address review comments * Check all threads to determine if call completed * Fix test * Fix test * Fix test * Address review comments Co-authored-by: Polina Sokolova <polinasok@users.noreply.github.com>

polinasok added 5 commits September 17, 2020 13:29

Support evaluate request

a73946d

Fix failing tests

8e3a374

Call support

9d74a4f

Sync with master

c4ecafb

Remove debugger.CurrentThread() that got accidentally reintroduced du…

ab96172

…ring merge

aarzilli reviewed Sep 28, 2020

View reviewed changes

polinasok added 7 commits October 21, 2020 18:10

Merge master

a60faa2

Address review comments

6380bbb

Merge master

b1de461

Function to stringify stop reason

aff60ec

Add resetHandlesForStop

23851ce

Handle stop inside call

558a478

More tests

b430ae0

Address review comments

48ce3e5

Check all threads to determine if call completed

53c1441

Sync to head

09ee192

polinasok added 3 commits November 3, 2020 00:54

Fix test

afc15bb

Fix test

57870d5

Fix test

da71246

aarzilli reviewed Nov 3, 2020

View reviewed changes

Address review comments

122606f

aarzilli approved these changes Nov 4, 2020

View reviewed changes

derekparker merged commit c94db60 into go-delve:master Nov 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

service/dap: support evaluate requests with expressions and calls #2185

service/dap: support evaluate requests with expressions and calls #2185

polinasok commented Sep 28, 2020

aarzilli Sep 28, 2020

polinasok Oct 23, 2020 •

edited

Loading

aarzilli Oct 25, 2020

polinasok Oct 30, 2020

aarzilli Sep 28, 2020

polinasok Sep 30, 2020

aarzilli commented Oct 25, 2020

polinasok commented Oct 30, 2020

aarzilli commented Nov 2, 2020

polinasok commented Nov 2, 2020

polinasok commented Nov 3, 2020

aarzilli Nov 3, 2020

polinasok Nov 3, 2020

aarzilli Nov 3, 2020

polinasok Nov 3, 2020

aarzilli Nov 3, 2020

polinasok Nov 3, 2020

aarzilli left a comment

service/dap: support evaluate requests with expressions and calls #2185

service/dap: support evaluate requests with expressions and calls #2185

Conversation

polinasok commented Sep 28, 2020

Choose a reason for hiding this comment

polinasok Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarzilli commented Oct 25, 2020

polinasok commented Oct 30, 2020

aarzilli commented Nov 2, 2020

polinasok commented Nov 2, 2020

polinasok commented Nov 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarzilli left a comment

Choose a reason for hiding this comment

polinasok Oct 23, 2020 •

edited

Loading