Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

service/dap: support auto-loading of unloaded interfaces #2362

Merged
merged 5 commits into from
Mar 8, 2021

Conversation

polinasok
Copy link
Collaborator

@polinasok polinasok commented Feb 25, 2021

Updates #1515

While we use fully qualified names for evaluateName (added in #2292) for readability, the auto-loading expressions are based on ClientHowto.md#loading-more-of-a-variable. These expressions work in a context of any frame.

Copy link
Member

@aarzilli aarzilli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the onEvaluateRequest can be called on stack frames that aren't the topmost frame of the current goroutine. AFAICT using qualifiedNameOrExpr with those, instead of the pointer expression, will not work.

@polinasok
Copy link
Collaborator Author

I see that the onEvaluateRequest can be called on stack frames that aren't the topmost frame of the current goroutine. AFAICT using qualifiedNameOrExpr with those, instead of the pointer expression, will not work.

Evaluate requests are not triggered by variable loading (which is what happens when you expand layers in VARIABLES pane), but by WATCH expressions and by manual expressions in DEBUG CONSOLE. These can happen relative to a non-topmost frame if a user highlights a different stackframe in the UI. Here is an illustration of this behavior with some creative snapshotting.

image

This is actually making me realize that variable loading should not be done with fully qualified names after all because it needs to work no matter what frame the users chooses to view. I will update this PR to use the "magic" expression.

However, I think evaluateName used to add variables to WATCH pane should stay as names between a list of readable and easily identifiable names and a list of cryptic and look-alike expressions with addresses in the WATCH pane, an average user would prefer the names. It does mean that those watch variables will only be useful when looking at their frame and not while debugging the rest of the code. But if the truly frame-less watching is needed, the user can enter the expression manually, can't they?

@aarzilli
Copy link
Member

However, I think evaluateName used to add variables to WATCH pane should stay as names between a list of readable and easily identifiable names and a list of cryptic and look-alike expressions with addresses in the WATCH pane, an average user would prefer the names. It does mean that those watch variables will only be useful when looking at their frame and not while debugging the rest of the code. But if the truly frame-less watching is needed, the user can enter the expression manually, can't they?

I agree with this.

@polinasok
Copy link
Collaborator Author

polinasok commented Feb 26, 2021

However, I think evaluateName used to add variables to WATCH pane should stay as names between a list of readable and easily identifiable names and a list of cryptic and look-alike expressions with addresses in the WATCH pane, an average user would prefer the names. It does mean that those watch variables will only be useful when looking at their frame and not while debugging the rest of the code. But if the truly frame-less watching is needed, the user can enter the expression manually, can't they?

I agree with this.

Great. I was actually thinking that it would be nice if users had a shortcut to get these expressions for that case (with good documentation to advertise). And then I realized that the shortcut is kind of there with dlv cli if you do the following:

(dlv) p aas  ### same as p *(&aas)
[]main.a len: 1, cap: 1, [
	{
		aas: []main.a len: 1, cap: 1, [
			(*main.a)(0xc00000c0e0),
		],},
]
(dlv) p &aas  ### This gives you the address expression
(*[]main.a)(0xc000107390)  ### Value not loaded
### Dereference that expression to get to load the variable without the name
(dlv) p *(*[]main.a)(0xc000107390)
[]main.a len: 1, cap: 1, [
	{
		aas: []main.a len: 1, cap: 1, [
			(*main.a)(0xc00000c0e0),
		],},
]

But in DAP, based on legacy adapter, which is likely based on other adapters, we use a different syntax with <>. I used to think it was just cosmetic, but now realize that I should change it to what dlv cli prints because it can actually be used in expressions.

I will do this in a separate PR.

@polinasok
Copy link
Collaborator Author

Some of the check failures look related to my changes (although in some odd ways - e.g. DeepSource). I am still figuring out how to correct.

@polinasok
Copy link
Collaborator Author

DeepSource is complaining that main is redeclared in testvariables.go. I think that's because all fixtures use package main and have a main function. I guess it is detecting this because the files has been touched in this PR?

Copy link
Member

@aarzilli aarzilli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Unfortunately DeepSource started giving too many false positives lately. In particular it was instructed to ignore the _fixtures directory but refuses to do so.

@derekparker derekparker merged commit 90fb0a5 into go-delve:master Mar 8, 2021
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.

3 participants