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

Allow display of local variables in frames multiple levels deep. #345

Merged
merged 1 commit into from
Apr 26, 2022

Conversation

brownts
Copy link
Collaborator

@brownts brownts commented Apr 22, 2022

Fixes #342.

Remove the arbitrary separation between the variable references used
to represent scopes and those used to represent variable objects.
This implementation uses a common pool of handles to support both
variable object references as well as scope references, thus
eliminating the previous bounded numerical range used for scopes,
which was not large enough to represent stack frames multiple levels
deep, due to thread and level information being encoded in the scope
variable reference value.

@brownts brownts force-pushed the bugfix/stack_frame_locals branch from 4c95251 to ff40395 Compare April 22, 2022 13:00
Copy link
Collaborator

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

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

Looks good to me in general. Things I'm not sure about:

  1. Shouldn't there be more than the "Local" scope?
  2. Is it possible to use the scope from the DAP here instead of an own class?

@GitMensch
Copy link
Collaborator

See #242 for how to acquire variables for the Register scope, which would be a possible follow-up to this change.

@brownts
Copy link
Collaborator Author

brownts commented Apr 24, 2022

  1. Shouldn't there be more than the "Local" scope?

The current implementation only supports the "Local" scope. Additional scopes could be added, but this PR is to address a bug in the current implementation. I'd rather keep bug fixes and new functionality separate, from a semantic versioning perspective, as bug fixes can go into patch releases (e.g., 0.26.1), but new functionality should go into a minor release (e.g., 0.27.0). It seems there are other open issues (e.g., #214) to capture adding an additional scope.

  1. Is it possible to use the scope from the DAP here instead of an own class?

I assume you are referring to the addition of the VariableScope class. That class is used to map the scope name, debugger thread and stack frame level to the corresponding variablesReference value that is supplied in the DAP Scope response (see return value of MI2DebugSession::scopesRequest::createScope for how this information is used to map to the variablesReference supplied in the DAP Scope response). The previous implementation attempted to encode the debugger's thread and level directly into the variablesReference in the DAP Scope response, but when stack levels were deep enough the encoded value clashed with the variablesReference numerical space used to represent variables.

So, to answer your question, the DAP Scope is used (as stated above), but the VariableScope is used to hold additional internal information (i.e., thread id, stack frame level) that is not directly represented in the DAP Scope response.

@GitMensch GitMensch requested a review from WebFreak001 April 24, 2022 18:47
Copy link
Owner

@WebFreak001 WebFreak001 left a comment

Choose a reason for hiding this comment

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

I like the change, can you check out my two comments?

Remove the arbitrary separation between the variable references used
to represent scopes and those used to represent variable objects.
This implementation uses a common pool of handles to support both
variable object references as well as scope references, thus
eliminating the previous bounded numerical range used for scopes,
which was not large enough to represent stack frames multiple levels
deep, due to thread and level information being encoded in the scope
variable reference value.
@brownts brownts force-pushed the bugfix/stack_frame_locals branch from ff40395 to 6baa66b Compare April 26, 2022 03:03
@GitMensch GitMensch merged commit 470e166 into WebFreak001:master Apr 26, 2022
@GitMensch
Copy link
Collaborator

I'd rather keep bug fixes and new functionality separate, from a semantic versioning perspective, as bug fixes can go into patch releases (e.g., 0.26.1), but new functionality should go into a minor release (e.g., 0.27.0)

Of course new minor versions can/should also include bug fixes ;-)

I think for 0.26.1 #346 and possibly #332 are the open points, before going on to bigger changes. @brownts I guess you don't want to include #330 soon, do you?
#347 would be useful at any time, @WebFreak001 are you working on this or is it mainly a point on the TODO list?

@WebFreak001
Copy link
Owner

@GitMensch it's just a point on the TODO list, I haven't really touched code-debug code in a while

@brownts
Copy link
Collaborator Author

brownts commented Apr 26, 2022

In reference to #347, I do have a unit test github action that I created locally to run the existing unit tests for my own testing, but hadn't pushed it yet. I can create a PR for that soon. I haven't done anything towards an integration test though.

@GitMensch
Copy link
Collaborator

Sounds good! Please do so and let's discuss automated testing in that PR or the referenced issue.

@brownts
Copy link
Collaborator Author

brownts commented Apr 26, 2022

Also, to follow-up on your other question, I haven't done much on #330 yet, so it's not ready to go into a near-term release.

@GitMensch
Copy link
Collaborator

In reference to #347, I do have a unit test github action that I created locally to run the existing unit tests for my own testing, but hadn't pushed it yet. I can create a PR for that soon. I haven't done anything towards an integration test though.

@brownts: was the unit testing part already pushed with the sourceFileMap changes today or are there more? In any case: can you please PR the unit testing GH action you already have?

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.

Local variables not displayed more than 2 stack frames deep
3 participants