Fix expression evaluation result expansion in lldb-vscode

[yinghuitan]

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.

[clayborg]

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

[yinghuitan]

Address review comments

[yinghuitan]

Rerun linter

[yinghuitan]

rebase to latest master

[clayborg]

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

[clayborg]

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]

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]

Address review comments and add testcase.

[clayborg]

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]

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

[wallace]

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]

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]

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

[yinghuitan]

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

[yinghuitan]

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]

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]

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.

[clayborg]

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]

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]

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.

lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py

for i in range(start_idx, start_idx + count):
         verify_dict['[%i]' % (i)] = {'type': 'int', 'value': str(i+offset)}
     return verify_dict
 
 
 class TestVSCode_variables(lldbvscode_testcase.VSCodeTestCaseBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
-    def verify_values(self, verify_dict, actual, varref_dict=None):
+    def verify_values(self, verify_dict, actual, varref_dict=None, expression=None):
         if 'equals' in verify_dict:
             verify = verify_dict['equals']
             for key in verify:
                 verify_value = verify[key]
                 actual_value = actual[key]
                 self.assertEqual(verify_value, actual_value,
                                 '"%s" keys don\'t match (%s != %s)' % (
                                     key, actual_value, verify_value))
def verify_values(self, verify_dict, actual, varref_dict=None, expression=None):
                                     verify_value))
         hasVariablesReference = 'variablesReference' in actual
         varRef = None
         if hasVariablesReference:
             # Remember variable references in case we want to test further
             # by using the evaluate name.
             varRef = actual['variablesReference']
             if varRef != 0 and varref_dict is not None:
