This is an archive of the discontinued LLVM Phabricator instance.

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

yinghuitan created this revision.Jun 29 2021, 6:34 PM
yinghuitan requested review of this revision.Jun 29 2021, 6:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2021, 6:34 PM
clayborg requested changes to this revision.Jun 29 2021, 7:33 PM

Mostly LLDB and LLVM coding guideline conventions that need to be followed. Many comments and code suggestions. But there are some code changes like having unique IDs for temp variable references and permanent ones.

Main issues are:

  • LLVM coding guidelines can't go over 80 columns, must wrap. If you use Phabricator it would fix these for you
  • LLDB variable names are lower case with underscores between words ("isPermanent" should be "is_permament")
  • LLDB methods are camel case and start with upper case letter ("getNewVariableReference" should be "GetNewVariableReference")
  • LLVM coding guidelines want no braces on single line if/else
  • LLVM coding guidelines for inline variable names are to use inline C comments with equal sign after variable name: "/*is_permanent=*/true"
lldb/tools/lldb-vscode/VSCode.cpp
33

as long as we are changing this, we don't need to default construct anything

383–390

over 80 character column limit

531

LLDB function naming is camel case with upper case first letter.

532–535
539–541

80 columns max and use separate var ref indexes

544–546

fix LLDB coding guidelines

548
550–555

Follow LLVM coding guidelines where there are no braces on single statement if/else, lldb var names

559–567

LLDB coding guidelines and 80 column limit

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

Rename

85

We should just define this as a mask instead of a bit number as we always use it as a mask.

91

LLDB naming conventions, and we should have two IDs, one for permanent and one for temp

92
93
96

Follow LLDB coding guidelines for locals which differs from LLVM, and functions start with capital letter

99
100

"is_permanent" is the LLDB coding style for locals and function names

102

80 columns max, wrap this comment per llvm coding guidelines.

103
106

80 columns max, wrap this comment per llvm coding guidelines and make LLDB variable names.

128
lldb/tools/lldb-vscode/lldb-vscode.cpp
112

make a "g_vsc.willContinue()" method in case we need to add more things later and move "g_vsc.variableHolder.clear();" into that new method

115

Lets just call this function to see if the variable reference is a top level scope. This will avoid any asserts and clean up the code. Name of this function can improve

116

remove assert, we can return NULL to indicate this isn't a top level scope.

1236–1239

80 column limit

1916–1925

LLDB and LLVM inline comment coding guideline issues

2769–2774

Use new GetTopLevelScope function return value

2773–2774

We don't have to search backwards anymore since we make unique variable names when we have duplicates. So this for loop can now be:

uint64_t end_idx = pScopeRef->GetSize();
for (uint64_t i=0; i<end_idx; ++i)
2774–2778

use lldb variable name

2794

80 column exceeded. I would suggest renaming "getVariableFromVariableReference" to a shorter name like just "GetVariable(int64_t var_Ref)"

2810–2812

80 column limit and naming conventions.

2923–2930

Use return value of GetTopLevelScope() and rename vars

2947

lldb var names

2948–2950

remove braces and obey 80 column limit

2948–2951

80 columns exceeded

2951
2960

80 cols, use new LLDB var name for variablesReference

2968–2974

80 columns, use lldb variable names

This revision now requires changes to proceed.Jun 29 2021, 7:33 PM
yinghuitan marked 37 inline comments as done.

Address review comments

yinghuitan marked an inline comment as done.

Rerun linter

rebase to latest master

clayborg requested changes to this revision.Jun 29 2021, 9:29 PM
clayborg added inline comments.
lldb/tools/lldb-vscode/VSCode.cpp
531
535

Do we want to set "next_temporary_var_ref" to VARREF_FIRST_VAR_IDX here too? Ok if we don't.

538

rename

549

rename to "GetVariable" to help with 80 column limit and make const;

550–555

We need to guard against the variable reference not existing in the map to make sure we never crash:

558

var name for LLDB and rename to shorter name.

560

rename newVariablesReferences to "var_ref"

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

rename to "temporary_variables" and add a comment

95

rename to "permanent_variables" and add comment

99

lldb variable name, and make static since it doesn't require any ivar access

110

Fix LLDB variable name

114

clear sounds like you are clearing everything in the class. I would name this willContinue() like the others

243

Correct camel case for LLDB function names

This revision now requires changes to proceed.Jun 29 2021, 9:29 PM

We will need to add tests for this as well to ensure it doesn't regress. Tests should be added to: lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py

I would add a new test function into the TestVSCode_variables class:

@skipIfWindows
@skipIfRemote
def test_scopes_and_evaluate(self):

You can probably use the same test program as test_scopes_variables_setVariable_evaluate. We will want to:

  • send a "scopes" packet
  • send a "evaluate" packet with some structure as the expression and "repl" as the source
  • send a "scopes packet
  • make sure we can expand the "evaluate" we did prior to running the second "scopes"
yinghuitan added inline comments.Jun 30 2021, 2:49 PM
lldb/tools/lldb-vscode/VSCode.cpp
531

I am not sure. I like to call it Clear() in cause we will need to call it from other scenario beyond process continue.

535

Good question. I guess we can reset it if wanted but not matter much though.

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

I kind of disagree with this. I think the keyword "expandable" is very important for reading. It immediately tells reader that the map only contains expandable variables instead of any other variables.

95

The same as my comment above.

114

I am not sure. I like to call it Clear() in cause we will need to call it from other scenario beyond process continue. I do not think reader will be confused by permanent map is not cleared because the name permanent imply that.

yinghuitan marked 13 inline comments as done.

Address review comments and add testcase.

clayborg requested changes to this revision.Jul 1 2021, 11:09 AM
clayborg added inline comments.
lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py
53–54

Don't use 1 line if statements, hard to read and it is over 80 columns

332

a/Evluate/Evaluate/

333–339

Why is this here? this has the same name as the next variable.

clayborg added inline comments.Jul 1 2021, 11:09 AM
lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py
386–389

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(..)"

2950

long function name, rename to just "Insert"?

2969–2974

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
292

i think you are missing a verb here

305–378

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

359–364

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

743

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

743

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
386–389

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–54

you need to fix this

287

just one line between methods

393

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
743

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.

2540

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.

2593

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.

2784

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.

2812

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

2950

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
2784

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

2812

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
2812

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.