[Lldb-commits] [PATCH] D127702: Support logpoints in lldb-vscode

[jeffrey tan via Phabricator via lldb-commits]

yinghuitan added inline comments.
Herald added a subscriber: Michael137.


================
Comment at: lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_logpoints.py:60
+            [loop_line, after_loop_line],
+            [{'logMessage': logMessage}, {}]
+        )
----------------
clayborg wrote:
> Any reason we have an empty dictionary at the end here?
We are setting two source line breakpoints here, the first one has logMessage while the second one does not have any condition. I am explicitly using {} for second breakpoint to be clear.


================
Comment at: lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_logpoints.py:120
+            [loop_line, after_loop_line],
+            [{'logMessage': logMessage}, {}]
+        )
----------------
clayborg wrote:
> Any reason we have an empty dictionary at the end here?
{} means no logMessage for second breakpoint (after_loop_line).


================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.cpp:46
+  // ascending order without any overlap between them.
+  std::vector<std::pair<int, int>> matched_curly_braces_ranges;
+
----------------
clayborg wrote:
> Might be easier to use a std::map<int, int> here instead of a vector of pairs?
I do not think so. We never need fast key lookup from the data structure. All the operations are checking/popping/pushing the back of the vector. It is more like a stack.


================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.cpp:91
+    assert(curly_braces_range.first >= last_raw_text_start);
+    size_t raw_text_len = curly_braces_range.first - last_raw_text_start;
+    rawTextMessages.emplace_back(llvm::StringRef(
----------------
clayborg wrote:
> If the is a '{' at the start of the string, this code seems like it pushes an empty string. this must be needed because we have two vectors (rawTextMessages and evalExpressions)?
Correct. We assume there is always a prefix raw text before any expressions, so rawTextMessages is always one element larger than expressions, I will enhance the documentation (see my format comment above):
```
assert(rawTextMessages.size() == evalExpressions.size() + 1);
```


================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.cpp:124
+
+  for (size_t i = 0; i < bp->evalExpressions.size(); ++i) {
+    const llvm::StringRef &expr = bp->evalExpressions[i];
----------------
clayborg wrote:
> If we use a single vector of LogMessagePart objects, this code becomes simpler:
> ```
> for (const LogMessagePart &part: bp->logMessageParts) {
>   if (part.is_expr) {
>     lldb::SBValue value = frame.EvaluateExpression(part.text.str().c_str());
>     const char *expr_result = value.GetValue();
>     if (expr_result)
>       output += expr_result;
>    } else {
>      output += part.text.str();
>    }
> }
Can you elaborate how is the suggested code simpler than what we have here? The only difference seems to be that the suggested code added a if...else branch while the code here does not branch? Seems not much difference.


================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.cpp:128
+    lldb::SBValue value = frame.EvaluateExpression(expr.str().c_str());
+    output += value.GetValue();
+    output += bp->rawTextMessages[i + 1];
----------------
clayborg wrote:
> This will crash if "value.GetValue()" returns NULL.
Good catch, will handle null.


================
Comment at: lldb/tools/lldb-vscode/BreakpointBase.h:31-32
   std::string logMessage;
+  std::vector<llvm::StringRef> rawTextMessages;
+  std::vector<llvm::StringRef> evalExpressions;
   // The LLDB breakpoint associated wit this source breakpoint
----------------
clayborg wrote:
> clayborg wrote:
> > How do we know what order to emit things with here between "rawTextMessages" and "evalExpressions" in the breakpoint hit callback?
> Seems like it would be easier to understand the code if we used:
> ```
> struct LogMessagePart {
>   llvm::StringRef text;
>   bool is_expr;
> };
> std::vector<LogMessagePart> logMessageParts;
> ```
> 
> Right now you have two arrays and it isn't clear that each of these arrays must be the same size, or which one would be emitted first.
Sorry, I think I forgot to document this. The format of the logMessage is parsed as:
```
prefixRawText/expression1/rawText1/expression2/rawText2..../expressionN/rawTextN
```
Basically rawTextMessages wraps expressions. This is enforced by invariant `assert(rawTextMessages.size() == evalExpressions.size() + 1);` in the cpp file. 
Let me add this as documentation.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2084-2087
+        g_vsc.source_breakpoints[path][src_bp.line] = src_bp;
+        SourceBreakpoint &new_bp = g_vsc.source_breakpoints[path][src_bp.line];
+        new_bp.SetBreakpoint(path.data());
+        AppendBreakpoint(new_bp.bp, response_breakpoints, path, new_bp.line);
----------------
clayborg wrote:
> Why was this changed? I seemed clearer before this change and I can't see anything that is different here?
This change is required because BreakpointBase::SetBreakpoint() will trigger BreakpointBase::SetLogMessage() which stores this pointer for `BreakpointBase::BreakpointHitCallback` callback access now. This means SetBreakpoint() can't be called on Breakpoint object on stack (src_bp is on stack). The new code solves the issue by storing stack object src_bp into `g_vsc.source_breakpoints` (on heap) then call SetBreakpoint() on the heap object.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2312-2314
+    FunctionBreakpoint &new_bp = g_vsc.function_breakpoints[pair.first()];
+    new_bp.SetBreakpoint();
+    AppendBreakpoint(new_bp.bp, response_breakpoints);
----------------
clayborg wrote:
> Why was this changed? Revert?
The same see my comment above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127702