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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
