Skip to content

[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

Merged
merged 2 commits into from
Apr 25, 2025

Conversation

da-viper
Copy link
Contributor

Based on the DAP specification.
The output categories stdout and stderr should only be used for the debuggee's stdout and stderr.

 /**
     * 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 ?

@llvmbot
Copy link
Member

llvmbot commented Apr 23, 2025

@llvm/pr-subscribers-lldb

Author: Ebuka Ezike (da-viper)

Changes

Based on the DAP specification.
The output categories stdout and stderr should only be used for the debuggee's stdout and stderr.

 /**
     * 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:

  • (modified) lldb/tools/lldb-dap/DAP.cpp (+2-2)
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;
 

Copy link
Member

@JDevlieghere JDevlieghere left a 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.

@da-viper
Copy link
Contributor Author

I think that may not apply to LLDB-dap.

Since dap uses the SBAPI it sends the debugee output to the client here.

// Grab any STDOUT and STDERR from the process and send it up to VS Code
// via an "output" event to the "stdout" and "stderr" categories.
void SendStdOutStdErr(DAP &dap, lldb::SBProcess &process) {
char buffer[OutputBufferSize];
size_t count;
while ((count = process.GetSTDOUT(buffer, sizeof(buffer))) > 0)
dap.SendOutput(OutputType::Stdout, llvm::StringRef(buffer, count));
while ((count = process.GetSTDERR(buffer, sizeof(buffer))) > 0)
dap.SendOutput(OutputType::Stderr, llvm::StringRef(buffer, count));
}

Copy link
Member

@JDevlieghere JDevlieghere left a 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>
@da-viper da-viper force-pushed the redirect-lldb-msg-to-the-correct-cateory branch from 2011eec to 5834062 Compare April 25, 2025 10:50
@da-viper da-viper merged commit 2d1b5a2 into llvm:main Apr 25, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants