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

Tue Jul 20 16:19:06 PDT 2021

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

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.

================
Comment at: lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py:287
+
+
+    @skipIfWindows
----------------
just one line between methods

================
Comment at: lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py:393
+        self.verify_variables(
+            expandable_expression["children"], response["body"]["variables"]
+        )
----------------
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

================
Comment at: lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py:53
             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
----------------
clayborg wrote:
> Don't use 1 line if statements, hard to read and it is over 80 columns
you need to fix this

================
Comment at: lldb/tools/lldb-vscode/VSCode.h:84-85
+struct Variables {
+  // Bit mask to tell if a variableReference is inside
+  // expandable_permanent_variables or not.
+  static constexpr int64_t PermanentVariableBitMask = (1ll << 32);
----------------
use /// for headerdoc comments

================
Comment at: lldb/tools/lldb-vscode/VSCode.h:95-96
+
+  /// Containing expandable variables that are alive in this stop state.
+  /// Will be cleared when debuggee resumes.
+  llvm::DenseMap<int64_t, lldb::SBValue> expandable_variables;
----------------
Remove the "Containing" word

================
Comment at: lldb/tools/lldb-vscode/VSCode.h:98
+  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.
----------------
same here about the word "Containing"

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

+struct Variables {
+  // Bit mask to tell if a variableReference is inside
----------------
yinghuitan wrote:
> wallace wrote:
> > Move it to a new file and add a global comment explaining the different kinds of variables that exist
> I can do this in future refactoring. I am doing pragmatic refactoring here without delaying fixing the important bug.
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.

================
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;
----------------
yinghuitan wrote:
> wallace wrote:
> > 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
> I can add a comment to make it clear. Greg is against long names.
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. 

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

================
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:
> wallace wrote:
> > formatting
> also explain what happens if the var_ref is invalid, does it crash or an invalid SBValue is returned?
this is pending, specially the explanation of what happens if var_ref is invalid

================
Comment at: lldb/tools/lldb-vscode/VSCode.h:109-110
+
+  /// Insert a new \p variable.
+  int64_t InsertNewExpandableVariable(lldb::SBValue variable,
+                                      bool is_permanent);
----------------
wallace wrote:
> explain what the return value is
same here, yo haven't done this...

================
Comment at: lldb/tools/lldb-vscode/VSCode.h:86
+  // Bit tag bit to tell if a variableReference is inside expandablePermanentVariables or not.
+  static constexpr int PermanentVariableReferenceTagBit = 32;
+
----------------
clayborg wrote:
> We should just define this as a mask instead of a bit number as we always use it as a mask.
this is pending

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105166