[Lldb-commits] [PATCH] D105166: Fix expression evaluation result expansion in lldb-vscode

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

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



================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:540
+  if (is_permanent)
+    return PermanentVariableBitMask | next_permanent_var_ref++;
+  else
----------------
If we initialize this to PermanentVariableBitMask, then this just becomes the code suggestion


================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:562
+
+int64_t Variables::InserExpandableVariable(lldb::SBValue variable,
+                                           bool is_permanent) {
----------------
rename to "Insert(...)" as mentioned before. 


================
Comment at: lldb/tools/lldb-vscode/VSCode.h:93
+  int64_t next_temporary_var_ref{VARREF_FIRST_VAR_IDX};
+  int64_t next_permanent_var_ref{VARREF_FIRST_VAR_IDX};
+
----------------
Just set this to be PermanentVariableBitMask. No need to skip the first 4 entries 


================
Comment at: lldb/tools/lldb-vscode/VSCode.h:118
+  /// \return variableReference assigned to this expandable variable.
+  int64_t InserExpandableVariable(lldb::SBValue variable, bool is_permanent);
+
----------------
Fix type in "Inser" instead of "Insert". Consider renaming to just "Insert(...)" for simplicity


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:743
   g_vsc.focus_tid = GetUnsigned(arguments, "threadId", LLDB_INVALID_THREAD_ID);
+  g_vsc.WillContinue();
   lldb::SBError error = process.Continue();
----------------
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.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1236
       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.InserExpandableVariable(
+            value, /*is_permanent=*/context == "repl");
----------------
rename to "Insert(..)"


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1790
     g_vsc.focus_tid = thread.GetThreadID();
+    g_vsc.WillContinue();
     thread.StepOver();
----------------
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.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2540
     g_vsc.focus_tid = thread.GetThreadID();
+    g_vsc.WillContinue();
     thread.StepInto();
----------------
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.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2593
     g_vsc.focus_tid = thread.GetThreadID();
+    g_vsc.WillContinue();
     thread.StepOut();
----------------
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.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2784
         if (curr_variable.MightHaveChildren())
-          newVariablesReference = i;
+          newVariablesReference = g_vsc.variables.GetNewVariableRefence(true);
         break;
----------------
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.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2812
+            g_vsc.variables.IsPermanentVariableReference(variablesReference);
+        g_vsc.variables.InserExpandableVariable(variable, is_permanent);
       }
----------------
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". 


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2950
+      if (variable.MightHaveChildren()) {
+        var_ref = g_vsc.variables.InserExpandableVariable(
+            variable, /*is_permanent=*/false);
----------------
Rename to Insert(...) as mentioned before.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105166/new/

https://reviews.llvm.org/D105166