Page MenuHomePhabricator

Fix expression evaluation result expansion in lldb-vscode
ClosedPublic

Authored by yinghuitan on Jun 29 2021, 6:34 PM.

Details

Summary

VScode now sends a "scopes" DAP request immediately after any expression evaluation.
This scopes request would clear and invalidate any non-scoped expandable variables in g_vsc.variables, causing later "variables" request to return empty result.
The symptom is that any expandable variables in VScode watch window/debug console UI to return empty content.

This diff fixes this issue by only clearing the expandable variables at process continue time. To achieve this, we have to repopulate all scoped variables
during context switch for each "scopes" request without clearing global expandable variables.
So the PR puts the scoped variables into its own locals/globals/registers; and all expandable variables into separate "expandableVariables" list.
Also, instead of using the variable index for "variableReference", it generates a new variableReference id each time as the key of "expandableVariables".

As a further new feature, this PR adds a new "expandablePermanentVariables" which has the lifetime of debug session. Any expandable variables from debug console
are added into this list. This enables users to snapshot expanable old variable in debug console and compare with new variables if desire.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
clayborg added inline comments.Jul 1 2021, 11:09 AM
lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py
389–392

When we are stopped here, has the actual "pt" value been updated? I would like the test to have the current value of "pt" differ from the freeze dried version from the 'repl'. We need to make sure the value hasn't changed, but the real 'pt' has

lldb/tools/lldb-vscode/VSCode.cpp
540–541

use if statement. Easier to read

563

don't use auto, nice to see what the type is: int64_t

lldb/tools/lldb-vscode/VSCode.h
95

my main point here is that really long variable names make it hard to write code that fits into 80 columns. So check where this is used in the code and make sure you aren't running into 80 cols a lot. If you are, consider shortening it.

lldb/tools/lldb-vscode/lldb-vscode.cpp
1236–1238

Example of long function names making 80 columns harder here. We could rename to just "Insert(..)"

2945

long function name, rename to just "Insert"?

2964–2969

don't use auto for bool

This revision now requires changes to proceed.Jul 1 2021, 11:09 AM
wallace requested changes to this revision.Jul 7 2021, 12:59 PM
wallace added inline comments.
lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py
295

i think you are missing a verb here

308–381

I suggest removing these numbers (1 to 5), because if later we want to add a check step in the middle, we'll have to modify many more lines polluting the git blame

362–367

try to stick to 80 cols

lldb/tools/lldb-vscode/VSCode.cpp
33

probably you shouldn't have removed log()

536

Better not reset it, if every variable has a distinct ref throughout the debug session, it'll be easier for debugging the DAP messages

540–541

+1

563

yes, in lldb auto should only be used if it really helps readability, and in this case it only helps writing, not reading

lldb/tools/lldb-vscode/VSCode.h
57–61

tbh it might be better if you just make them consts variables

83

Move it to a new file and add a global comment explaining the different kinds of variables that exist

92–93

if we are already distinguishing by the bitmask field, then I don't think we need to have these two counters. Just one should be enough.

94

maybe call this expandable_temporary_variables to distinguish it from the next variable, otherwise it seems that the permanent ones are a subset of the other one

97

better explain the broad goal of this method instead of the implementation. Something like this might be better

Check if \p var_ref points to a variable that should persist for the entire duration of the debug session, e.g. repl expandable variables.
101–102

This formatting is wrong, \return should be alone in one single line

101–102

similar to my comment above, explain the broad goal and not the implementation detail. Users of this API shouldn't really know about that bit mask

105–106

formatting

105–106

also explain what happens if the var_ref is invalid, does it crash or an invalid SBValue is returned?

109–110

explain what the return value is

110

this is the only insert method here, so either InsertExpandableVariable or Insert should be fine, new is redundant

113–114

Rename this to ClearCurrentScopeVariables or ClearTemporaryVariables that to be more explicit

lldb/tools/lldb-vscode/lldb-vscode.cpp
110

I don't it's necessary to pass a pointer, a SBValueList should be pretty cheap to copy/move around. It already has a bool operator () that you can use to check if the variable is valid or not, so your if statements should work well

119

return SBValueList() should be fine

744

better rename this to WillResume, as this is also used for stepping

744

this might not be enough if the user steps using the command line, e.g when they do instruction level stepping

It might be better to invoke this as part of the lldb::eStateRunning case in EventThreadFunction. What do you think @clayborg ?

yinghuitan marked 33 inline comments as done.

