-
Notifications
You must be signed in to change notification settings - Fork 13.3k
[lldb][lldb-dap] Redirect LLDB's messages to the right output category. #137002
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[lldb][lldb-dap] Redirect LLDB's messages to the right output category. #137002
Conversation
@llvm/pr-subscribers-lldb Author: Ebuka Ezike (da-viper) ChangesBased on the DAP specification. /**
* The output category. If not specified or if the category is not
* understood by the client, `console` is assumed.
* Values:
* 'console': Show the output in the client's default message UI, e.g. a
* 'debug console'. This category should only be used for informational
* output from the debugger (as opposed to the debuggee).
* 'important': A hint for the client to show the output in the client's UI
* for important and highly visible information, e.g. as a popup
* notification. This category should only be used for important messages
* from the debugger (as opposed to the debuggee). Since this category value
* is a hint, clients might ignore the hint and assume the `console`
* category.
* 'stdout': Show the output as normal program output from the debuggee.
* 'stderr': Show the output as error program output from the debuggee.
* 'telemetry': Send the output to telemetry instead of showing it to the
* user.
* etc.
*/
category?: 'console' | 'important' | 'stdout' | 'stderr' | 'telemetry' | string; What I am not sure if error should use the important category ? Full diff: https://github.com/llvm/llvm-project/pull/137002.diff 1 Files Affected:
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 597fe3a1e323b..831075af1cd63 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -208,12 +208,12 @@ llvm::Error DAP::ConfigureIO(std::FILE *overrideOut, std::FILE *overrideErr) {
in = lldb::SBFile(std::fopen(DEV_NULL, "r"), /*transfer_ownership=*/true);
if (auto Error = out.RedirectTo(overrideOut, [this](llvm::StringRef output) {
- SendOutput(OutputType::Stdout, output);
+ SendOutput(OutputType::Console, output);
}))
return Error;
if (auto Error = err.RedirectTo(overrideErr, [this](llvm::StringRef output) {
- SendOutput(OutputType::Stderr, output);
+ SendOutput(OutputType::Console, output);
}))
return Error;
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this is correct. LLDB has a notion of an output and error stream, which combines the output from the debuggee, and output from the debugger. Since we can't detangle the two, I think the current implementation most closely matches the spec.
I think that may not apply to LLDB-dap. Since dap uses the SBAPI it sends the debugee output to the client here. llvm-project/lldb/tools/lldb-dap/EventHelper.cpp Lines 205 to 215 in 1b6cbaa
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize lldb-dap was handing the process output separately. In that case, this LGTM. Apologies for the confusion!
Based on the DAP specification. The output categories stdout and stderr should only be used for the debuggee's stdout and stderr. Signed-off-by: Ebuka Ezike <yerimyah1@gmail.com>
2011eec
to
5834062
Compare
Based on the DAP specification.
The output categories stdout and stderr should only be used for the debuggee's stdout and stderr.
What I am not sure if error should use the important category ?