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

Wed Jul 7 12:59:41 PDT 2021

wallace requested changes to this revision.
wallace added inline comments.

================
Comment at: lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py:292
+            Tests the evaluated expression expands successfully after "scopes" packets
+            and permanent
+        '''
----------------
i think you are missing a verb here

================
Comment at: lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py:305-378
+        # 1. Verify locals
+        locals = self.vscode.get_local_variables()
+        buffer_children = make_buffer_verify_dict(0, 32)
+        verify_locals = {
+            'argc': {
+                'equals': {'type': 'int', 'value': '1'}
+            },
----------------
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

================
Comment at: lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py:359-364
+        self.verify_values(expandable_expression['response'], response['body'], permanent_expr_varref_dict, expandable_expression['name'])
+
+        # 2.2. 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'])
----------------
try to stick to 80 cols

================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:33
 VSCode::VSCode()
-    : variables(), broadcaster("lldb-vscode"), num_regs(0), num_locals(0),
-      num_globals(0), log(),
+    : broadcaster("lldb-vscode"),
       exception_breakpoints(
----------------
probably you shouldn't have removed log()

================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:536
+  expandable_variables.clear();
+  next_temporary_var_ref = VARREF_FIRST_VAR_IDX;
+}
----------------
Better not reset it, if every variable has a distinct ref throughout the debug session, it'll be easier for debugging the DAP messages

================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:540-541
+int64_t Variables::GetNewVariableRefence(bool is_permanent) {
+  return is_permanent ? (PermanentVariableBitMask | next_permanent_var_ref++)
+                      : (next_temporary_var_ref++);
+}
----------------
clayborg wrote:
> use if statement. Easier to read
+1

================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:563
+                                               bool is_permanent) {
+  auto var_ref = GetNewVariableRefence(is_permanent);
+  if (is_permanent)
----------------
clayborg wrote:
> don't use auto, nice to see what the type is: int64_t
yes, in lldb auto should only be used if it really helps readability, and in this case it only helps writing, not reading

================
Comment at: lldb/tools/lldb-vscode/VSCode.h:57-60
 #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
----------------
tbh it might be better if you just make them consts variables

================
Comment at: lldb/tools/lldb-vscode/VSCode.h:83

+struct Variables {
+  // Bit mask to tell if a variableReference is inside
----------------
Move it to a new file and add a global comment explaining the different kinds of variables that exist

================
Comment at: lldb/tools/lldb-vscode/VSCode.h:92-93
+
+  int64_t next_temporary_var_ref{VARREF_FIRST_VAR_IDX};
+  int64_t next_permanent_var_ref{VARREF_FIRST_VAR_IDX};
+  llvm::DenseMap<int64_t, lldb::SBValue> expandable_variables;
----------------
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.

================
Comment at: lldb/tools/lldb-vscode/VSCode.h:94
+  int64_t next_permanent_var_ref{VARREF_FIRST_VAR_IDX};
+  llvm::DenseMap<int64_t, lldb::SBValue> expandable_variables;
+  llvm::DenseMap<int64_t, lldb::SBValue> expandable_permanent_variables;
----------------
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

================
Comment at: lldb/tools/lldb-vscode/VSCode.h:97
+
+  /// Check if \p var_ref points to variable in a
+  /// expandable_permanent_variables.
----------------
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.

================
Comment at: lldb/tools/lldb-vscode/VSCode.h:101-102
+
+  /// \return a new variableReference. If is_permanent is true the returned
+  /// value will have the PermanentVariableBitMask bit set.
+  int64_t GetNewVariableRefence(bool is_permanent);
----------------
This formatting is wrong, \return should be alone in one single line

================
Comment at: lldb/tools/lldb-vscode/VSCode.h:101-102
+
+  /// \return a new variableReference. If is_permanent is true the returned
+  /// value will have the PermanentVariableBitMask bit set.
+  int64_t GetNewVariableRefence(bool is_permanent);
----------------
wallace wrote:
> This formatting is wrong, \return should be alone in one single line
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

================
Comment at: lldb/tools/lldb-vscode/VSCode.h:105-106
+
+  /// \return the expandable variable corresponding with variableReference value
+  /// of \p value.
+  lldb::SBValue GetVariable(int64_t var_ref) const;
----------------
formatting

================
Comment at: lldb/tools/lldb-vscode/VSCode.h:105-106
+
+  /// \return the expandable variable corresponding with variableReference value
+  /// of \p value.
+  lldb::SBValue GetVariable(int64_t var_ref) const;
----------------
wallace wrote:
> formatting
also explain what happens if the var_ref is invalid, does it crash or an invalid SBValue is returned?

================
Comment at: lldb/tools/lldb-vscode/VSCode.h:109-110
+
+  /// Insert a new \p variable.
+  int64_t InsertNewExpandableVariable(lldb::SBValue variable,
+                                      bool is_permanent);
----------------
explain what the return value is

================
Comment at: lldb/tools/lldb-vscode/VSCode.h:110
+  /// Insert a new \p variable.
+  int64_t InsertNewExpandableVariable(lldb::SBValue variable,
+                                      bool is_permanent);
----------------
this is the only insert method here, so either InsertExpandableVariable or Insert should be fine, new is redundant

================
Comment at: lldb/tools/lldb-vscode/VSCode.h:113-114
+
+  /// Clear all scope variables and non-permanent expandable variables.
+  void Clear();
+};
----------------
Rename this to ClearCurrentScopeVariables or ClearTemporaryVariables that to be more explicit

================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:110

+lldb::SBValueList *GetTopLevelScope(int64_t variablesReference) {
+  switch (variablesReference) {
----------------
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

================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:119
+  default:
+    return nullptr;
+  }
----------------
return SBValueList() should be fine

================
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();
----------------
better rename this to WillResume, as this is also used for stepping

================
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();
----------------
wallace wrote:
> better rename this to WillResume, as this is also used for stepping
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 ?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105166