Address review comments.

lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py
389–392

Per discussion offline with Greg, the SBValue::Persist() does not seem to freeze dry the struct so will revisit in future.

lldb/tools/lldb-vscode/VSCode.cpp
33

You should talk to Greg. He suggest removing above. For me, I really do not think it matters.

540–541

I am ok either way, but I think it is purely a preference. Forcing people to use one than the other is wasting time/energy.

lldb/tools/lldb-vscode/VSCode.h
83

I can do this in future refactoring. I am doing pragmatic refactoring here without delaying fixing the important bug.

92–93

I know I can save a variable but it is clear to keep separate counter for these two categories so that they have continuous value.

94

I can add a comment to make it clear. Greg is against long names.

95

I understand that. But in modern development world, it should be the job of tools/IDE to format keep the 80 columns guideline. I do not like to scarifies the readability to short function/variable names just to meet 80 columns. If the function/variable is long formatter/IDE will wrap it not a big deal.

113–114

Talk to Greg who suggested short function name. Again, everyone has different opinion on this.

lldb/tools/lldb-vscode/lldb-vscode.cpp
110

We can, but the current approach works so I prefer to keep it.

wallace requested changes to this revision.Jul 20 2021, 4:19 PM

There are a lot of comments from your previous diff that were not addressed. I reviewed up to a certain point but fix the remaining ones anyway. About documentation, try to be more verbose, as LLDB development relies a lot on comprehensive documentation available.

Also check my comment about making sure that this fix works even if you do instruction level stepping. Another possibility is not to Clear the temporary variable list upon resumes, but instead link these variables to the current StopID, so that the next time you look for instructions, if the StopID has changed, then you know that you have resumed. That will help you with avoiding having to call WillContinue from many places. I'm afraid that if we add another stepping method to the DAP (which is totally doable) we could miss adding a call to WillContinue, and then this bug would surface again.

lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py
53–57

you need to fix this

290

just one line between methods

396

could you add an additional test making sure that if you issue the command "thread step-insn" the variables are updated correctly? Right now the only way to do instruction-level stepping is through the command line, which some users do. The IDE gets notified when stepping is done through the command line, so your fix should work in this case as well.
See my command from last time about listening to the Resume event, so that you can do your logic there instead of adding a WillContinue method in every DAP resume/step request

lldb/tools/lldb-vscode/VSCode.h
83

this bug has been around probably for years, so I think it's reasonable to spend a few minutes polishing the code and delivering something nicer. Otherwise we might never do any refactor.

Besides, don't forget to add some top-level documentation in this class explaining the difference in variables and how they work in lldb-vscode. It'll be very useful for future improvements, as it's not intuitive to figure out some of the details.

84–85

use /// for headerdoc comments

85

this is pending

94

Then use expandable_temp_vars (or even expandable_temp_variables). Almost the same size and gets rid of the possibility of misunderstanding expandable_permanent_variables as a subset of expandable_variables. We shouldn't try to exaggerate with long names, but when they do bring clarity, then we should do it.

95–96

Remove the "Containing" word

97

I insist that we should be descriptive in the documentation. In LLDB very often you have no one to ask questions, so the documentation is the go-to place to understand what's going on in the code.

In this case, IsPermanentVariableReference is the API that people will first read, and expandable_permanent_variables is just an implementation detail. So better make the comment of the API more descriptive than the variable.

98

same here about the word "Containing"

105–106

this is pending, specially the explanation of what happens if var_ref is invalid

109–110

same here, yo haven't done this...

This revision now requires changes to proceed.Jul 20 2021, 4:19 PM

Last push failed to include the changes due to git issue. Push including the real changes.

Debug console and VScode UI synchronization is a much bigger topic than instruction level stepping so I do not plan to fix in this patch. For example, if user change value via lldb comamnd, the watch/locals won't change. If user switch stack frames/threads, it won't reflect in VScode UI. It needs a thorough discussion to cover all the synchronization issues.

yinghuitan added inline comments.Jul 20 2021, 6:57 PM
lldb/tools/lldb-vscode/VSCode.h
83

I disagree. The bug is not years but a recent VScode protocol timing/sequence change caused regression. The point is not about several minutes time but focusing on big picture -- I am fixing a very important bug in lldb which prevents users from using watch window in VScode. Everyone has different opinion about refactoring or make its better. We can keep on iterating but I think landing the change to unblock users is more important at this point.

clayborg requested changes to this revision.Jul 23 2021, 4:12 PM

