This is an archive of the discontinued LLVM Phabricator instance.

Support logpoints in lldb-vscode
ClosedPublic

Authored by yinghuitan on Jun 13 2022, 3:24 PM.

Details

Summary

This patch implements VSCode DAP logpoints feature (also called tracepoint
in VS debugger).
This will provide a convenient way for user to do printf style logging
debugging without pausing debuggee.

Diff Detail

Event Timeline

yinghuitan created this revision.Jun 13 2022, 3:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2022, 3:24 PM
yinghuitan requested review of this revision.Jun 13 2022, 3:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2022, 3:24 PM
yinghuitan edited the summary of this revision. (Show Details)Jun 13 2022, 3:26 PM
clayborg requested changes to this revision.Jun 13 2022, 4:11 PM
clayborg added inline comments.
lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_logpoints.py
61

Any reason we have an empty dictionary at the end here?

121

Any reason we have an empty dictionary at the end here?

lldb/tools/lldb-vscode/BreakpointBase.cpp
46

Might be easier to use a std::map<int, int> here instead of a vector of pairs?

91

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)?

124

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();
   }
}
128

This will crash if "value.GetValue()" returns NULL.

lldb/tools/lldb-vscode/BreakpointBase.h
35–36

How do we know what order to emit things with here between "rawTextMessages" and "evalExpressions" in the breakpoint hit callback?

35–36

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.

lldb/tools/lldb-vscode/lldb-vscode.cpp
2084–2088

Why was this changed? I seemed clearer before this change and I can't see anything that is different here?

2312–2314

Why was this changed? Revert?

This revision now requires changes to proceed.Jun 13 2022, 4:11 PM
clayborg added inline comments.Jun 13 2022, 4:23 PM
lldb/tools/lldb-vscode/BreakpointBase.cpp
126

I would definitely try an local variable paths first via:

lldb::SBValue value = frame.GetValueForVariablePath(part.text.str().c_str(), lldb::eDynamicDontRunTarget);

before doing the evaluate expression due to performance which will be important in this case.

yinghuitan added inline comments.Jun 14 2022, 10:22 AM
lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_logpoints.py
61

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.

121

{} means no logMessage for second breakpoint (after_loop_line).

lldb/tools/lldb-vscode/BreakpointBase.cpp
46

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.

91

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);
124

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.

128

Good catch, will handle null.

lldb/tools/lldb-vscode/BreakpointBase.h
35–36

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.

lldb/tools/lldb-vscode/lldb-vscode.cpp
2084–2088

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.

2312–2314

The same see my comment above.

I would like to get to one array solution of LogMessagePart entries and avoid having two arrays of strings that need to stay in sync (rawTextMessages and evalExpressions). Simpler to understand and no need to document the interdependence of rawTextMessages and evalExpressions.

lldb/tools/lldb-vscode/BreakpointBase.cpp
91

I would like to avoid having two arrays that need to stay in sync here if possible. See comment on using just a single array of "LogMessagePart" entries. Simpler code without the need to document that rawTextMessages and evalExpressions need to be in some specific order.

124

There is only one array and there is no need to document that the usage of two arrays and their codependence, such as you are doing with invariants above:

assert(bp->rawTextMessages.size() == bp->evalExpressions.size() + 1);

Using a single structured LogMessagePart per suggestion.

clayborg accepted this revision.Jun 20 2022, 3:32 PM
This revision is now accepted and ready to land.Jun 20 2022, 3:32 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jun 20 2022, 4:37 PM

Looks like this might not build: http://45.33.8.238/linux/79194/step_4.txt

Please take a look and revert for now if it takes a while to fix.

Sorry, I was fooled by the buildbot which says everything is green. Working on a fix for the build break now.

cmtice added a subscriber: cmtice.Jun 24 2022, 12:29 PM

Just FYI, this commit appears to cause lldb//test/API/commands/target/auto-install-main-executable/TestAutoInstallMainExecutable.py to fail. The error I'm seeing is:

Traceback (most recent call last):

File "../lldb/test/API/commands/target/auto-install-main-executable/TestAutoInstallMainExecutable.py", line 72, in test_target_auto_install_main_executable
  self.expect("process launch", substrs=["exited with status = 74"])
File ".../lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2295, in expect
  self.runCmd(
File ".../lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1998, in runCmd
  self.assertTrue(self.res.Succeeded(),

AssertionError: False is not True : Command 'process launch
Error output:
error: failed to get reply to handshake packet within timeout of 0.0 seconds
' did not return successfully

@cmtice, are you sure the failure is caused by/related with this patch? This is a pure lldb-vscode change which is not used by normal lldb. Also, no mention of code in this patch in the error message.

I apologize; I was looking at several commits, and I accidentally added my comment to the wrong one. No, this commit is not the one that caused my problem (I am very sorry about the confusion).