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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? |
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. |
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. | |
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); |
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.
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).
Any reason we have an empty dictionary at the end here?