-                varref_dict[actual['evaluateName']] = varRef
+                evaluateName = expression if expression is not None else actual['evaluateName']
+                varref_dict[evaluateName] = varRef
         if ('hasVariablesReference' in verify_dict and
                 verify_dict['hasVariablesReference']):
             self.assertTrue(hasVariablesReference,
                             "verify variable reference")
         if 'children' in verify_dict:
             self.assertTrue(hasVariablesReference and varRef is not None and
                             varRef != 0,
                             ("children verify values specified for "
def test_scopes_variables_setVariable_evaluate(self):
         names = [var['name'] for var in locals]
         # The first shadowed x shouldn't have a suffix anymore
         verify_locals['x'] = {'equals': {'type': 'int', 'value': '17'}}
         self.assertNotIn('x @ main.cpp:17', names)
         self.assertNotIn('x @ main.cpp:19', names)
         self.assertNotIn('x @ main.cpp:21', names)
 
         self.verify_variables(verify_locals, locals)
+
+
+    @skipIfWindows
+    @skipIfRemote
+    def test_scopes_and_evaluate_expansion(self):
+        """
+        Tests the evaluated expression expands successfully after "scopes" packets
+        and permanent expressions persist.
+        """
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+        source = "main.cpp"
+        breakpoint1_line = line_number(source, "// breakpoint 1")
+        lines = [breakpoint1_line]
+        # Set breakpoint in the thread function so we can step the threads
+        breakpoint_ids = self.set_source_breakpoints(source, lines)
+        self.assertEqual(
+            len(breakpoint_ids), len(lines), "expect correct number of breakpoints"
+        )
+        self.continue_to_breakpoints(breakpoint_ids)
+
+        # Verify locals
+        locals = self.vscode.get_local_variables()
+        buffer_children = make_buffer_verify_dict(0, 32)
+        verify_locals = {
+            "argc": {"equals": {"type": "int", "value": "1"}},
+            "argv": {
+                "equals": {"type": "const char **"},
+                "startswith": {"value": "0x"},
+                "hasVariablesReference": True,
+            },
+            "pt": {
+                "equals": {"type": "PointType"},
+                "hasVariablesReference": True,
+                "children": {
+                    "x": {"equals": {"type": "int", "value": "11"}},
+                    "y": {"equals": {"type": "int", "value": "22"}},
+                    "buffer": {"children": buffer_children},
+                },
+            },
+            "x": {"equals": {"type": "int"}},
+        }
+        self.verify_variables(verify_locals, locals)
+
+        # Evaluate expandable expression twice: once permanent (from repl)
+        # the other temporary (from other UI).
+        expandable_expression = {
+            "name": "pt",
+            "response": {
+                "equals": {"type": "PointType"},
+                "startswith": {"result": "PointType @ 0x"},
+                "hasVariablesReference": True,
+            },
+            "children": {
+                "x": {"equals": {"type": "int", "value": "11"}},
+                "y": {"equals": {"type": "int", "value": "22"}},
+                "buffer": {"children": buffer_children},
+            },
+        }
+
+        # Evaluate from permanent UI.
+        permanent_expr_varref_dict = {}
+        response = self.vscode.request_evaluate(
+            expandable_expression["name"], frameIndex=0, threadId=None, context="repl"
+        )
+        self.verify_values(
+            expandable_expression["response"],
+            response["body"],
+            permanent_expr_varref_dict,
+            expandable_expression["name"],
+        )
+
+        # Evaluate from temporary UI.
+        temporary_expr_varref_dict = {}
+        response = self.vscode.request_evaluate(expandable_expression["name"])
+        self.verify_values(
+            expandable_expression["response"],
+            response["body"],
+            temporary_expr_varref_dict,
+            expandable_expression["name"],
+        )
+
+        # Evaluate locals again.
+        locals = self.vscode.get_local_variables()
+        self.verify_variables(verify_locals, locals)
+
+        # Verify the evaluated expressions before second locals evaluation
+        # can be expanded.
+        var_ref = temporary_expr_varref_dict[expandable_expression["name"]]
+        response = self.vscode.request_variables(var_ref)
+        self.verify_variables(
+            expandable_expression["children"], response["body"]["variables"]
+        )
+
+        # Continue to breakpoint 3, permanent variable should still exist
+        # after resume.
+        breakpoint3_line = line_number(source, "// breakpoint 3")
+        lines = [breakpoint3_line]
+        breakpoint_ids = self.set_source_breakpoints(source, lines)
+        self.assertEqual(
+            len(breakpoint_ids), len(lines), "expect correct number of breakpoints"
+        )
+        self.continue_to_breakpoints(breakpoint_ids)
+
+        var_ref = permanent_expr_varref_dict[expandable_expression["name"]]
+        response = self.vscode.request_variables(var_ref)
+        self.verify_variables(
+            expandable_expression["children"], response["body"]["variables"]
+        )

lldb/tools/lldb-vscode/VSCode.h

 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include "lldb/API/SBAttachInfo.h"
 #include "lldb/API/SBBreakpoint.h"
 #include "lldb/API/SBBreakpointLocation.h"
 #include "lldb/API/SBCommandInterpreter.h"
 #include "lldb/API/SBCommandReturnObject.h"
 #include "lldb/API/SBCommunication.h"
 #include "lldb/API/SBDebugger.h"
 #include "lldb/API/SBEvent.h"
 #include "lldb/API/SBHostOS.h"

 #include "ExceptionBreakpoint.h"
 #include "FunctionBreakpoint.h"
 #include "IOStream.h"
 #include "ProgressEvent.h"
 #include "RunInTerminal.h"
 #include "SourceBreakpoint.h"
 #include "SourceReference.h"
 
 #define VARREF_LOCALS (int64_t)1
 #define VARREF_GLOBALS (int64_t)2
 #define VARREF_REGS (int64_t)3
 #define VARREF_FIRST_VAR_IDX (int64_t)4
-#define VARREF_IS_SCOPE(v) (VARREF_LOCALS <= 1 && v < VARREF_FIRST_VAR_IDX)
-#define VARIDX_TO_VARREF(i) ((i) + VARREF_FIRST_VAR_IDX)
-#define VARREF_TO_VARIDX(v) ((v)-VARREF_FIRST_VAR_IDX)
 #define NO_TYPENAME "<no-type>"
 
 namespace lldb_vscode {
 
 typedef llvm::DenseMap<uint32_t, SourceBreakpoint> SourceBreakpointMap;
 typedef llvm::StringMap<FunctionBreakpoint> FunctionBreakpointMap;
 enum class OutputType { Console, Stdout, Stderr, Telemetry };
 
 enum VSCodeBroadcasterBits {
   eBroadcastBitStopEventThread = 1u << 0,
   eBroadcastBitStopProgressThread = 1u << 1
 };
 
 typedef void (*RequestCallback)(const llvm::json::Object &command);
 
 enum class PacketStatus {
   Success = 0,
   EndOfFile,
   JSONMalformed,
   JSONNotObject
 };
 
+struct Variables {
+  // Bit mask to tell if a variableReference is inside
+  // expandable_permanent_variables or not.
+  static constexpr int64_t PermanentVariableBitMask = (1ll << 32);
+
+  lldb::SBValueList locals;
+  lldb::SBValueList globals;
+  lldb::SBValueList registers;
+
+  int64_t next_temporary_var_ref{VARREF_FIRST_VAR_IDX};
+  int64_t next_permanent_var_ref{VARREF_FIRST_VAR_IDX};
+
+  /// Containing expandable variables that are alive in this stop state.
+  /// Will be cleared when debuggee resumes.
+  llvm::DenseMap<int64_t, lldb::SBValue> expandable_variables;
+  /// Containing expandable variables that persist across entire debug session.
+  /// These are the variables evaluated from debug console REPL.
+  llvm::DenseMap<int64_t, lldb::SBValue> expandable_permanent_variables;
+
+  /// Check if \p var_ref points to variable in a
+  /// expandable_permanent_variables.
+  static bool IsPermanentVariableReference(int64_t var_ref);
+
+  /// \return a new variableReference. If is_permanent is true the returned
+  /// value will have the PermanentVariableBitMask bit set.
+  int64_t GetNewVariableRefence(bool is_permanent);
+
+  /// \return the expandable variable corresponding with variableReference value
+  /// of \p value.
+  lldb::SBValue GetVariable(int64_t var_ref) const;
+
+  /// Insert a new \p variable.
+  int64_t InsertNewExpandableVariable(lldb::SBValue variable,
+                                      bool is_permanent);
+
+  /// Clear all scope variables and non-permanent expandable variables.
+  void Clear();
+};
+
 struct VSCode {
   std::string debug_adaptor_path;
   InputStream input;
   OutputStream output;
   lldb::SBDebugger debugger;
   lldb::SBTarget target;
-  lldb::SBValueList variables;
+  Variables variables;
   lldb::SBBroadcaster broadcaster;
-  int64_t num_regs;
-  int64_t num_locals;
-  int64_t num_globals;
   std::thread event_thread;
   std::thread progress_event_thread;
   std::unique_ptr<std::ofstream> log;
   llvm::DenseMap<lldb::addr_t, int64_t> addr_to_source_ref;
   llvm::DenseMap<int64_t, SourceReference> source_map;
   llvm::StringMap<SourceBreakpointMap> source_breakpoints;
   FunctionBreakpointMap function_breakpoints;
   std::vector<ExceptionBreakpoint> exception_breakpoints;
struct VSCode {
   ///     The name of the request following the Debug Adapter Protocol
   ///     specification.
   ///
   /// \param[in] callback
   ///     The callback to execute when the given request is triggered by the
   ///     IDE.
   void RegisterRequestCallback(std::string request, RequestCallback callback);
 
+  /// Debuggee will continue from stopped state.
+  void WillContinue() { variables.Clear(); }
+
 private:
   // Send the JSON in "json_str" to the "out" stream. Correctly send the
   // "Content-Length:" field followed by the length, followed by the raw
   // JSON bytes.
   void SendJSON(const std::string &json_str);
 };
 
 extern VSCode g_vsc;
 
 } // namespace lldb_vscode
 
 #endif

lldb/tools/lldb-vscode/VSCode.cpp

 
 using namespace lldb_vscode;
 
 namespace lldb_vscode {
 
 VSCode g_vsc;
 
 VSCode::VSCode()
-    : variables(), broadcaster("lldb-vscode"), num_regs(0), num_locals(0),
-      num_globals(0), log(),
+    : broadcaster("lldb-vscode"),
       exception_breakpoints(
           {{"cpp_catch", "C++ Catch", lldb::eLanguageTypeC_plus_plus},
            {"cpp_throw", "C++ Throw", lldb::eLanguageTypeC_plus_plus},
            {"objc_catch", "Objective C Catch", lldb::eLanguageTypeObjC},
            {"objc_throw", "Objective C Throw", lldb::eLanguageTypeObjC},
            {"swift_catch", "Swift Catch", lldb::eLanguageTypeSwift},
            {"swift_throw", "Swift Throw", lldb::eLanguageTypeSwift}}),
       focus_tid(LLDB_INVALID_THREAD_ID), sent_terminated_event(false),
lldb::SBFrame VSCode::GetLLDBFrame(const llvm::json::Object &arguments) {
   // Upper 32 bits is the thread index ID
   lldb::SBThread thread =
       process.GetThreadByIndexID(GetLLDBThreadIndexID(frame_id));
   // Lower 32 bits is the frame index
   return thread.GetFrameAtIndex(GetLLDBFrameID(frame_id));
 }
 
 llvm::json::Value VSCode::CreateTopLevelScopes() {
   llvm::json::Array scopes;
-  scopes.emplace_back(CreateScope("Locals", VARREF_LOCALS, num_locals, false));
-  scopes.emplace_back(
-      CreateScope("Globals", VARREF_GLOBALS, num_globals, false));
-  scopes.emplace_back(CreateScope("Registers", VARREF_REGS, num_regs, false));
+  scopes.emplace_back(CreateScope("Locals", VARREF_LOCALS,
+                                  g_vsc.variables.locals.GetSize(), false));
+  scopes.emplace_back(CreateScope("Globals", VARREF_GLOBALS,
+                                  g_vsc.variables.globals.GetSize(), false));
+  scopes.emplace_back(CreateScope("Registers", VARREF_REGS,
+                                  g_vsc.variables.registers.GetSize(), false));
   return llvm::json::Value(std::move(scopes));
 }
 
 void VSCode::RunLLDBCommands(llvm::StringRef prefix,
                              const std::vector<std::string> &commands) {
   SendOutput(OutputType::Console,
              llvm::StringRef(::RunLLDBCommands(prefix, commands)));
 }
 
PacketStatus VSCode::SendReverseRequest(llvm::json::Object request,
   return PacketStatus::EndOfFile;
 }
 
 void VSCode::RegisterRequestCallback(std::string request,
                                      RequestCallback callback) {
   request_handlers[request] = callback;
 }
 
+void Variables::Clear() {
+  locals.Clear();
+  globals.Clear();
+  registers.Clear();
+  expandable_variables.clear();
+  next_temporary_var_ref = VARREF_FIRST_VAR_IDX;
+}
+
+int64_t Variables::GetNewVariableRefence(bool is_permanent) {
+  return is_permanent ? (PermanentVariableBitMask | next_permanent_var_ref++)
+                      : (next_temporary_var_ref++);
+}
+
+bool Variables::IsPermanentVariableReference(int64_t var_ref) {
+  return (var_ref & PermanentVariableBitMask) != 0;
+}
+
+lldb::SBValue Variables::GetVariable(int64_t var_ref) const {
+  if (IsPermanentVariableReference(var_ref)) {
+    auto pos = expandable_permanent_variables.find(var_ref);
+    if (pos != expandable_permanent_variables.end())
+      return pos->second;
+  } else {
+    auto pos = expandable_variables.find(var_ref);
+    if (pos != expandable_variables.end())
+      return pos->second;
+  }
+  return lldb::SBValue();
+}
+
+int64_t Variables::InsertNewExpandableVariable(lldb::SBValue variable,
+                                               bool is_permanent) {
+  auto var_ref = GetNewVariableRefence(is_permanent);
+  if (is_permanent)
+    expandable_permanent_variables.insert(std::make_pair(var_ref, variable));
+  else
+    expandable_variables.insert(std::make_pair(var_ref, variable));
+  return var_ref;
+}
+
 } // namespace lldb_vscode

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

 public:
   LLDBVSCodeOptTable() : OptTable(InfoTable, true) {}
 };
 
 typedef void (*RequestCallback)(const llvm::json::Object &command);
 
 enum LaunchMethod { Launch, Attach, AttachForSuspendedLaunch };
 
+lldb::SBValueList *GetTopLevelScope(int64_t variablesReference) {
+  switch (variablesReference) {
+  case VARREF_LOCALS:
+    return &g_vsc.variables.locals;
+  case VARREF_GLOBALS:
+    return &g_vsc.variables.globals;
+  case VARREF_REGS:
+    return &g_vsc.variables.registers;
+  default:
+    return nullptr;
+  }
+}
+
 SOCKET AcceptConnection(int portno) {
   // Accept a socket connection from any host on "portno".
   SOCKET newsockfd = -1;
   struct sockaddr_in serv_addr, cli_addr;
   SOCKET sockfd = socket(AF_INET, SOCK_STREAM, 0);
   if (sockfd < 0) {
     if (g_vsc.log)
       *g_vsc.log << "error: opening socket (" << strerror(errno) << ")"

 void request_continue(const llvm::json::Object &request) {
   llvm::json::Object response;
   FillResponse(request, response);
   lldb::SBProcess process = g_vsc.target.GetProcess();
   auto arguments = request.getObject("arguments");
   // Remember the thread ID that caused the resume so we can set the
   // "threadCausedFocus" boolean value in the "stopped" events.
   g_vsc.focus_tid = GetUnsigned(arguments, "threadId", LLDB_INVALID_THREAD_ID);
+  g_vsc.WillContinue();
   lldb::SBError error = process.Continue();
   llvm::json::Object body;
   body.try_emplace("allThreadsContinued", true);
   response.try_emplace("body", std::move(body));
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
 }
 
 // "ConfigurationDoneRequest": {
if (!expression.empty() && expression[0] == '`') {
     // this fails, then if the context is not "hover", actually evaluate an
     // expression using the expression parser.
     //
     // "frame variable" is more reliable than the expression parser in
     // many cases and it is faster.
     lldb::SBValue value = frame.GetValueForVariablePath(
         expression.data(), lldb::eDynamicDontRunTarget);
 
+    // Freeze dry the value in case users expand it later in the debug console
+    if (value.GetError().Success() && context == "repl")
+      value = value.Persist();
+
     if (value.GetError().Fail() && context != "hover")
       value = frame.EvaluateExpression(expression.data());
 
     if (value.GetError().Fail()) {
       response["success"] = llvm::json::Value(false);
       // This error object must live until we're done with the pointer returned
       // by GetCString().
       lldb::SBError error = value.GetError();
       const char *error_cstr = error.GetCString();
       if (error_cstr && error_cstr[0])
         EmplaceSafeString(response, "message", std::string(error_cstr));
       else
         EmplaceSafeString(response, "message", "evaluate failed");
     } else {
       SetValueForKey(value, body, "result");
       auto value_typename = value.GetType().GetDisplayTypeName();
       EmplaceSafeString(body, "type",
                         value_typename ? value_typename : NO_TYPENAME);
       if (value.MightHaveChildren()) {
-        auto variablesReference = VARIDX_TO_VARREF(g_vsc.variables.GetSize());
-        g_vsc.variables.Append(value);
-        body.try_emplace("variablesReference", variablesReference);
+        auto variableReference = g_vsc.variables.InsertNewExpandableVariable(
+            value, /*is_permanent=*/context == "repl");
+        body.try_emplace("variablesReference", variableReference);
       } else {
         body.try_emplace("variablesReference", (int64_t)0);
       }
     }
   }
   response.try_emplace("body", std::move(body));
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
 }
 
void request_next(const llvm::json::Object &request) {
   llvm::json::Object response;
   FillResponse(request, response);
   auto arguments = request.getObject("arguments");
   lldb::SBThread thread = g_vsc.GetLLDBThread(*arguments);
   if (thread.IsValid()) {
     // Remember the thread ID that caused the resume so we can set the
     // "threadCausedFocus" boolean value in the "stopped" events.
     g_vsc.focus_tid = thread.GetThreadID();
+    g_vsc.WillContinue();
     thread.StepOver();
   } else {
     response["success"] = llvm::json::Value(false);
   }
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
 }
 
 // "PauseRequest": {
void request_scopes(const llvm::json::Object &request) {
   // in the LLDB library, we would show the wrong thing to the user. If the
   // users switches threads with a lldb command like "`thread select 14", the
   // GUI will not update as there are no "event" notification packets that
   // allow us to change the currently selected thread or frame in the GUI that
   // I am aware of.
   if (frame.IsValid()) {
     frame.GetThread().GetProcess().SetSelectedThread(frame.GetThread());
     frame.GetThread().SetSelectedFrame(frame.GetFrameID());
   }
-  g_vsc.variables.Clear();
-  g_vsc.variables.Append(frame.GetVariables(true,   // arguments
-                                            true,   // locals
-                                            false,  // statics
-                                            true)); // in_scope_only
-  g_vsc.num_locals = g_vsc.variables.GetSize();
-  g_vsc.variables.Append(frame.GetVariables(false,  // arguments
-                                            false,  // locals
-                                            true,   // statics
-                                            true)); // in_scope_only
-  g_vsc.num_globals = g_vsc.variables.GetSize() - (g_vsc.num_locals);
-  g_vsc.variables.Append(frame.GetRegisters());
-  g_vsc.num_regs =
-      g_vsc.variables.GetSize() - (g_vsc.num_locals + g_vsc.num_globals);
+  g_vsc.variables.locals = frame.GetVariables(/*arguments=*/true,
+                                              /*locals=*/true,
+                                              /*statics=*/false,
+                                              /*in_scope_only=*/true);
+  g_vsc.variables.globals = frame.GetVariables(/*arguments=*/false,
+                                               /*locals=*/false,
+                                               /*statics=*/true,
+                                               /*in_scope_only=*/true);
+  g_vsc.variables.registers = frame.GetRegisters();
   body.try_emplace("scopes", g_vsc.CreateTopLevelScopes());
   response.try_emplace("body", std::move(body));
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
 }
 
 // "SetBreakpointsRequest": {
 //   "allOf": [ { "$ref": "#/definitions/Request" }, {
 //     "type": "object",
void request_stepIn(const llvm::json::Object &request) {
   llvm::json::Object response;
   FillResponse(request, response);
   auto arguments = request.getObject("arguments");
   lldb::SBThread thread = g_vsc.GetLLDBThread(*arguments);
   if (thread.IsValid()) {
     // Remember the thread ID that caused the resume so we can set the
     // "threadCausedFocus" boolean value in the "stopped" events.
     g_vsc.focus_tid = thread.GetThreadID();
+    g_vsc.WillContinue();
     thread.StepInto();
   } else {
     response["success"] = llvm::json::Value(false);
   }
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
 }
 
 // "StepOutRequest": {
void request_stepOut(const llvm::json::Object &request) {
   llvm::json::Object response;
   FillResponse(request, response);
   auto arguments = request.getObject("arguments");
   lldb::SBThread thread = g_vsc.GetLLDBThread(*arguments);
   if (thread.IsValid()) {
     // Remember the thread ID that caused the resume so we can set the
     // "threadCausedFocus" boolean value in the "stopped" events.
     g_vsc.focus_tid = thread.GetThreadID();
+    g_vsc.WillContinue();
     thread.StepOut();
   } else {
     response["success"] = llvm::json::Value(false);
   }
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
 }
 
 // "ThreadsRequest": {
void request_setVariable(const llvm::json::Object &request) {
   // variablesReference. It helps to disambiguate between two variables that
   // have the same name within the same scope since the "setVariables" request
   // only specifies the variable reference of the enclosing scope/variable, and
   // the name of the variable. We could have two shadowed variables with the
   // same name in "Locals" or "Globals". In our case the "id" absolute index
   // of the variable within the g_vsc.variables list.
   const auto id_value = GetUnsigned(arguments, "id", UINT64_MAX);
   if (id_value != UINT64_MAX) {
-    variable = g_vsc.variables.GetValueAtIndex(id_value);
-  } else if (VARREF_IS_SCOPE(variablesReference)) {
+    variable = g_vsc.variables.GetVariable(id_value);
+  } else if (lldb::SBValueList *top_scope =
+                 GetTopLevelScope(variablesReference)) {
     // variablesReference is one of our scopes, not an actual variable it is
     // asking for a variable in locals or globals or registers
-    int64_t start_idx = 0;
-    int64_t end_idx = 0;
-    switch (variablesReference) {
-    case VARREF_LOCALS:
-      start_idx = 0;
-      end_idx = start_idx + g_vsc.num_locals;
-      break;
-    case VARREF_GLOBALS:
-      start_idx = g_vsc.num_locals;
-      end_idx = start_idx + g_vsc.num_globals;
-      break;
-    case VARREF_REGS:
-      start_idx = g_vsc.num_locals + g_vsc.num_globals;
-      end_idx = start_idx + g_vsc.num_regs;
-      break;
-    default:
-      break;
-    }
-
-    for (int64_t i = end_idx - 1; i >= start_idx; --i) {
-      lldb::SBValue curr_variable = g_vsc.variables.GetValueAtIndex(i);
+    int64_t end_idx = top_scope->GetSize();
+    // Searching backward so that we choose the variable in closest scope
+    // among variables of the same name.
+    for (int64_t i = end_idx - 1; i >= 0; --i) {
+      lldb::SBValue curr_variable = top_scope->GetValueAtIndex(i);
       std::string variable_name = CreateUniqueVariableNameForDisplay(
           curr_variable, is_duplicated_variable_name);
       if (variable_name == name) {
         variable = curr_variable;
         if (curr_variable.MightHaveChildren())
-          newVariablesReference = i;
+          newVariablesReference = g_vsc.variables.GetNewVariableRefence(true);
         break;
       }
     }
   } else {
     // This is not under the globals or locals scope, so there are no duplicated
     // names.
 
     // We have a named item within an actual variable so we need to find it
     // withing the container variable by name.
-    const int64_t var_idx = VARREF_TO_VARIDX(variablesReference);
-    lldb::SBValue container = g_vsc.variables.GetValueAtIndex(var_idx);
+    lldb::SBValue container = g_vsc.variables.GetVariable(variablesReference);
     variable = container.GetChildMemberWithName(name.data());
     if (!variable.IsValid()) {
       if (name.startswith("[")) {
         llvm::StringRef index_str(name.drop_front(1));
         uint64_t index = 0;
         if (!index_str.consumeInteger(0, index)) {
           if (index_str == "]")
             variable = container.GetChildAtIndex(index);
         }
       }
     }
 
     // We don't know the index of the variable in our g_vsc.variables
     if (variable.IsValid()) {
       if (variable.MightHaveChildren()) {
-        newVariablesReference = VARIDX_TO_VARREF(g_vsc.variables.GetSize());
-        g_vsc.variables.Append(variable);
+        auto is_permanent =
+            g_vsc.variables.IsPermanentVariableReference(variablesReference);
+        g_vsc.variables.InsertNewExpandableVariable(variable, is_permanent);
       }
     }
   }
 
   if (variable.IsValid()) {
     lldb::SBError error;
     bool success = variable.SetValueFromCString(value.data(), error);
     if (success) {
const auto variablesReference =
       GetUnsigned(arguments, "variablesReference", 0);
   const int64_t start = GetSigned(arguments, "start", 0);
   const int64_t count = GetSigned(arguments, "count", 0);
   bool hex = false;
   auto format = arguments->getObject("format");
   if (format)
     hex = GetBoolean(format, "hex", false);
 
-  if (VARREF_IS_SCOPE(variablesReference)) {
+  if (lldb::SBValueList *top_scope = GetTopLevelScope(variablesReference)) {
     // variablesReference is one of our scopes, not an actual variable it is
     // asking for the list of args, locals or globals.
     int64_t start_idx = 0;
     int64_t num_children = 0;
-    switch (variablesReference) {
-    case VARREF_LOCALS:
-      start_idx = start;
-      num_children = g_vsc.num_locals;
-      break;
-    case VARREF_GLOBALS:
-      start_idx = start + g_vsc.num_locals + start;
-      num_children = g_vsc.num_globals;
-      break;
-    case VARREF_REGS:
-      start_idx = start + g_vsc.num_locals + g_vsc.num_globals;
-      num_children = g_vsc.num_regs;
-      break;
-    default:
-      break;
-    }
+
+    num_children = top_scope->GetSize();
     const int64_t end_idx = start_idx + ((count == 0) ? num_children : count);
 
     // We first find out which variable names are duplicated
     std::map<std::string, int> variable_name_counts;
     for (auto i = start_idx; i < end_idx; ++i) {
-      lldb::SBValue variable = g_vsc.variables.GetValueAtIndex(i);
+      lldb::SBValue variable = top_scope->GetValueAtIndex(i);
       if (!variable.IsValid())
         break;
       variable_name_counts[GetNonNullVariableName(variable)]++;
     }
 
     // Now we construct the result with unique display variable names
     for (auto i = start_idx; i < end_idx; ++i) {
-      lldb::SBValue variable = g_vsc.variables.GetValueAtIndex(i);
+      lldb::SBValue variable = top_scope->GetValueAtIndex(i);
 
       if (!variable.IsValid())
         break;
-      variables.emplace_back(CreateVariable(variable, VARIDX_TO_VARREF(i), i,
-                                            hex,
+
+      int64_t var_ref = 0;
+      if (variable.MightHaveChildren()) {
+        var_ref = g_vsc.variables.InsertNewExpandableVariable(
+            variable, /*is_permanent=*/false);
+      }
+      variables.emplace_back(CreateVariable(
+          variable, var_ref, var_ref != 0 ? var_ref : UINT64_MAX, hex,
           variable_name_counts[GetNonNullVariableName(variable)] > 1));
     }
   } else {
     // We are expanding a variable that has children, so we will return its
     // children.
-    const int64_t var_idx = VARREF_TO_VARIDX(variablesReference);
-    lldb::SBValue variable = g_vsc.variables.GetValueAtIndex(var_idx);
+    lldb::SBValue variable = g_vsc.variables.GetVariable(variablesReference);
     if (variable.IsValid()) {
       const auto num_children = variable.GetNumChildren();
       const int64_t end_idx = start + ((count == 0) ? num_children : count);
       for (auto i = start; i < end_idx; ++i) {
         lldb::SBValue child = variable.GetChildAtIndex(i);
         if (!child.IsValid())
           break;
         if (child.MightHaveChildren()) {
-          const int64_t var_idx = g_vsc.variables.GetSize();
-          auto childVariablesReferences = VARIDX_TO_VARREF(var_idx);
-          variables.emplace_back(
-              CreateVariable(child, childVariablesReferences, var_idx, hex));
-          g_vsc.variables.Append(child);
+          auto is_permanent =
+              g_vsc.variables.IsPermanentVariableReference(variablesReference);
+          auto childVariablesReferences =
+              g_vsc.variables.InsertNewExpandableVariable(child, is_permanent);
+          variables.emplace_back(CreateVariable(child, childVariablesReferences,
+                                                childVariablesReferences, hex));
         } else {
           variables.emplace_back(CreateVariable(child, 0, INT64_MAX, hex));
         }
       }
     }
   }
   llvm::json::Object body;
   body.try_emplace("variables", std::move(variables));