So there are issues with the setVariable and the variable reference it is returning. See inlined comments.

lldb/tools/lldb-vscode/VSCode.cpp
540

If we initialize this to PermanentVariableBitMask, then this just becomes the code suggestion

562

rename to "Insert(...)" as mentioned before.

lldb/tools/lldb-vscode/VSCode.h
93

Just set this to be PermanentVariableBitMask. No need to skip the first 4 entries

118

Fix type in "Inser" instead of "Insert". Consider renaming to just "Insert(...)" for simplicity

lldb/tools/lldb-vscode/lldb-vscode.cpp
744

That is a good point. Jeffrey: we can currently enter commands in the debugger console, and like:

`s

This will end up stepping the program. The user could also enter "c" or "thread step-in", etc, so we should move this to where we have the eStateRunning or to the eStateStopped in the event handler.

1236–1238

rename to "Insert(..)"

1790

Remove and handle in "eStateStopped" or "eStateRunning" as previously mentioned because the command line on the debugger console can end up making the process continue.

2539

Remove and handle in "eStateStopped" or "eStateRunning" as previously mentioned because the command line on the debugger console can end up making the process continue.

2591

Remove and handle in "eStateStopped" or "eStateRunning" as previously mentioned because the command line on the debugger console can end up making the process continue.

2779–2780

We can't just make up a variable reference here, it needs to match the variable reference for "curr_variable" when it was first inserted. It used to be set to "i" before since that _was_ the actualy variable reference of the item we are changing the value of. It is fine for this to be 0 if we are setting say a local variable that is a "int x" since it has no variable reference, but we do need the variable reference to match the original.

So, we need to track the ID given to each local, global, or register variable and map it to a variable reference.

I am not really sure why the result of this needs to return the new variable reference, or how it is used in the IDE.

2802

Rename to "Insert(...)" and you must use the variable reference that was returned.

That being said, the old code was incorrect as it was appending the same value again in the the "g_vsc.variables" list, but it didn't need to. It should have just been returning the old index of the for "variable" in the g_vsc.variables" list. So we will need a way to get the original variable reference for any "SBValue" back from the "g_vsc.variables" class. One idea is to rely on the following SBValue method:

lldb::user_id_t SBValue::GetID();

And when we call "g_vsc.variables.InserExpandableVariable(variable, is_permanent)" we have that method keep a map of "lldb::user_id_t -> var_ref".

2945

Rename to Insert(...) as mentioned before.

This revision now requires changes to proceed.Jul 23 2021, 4:12 PM
clayborg added inline comments.Jul 23 2021, 4:24 PM
lldb/tools/lldb-vscode/lldb-vscode.cpp
2779–2780

The quicker way to fix this without having to track a map of lldb::SBValue -> var_ref would be to always insert it again into our temp lists:

newVariablesReference = g_vsc.variables.InsertExpandableVariable(variable, false);

We would end up with multiple entries for this variable, but that should be ok as they will get cleared out the next time we resume/stop with the Clear() method...

2802

Thinking about this some more, we should probably just go with the code fix suggestion I made above. This might insert the same "variable" into the lists again, but this only happens when we set a variable value, so this won't be too often. Another point is people don't often edit top level structures, they edit variables that have values. So if you have a "Point pt" variable, you won't usually try to edit top level "pt" value, you would edit the "pt.x" or "pt.y" values. If you do try to edit the top level structure value, it will probably return an error. So it is doubtful that this will even cause many problems since most items users will edit won't return "true" for a call to "variable.MightHaveChildren()"

yinghuitan added inline comments.Jul 23 2021, 6:18 PM
lldb/tools/lldb-vscode/lldb-vscode.cpp
2802

This is a good point. However, reading this code more, I wonder if the current behavior is wrong -- in current behavior, after variable is located in parent scope/object, it is checked to be expandable or not and is added into expandable_variables list *before* new value assignment. I think the the correct behavior should be only checking and add to expandable_variables list *after* new value assignment, right?

yinghuitan marked 4 inline comments as done.

Fix the issue in setVariable request.

lldb/tools/lldb-vscode/lldb-vscode.cpp
1790

Sure, I originally did not do this because I am not sure if a new debug event with eStateRunning is guaranteed to generate. If that guarantee is provided, sure.

clayborg accepted this revision.Jul 27 2021, 2:10 PM
This revision was not accepted when it landed; it landed in state Needs Review.Aug 3 2021, 3:32 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.