From 9eddc8b9bf4e4e0b01e2ecc90a71c4b3b4e9c8af Mon Sep 17 00:00:00 2001 From: Adrian Prantl Date: Mon, 14 Oct 2024 16:29:26 -0700 Subject: [PATCH] [lldb] Expose structured command diagnostics via the SBAPI. (#112109) This allows IDEs to render LLDB expression diagnostics to their liking without relying on characterprecise ASCII art from LLDB. It is exposed as a versioned SBStructuredData object, since it is expected that this may need to be tweaked based on actual usage. --- lldb/include/lldb/API/SBCommandReturnObject.h | 1 + lldb/include/lldb/API/SBStructuredData.h | 2 + .../lldb/Interpreter/CommandReturnObject.h | 13 ++- lldb/source/API/SBCommandReturnObject.cpp | 11 +++ .../Commands/CommandObjectDWIMPrint.cpp | 2 +- .../Commands/CommandObjectExpression.cpp | 42 +++------- .../source/Interpreter/CommandInterpreter.cpp | 23 +++-- .../Interpreter/CommandReturnObject.cpp | 84 +++++++++++++++---- .../diagnostics/TestExprDiagnostics.py | 81 ++++++++---------- .../Shell/Commands/Inputs/multiline-expr.txt | 6 ++ .../Commands/command-expr-diagnostics.test | 32 +++++++ 11 files changed, 190 insertions(+), 107 deletions(-) create mode 100644 lldb/test/Shell/Commands/Inputs/multiline-expr.txt create mode 100644 lldb/test/Shell/Commands/command-expr-diagnostics.test diff --git a/lldb/include/lldb/API/SBCommandReturnObject.h b/lldb/include/lldb/API/SBCommandReturnObject.h index f96384a4710b16c..e8e20a3f3016b82 100644 --- a/lldb/include/lldb/API/SBCommandReturnObject.h +++ b/lldb/include/lldb/API/SBCommandReturnObject.h @@ -45,6 +45,7 @@ class LLDB_API SBCommandReturnObject { const char *GetOutput(); const char *GetError(); + SBStructuredData GetErrorData(); #ifndef SWIG LLDB_DEPRECATED_FIXME("Use PutOutput(SBFile) or PutOutput(FileSP)", diff --git a/lldb/include/lldb/API/SBStructuredData.h b/lldb/include/lldb/API/SBStructuredData.h index fc6e1ec95c7b86e..ccdd12cab94b2fb 100644 --- a/lldb/include/lldb/API/SBStructuredData.h +++ b/lldb/include/lldb/API/SBStructuredData.h @@ -9,6 +9,7 @@ #ifndef LLDB_API_SBSTRUCTUREDDATA_H #define LLDB_API_SBSTRUCTUREDDATA_H +#include "lldb/API/SBCommandReturnObject.h" #include "lldb/API/SBDefines.h" #include "lldb/API/SBModule.h" #include "lldb/API/SBScriptObject.h" @@ -110,6 +111,7 @@ class SBStructuredData { protected: friend class SBAttachInfo; + friend class SBCommandReturnObject; friend class SBLaunchInfo; friend class SBDebugger; friend class SBTarget; diff --git a/lldb/include/lldb/Interpreter/CommandReturnObject.h b/lldb/include/lldb/Interpreter/CommandReturnObject.h index eda841869ba4329..a491a6c1535b11f 100644 --- a/lldb/include/lldb/Interpreter/CommandReturnObject.h +++ b/lldb/include/lldb/Interpreter/CommandReturnObject.h @@ -13,6 +13,7 @@ #include "lldb/Utility/DiagnosticsRendering.h" #include "lldb/Utility/StreamString.h" #include "lldb/Utility/StreamTee.h" +#include "lldb/Utility/StructuredData.h" #include "lldb/lldb-private.h" #include "llvm/ADT/StringRef.h" @@ -31,7 +32,7 @@ class CommandReturnObject { ~CommandReturnObject() = default; /// Format any inline diagnostics with an indentation of \c indent. - llvm::StringRef GetInlineDiagnosticString(unsigned indent); + std::string GetInlineDiagnosticString(unsigned indent); llvm::StringRef GetOutputString() { lldb::StreamSP stream_sp(m_out_stream.GetStreamAtIndex(eStreamStringIndex)); @@ -40,7 +41,13 @@ class CommandReturnObject { return llvm::StringRef(); } - llvm::StringRef GetErrorString(); + /// Return the errors as a string. + /// + /// If \c with_diagnostics is true, all diagnostics are also + /// rendered into the string. Otherwise the expectation is that they + /// are fetched with \ref GetInlineDiagnosticString(). + std::string GetErrorString(bool with_diagnostics = true); + StructuredData::ObjectSP GetErrorData(); Stream &GetOutputStream() { // Make sure we at least have our normal string stream output stream @@ -168,7 +175,6 @@ class CommandReturnObject { StreamTee m_out_stream; StreamTee m_err_stream; std::vector m_diagnostics; - StreamString m_diag_stream; std::optional m_diagnostic_indent; lldb::ReturnStatus m_status = lldb::eReturnStatusStarted; @@ -178,6 +184,7 @@ class CommandReturnObject { /// If true, then the input handle from the debugger will be hooked up. bool m_interactive = true; + bool m_colors; }; } // namespace lldb_private diff --git a/lldb/source/API/SBCommandReturnObject.cpp b/lldb/source/API/SBCommandReturnObject.cpp index a94eff75ffcb9e9..9df8aa48b993664 100644 --- a/lldb/source/API/SBCommandReturnObject.cpp +++ b/lldb/source/API/SBCommandReturnObject.cpp @@ -11,6 +11,8 @@ #include "lldb/API/SBError.h" #include "lldb/API/SBFile.h" #include "lldb/API/SBStream.h" +#include "lldb/API/SBStructuredData.h" +#include "lldb/Core/StructuredDataImpl.h" #include "lldb/Interpreter/CommandReturnObject.h" #include "lldb/Utility/ConstString.h" #include "lldb/Utility/Instrumentation.h" @@ -96,6 +98,15 @@ const char *SBCommandReturnObject::GetError() { return output.AsCString(/*value_if_empty*/ ""); } +SBStructuredData SBCommandReturnObject::GetErrorData() { + LLDB_INSTRUMENT_VA(this); + + StructuredData::ObjectSP data(ref().GetErrorData()); + SBStructuredData sb_data; + sb_data.m_impl_up->SetObjectSP(data); + return sb_data; +} + size_t SBCommandReturnObject::GetOutputSize() { LLDB_INSTRUMENT_VA(this); diff --git a/lldb/source/Commands/CommandObjectDWIMPrint.cpp b/lldb/source/Commands/CommandObjectDWIMPrint.cpp index beff5f4bbcc5b8b..ddf0aca25fb370b 100644 --- a/lldb/source/Commands/CommandObjectDWIMPrint.cpp +++ b/lldb/source/Commands/CommandObjectDWIMPrint.cpp @@ -194,7 +194,7 @@ void CommandObjectDWIMPrint::DoExecute(StringRef command, // Record the position of the expression in the command. std::optional indent; if (fixed_expression.empty()) { - size_t pos = m_original_command.find(expr); + size_t pos = m_original_command.rfind(expr); if (pos != llvm::StringRef::npos) indent = pos; } diff --git a/lldb/source/Commands/CommandObjectExpression.cpp b/lldb/source/Commands/CommandObjectExpression.cpp index 8effb1a5988370f..13491b5c794425d 100644 --- a/lldb/source/Commands/CommandObjectExpression.cpp +++ b/lldb/source/Commands/CommandObjectExpression.cpp @@ -485,35 +485,8 @@ bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr, result.SetStatus(eReturnStatusSuccessFinishResult); } else { - // Retrieve the diagnostics. - std::vector details; - llvm::consumeError(llvm::handleErrors( - result_valobj_sp->GetError().ToError(), - [&](DiagnosticError &error) { details = error.GetDetails(); })); - // Find the position of the expression in the command. - std::optional expr_pos; - size_t nchar = m_original_command.find(expr); - if (nchar != std::string::npos) - expr_pos = nchar + GetDebugger().GetPrompt().size(); - - if (!details.empty()) { - bool show_inline = - GetDebugger().GetShowInlineDiagnostics() && !expr.contains('\n'); - RenderDiagnosticDetails(error_stream, expr_pos, show_inline, details); - } else { - const char *error_cstr = result_valobj_sp->GetError().AsCString(); - llvm::StringRef error(error_cstr); - if (!error.empty()) { - if (!error.starts_with("error:")) - error_stream << "error: "; - error_stream << error; - if (!error.ends_with('\n')) - error_stream.EOL(); - } else { - error_stream << "error: unknown error\n"; - } - } result.SetStatus(eReturnStatusFailed); + result.SetError(result_valobj_sp->GetError().ToError()); } } } else { @@ -533,10 +506,13 @@ void CommandObjectExpression::IOHandlerInputComplete(IOHandler &io_handler, CommandReturnObject return_obj( GetCommandInterpreter().GetDebugger().GetUseColor()); EvaluateExpression(line.c_str(), *output_sp, *error_sp, return_obj); + if (output_sp) output_sp->Flush(); - if (error_sp) + if (error_sp) { + *error_sp << return_obj.GetErrorString(); error_sp->Flush(); + } } bool CommandObjectExpression::IOHandlerIsInputComplete(IOHandler &io_handler, @@ -679,6 +655,14 @@ void CommandObjectExpression::DoExecute(llvm::StringRef command, } } + // Previously the indent was set up for diagnosing command line + // parsing errors. Now point it to the expression. + std::optional indent; + size_t pos = m_original_command.rfind(expr); + if (pos != llvm::StringRef::npos) + indent = pos; + result.SetDiagnosticIndent(indent); + Target &target = GetTarget(); if (EvaluateExpression(expr, result.GetOutputStream(), result.GetErrorStream(), result)) { diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp index 19bb420f2116dce..bfac3f4fea8d40d 100644 --- a/lldb/source/Interpreter/CommandInterpreter.cpp +++ b/lldb/source/Interpreter/CommandInterpreter.cpp @@ -2636,20 +2636,18 @@ void CommandInterpreter::HandleCommands(const StringList &commands, } if (!success || !tmp_result.Succeeded()) { - llvm::StringRef error_msg = tmp_result.GetErrorString(); + std::string error_msg = tmp_result.GetErrorString(); if (error_msg.empty()) error_msg = ".\n"; if (options.GetStopOnError()) { - result.AppendErrorWithFormat( - "Aborting reading of commands after command #%" PRIu64 - ": '%s' failed with %s", - (uint64_t)idx, cmd, error_msg.str().c_str()); + result.AppendErrorWithFormatv("Aborting reading of commands after " + "command #{0}: '{1}' failed with {2}", + (uint64_t)idx, cmd, error_msg); m_debugger.SetAsyncExecution(old_async_execution); return; } else if (options.GetPrintResults()) { - result.AppendMessageWithFormat( - "Command #%" PRIu64 " '%s' failed with %s", (uint64_t)idx + 1, cmd, - error_msg.str().c_str()); + result.AppendMessageWithFormatv("Command #{0} '{1}' failed with {2}", + (uint64_t)idx + 1, cmd, error_msg); } } @@ -3187,11 +3185,12 @@ void CommandInterpreter::IOHandlerInputComplete(IOHandler &io_handler, io_handler.GetFlags().Test(eHandleCommandFlagPrintResult)) || io_handler.GetFlags().Test(eHandleCommandFlagPrintErrors)) { // Display any inline diagnostics first. - if (!result.GetImmediateErrorStream() && - GetDebugger().GetShowInlineDiagnostics()) { + const bool inline_diagnostics = !result.GetImmediateErrorStream() && + GetDebugger().GetShowInlineDiagnostics(); + if (inline_diagnostics) { unsigned prompt_len = m_debugger.GetPrompt().size(); if (auto indent = result.GetDiagnosticIndent()) { - llvm::StringRef diags = + std::string diags = result.GetInlineDiagnosticString(prompt_len + *indent); PrintCommandOutput(io_handler, diags, true); } @@ -3207,7 +3206,7 @@ void CommandInterpreter::IOHandlerInputComplete(IOHandler &io_handler, // Now emit the command error text from the command we just executed. if (!result.GetImmediateErrorStream()) { - llvm::StringRef error = result.GetErrorString(); + std::string error = result.GetErrorString(!inline_diagnostics); PrintCommandOutput(io_handler, error, false); } } diff --git a/lldb/source/Interpreter/CommandReturnObject.cpp b/lldb/source/Interpreter/CommandReturnObject.cpp index 28f76dc0c40f946..94f5ff608b2aea6 100644 --- a/lldb/source/Interpreter/CommandReturnObject.cpp +++ b/lldb/source/Interpreter/CommandReturnObject.cpp @@ -42,7 +42,7 @@ static void DumpStringToStreamWithNewline(Stream &strm, const std::string &s) { } CommandReturnObject::CommandReturnObject(bool colors) - : m_out_stream(colors), m_err_stream(colors), m_diag_stream(colors) {} + : m_out_stream(colors), m_err_stream(colors), m_colors(colors) {} void CommandReturnObject::AppendErrorWithFormat(const char *format, ...) { SetStatus(eReturnStatusFailed); @@ -123,30 +123,79 @@ void CommandReturnObject::SetError(llvm::Error error) { } } -llvm::StringRef -CommandReturnObject::GetInlineDiagnosticString(unsigned indent) { - RenderDiagnosticDetails(m_diag_stream, indent, true, m_diagnostics); +std::string CommandReturnObject::GetInlineDiagnosticString(unsigned indent) { + StreamString diag_stream(m_colors); + RenderDiagnosticDetails(diag_stream, indent, true, m_diagnostics); // Duplex the diagnostics to the secondary stream (but not inlined). - if (auto stream_sp = m_err_stream.GetStreamAtIndex(eStreamStringIndex)) + if (auto stream_sp = m_err_stream.GetStreamAtIndex(eImmediateStreamIndex)) RenderDiagnosticDetails(*stream_sp, std::nullopt, false, m_diagnostics); - // Clear them so GetErrorData() doesn't render them again. - m_diagnostics.clear(); - return m_diag_stream.GetString(); + return diag_stream.GetString().str(); } -llvm::StringRef CommandReturnObject::GetErrorString() { - // Diagnostics haven't been fetched; render them now (not inlined). - if (!m_diagnostics.empty()) { - RenderDiagnosticDetails(GetErrorStream(), std::nullopt, false, - m_diagnostics); - m_diagnostics.clear(); - } +std::string CommandReturnObject::GetErrorString(bool with_diagnostics) { + StreamString stream(m_colors); + if (with_diagnostics) + RenderDiagnosticDetails(stream, std::nullopt, false, m_diagnostics); lldb::StreamSP stream_sp(m_err_stream.GetStreamAtIndex(eStreamStringIndex)); if (stream_sp) - return std::static_pointer_cast(stream_sp)->GetString(); - return llvm::StringRef(); + stream << std::static_pointer_cast(stream_sp)->GetString(); + return stream.GetString().str(); +} + +StructuredData::ObjectSP CommandReturnObject::GetErrorData() { + auto make_array = []() { return std::make_unique(); }; + auto make_bool = [](bool b) { + return std::make_unique(b); + }; + auto make_dict = []() { + return std::make_unique(); + }; + auto make_int = [](unsigned i) { + return std::make_unique(i); + }; + auto make_string = [](llvm::StringRef s) { + return std::make_unique(s); + }; + auto dict_up = make_dict(); + dict_up->AddItem("version", make_int(1)); + auto array_up = make_array(); + for (const DiagnosticDetail &diag : m_diagnostics) { + auto detail_up = make_dict(); + if (auto &sloc = diag.source_location) { + auto sloc_up = make_dict(); + sloc_up->AddItem("file", make_string(sloc->file.GetPath())); + sloc_up->AddItem("line", make_int(sloc->line)); + sloc_up->AddItem("length", make_int(sloc->length)); + sloc_up->AddItem("hidden", make_bool(sloc->hidden)); + sloc_up->AddItem("in_user_input", make_bool(sloc->in_user_input)); + detail_up->AddItem("source_location", std::move(sloc_up)); + } + llvm::StringRef severity = "unknown"; + switch (diag.severity) { + case lldb::eSeverityError: + severity = "error"; + break; + case lldb::eSeverityWarning: + severity = "warning"; + break; + case lldb::eSeverityInfo: + severity = "note"; + break; + } + detail_up->AddItem("severity", make_string(severity)); + detail_up->AddItem("message", make_string(diag.message)); + detail_up->AddItem("rendered", make_string(diag.rendered)); + array_up->AddItem(std::move(detail_up)); + } + dict_up->AddItem("details", std::move(array_up)); + if (auto stream_sp = m_err_stream.GetStreamAtIndex(eStreamStringIndex)) { + auto text = std::static_pointer_cast(stream_sp)->GetString(); + if (!text.empty()) + dict_up->AddItem("text", make_string(text)); + } + return dict_up; } // Similar to AppendError, but do not prepend 'Status: ' to message, and don't @@ -179,6 +228,7 @@ void CommandReturnObject::Clear() { stream_sp = m_err_stream.GetStreamAtIndex(eStreamStringIndex); if (stream_sp) static_cast(stream_sp.get())->Clear(); + m_diagnostics.clear(); m_status = eReturnStatusStarted; m_did_change_process_state = false; m_suppress_immediate_output = false; diff --git a/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py b/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py index c63065a3f345a98..fac562edf9ece03 100644 --- a/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py +++ b/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py @@ -184,53 +184,44 @@ def test_source_locations_from_objc_modules(self): # the first argument are probably stable enough that this test can check for them. self.assertIn("void NSLog(NSString *format", value.GetError().GetCString()) - def test_command_expr_formatting(self): - """Test that the source and caret positions LLDB prints are correct""" + def test_command_expr_sbdata(self): + """Test the structured diagnostics data""" self.build() (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint( self, "// Break here", self.main_source_spec ) - frame = thread.GetFrameAtIndex(0) - self.expect("settings set show-inline-diagnostics true") - - def check(input_ref): - self.expect(input_ref[0], error=True, substrs=input_ref[1:]) - - check( - [ - "expression -- a+b", - " ^ ^", - " | error: use of undeclared identifier 'b'", - " error: use of undeclared identifier 'a'", - ] - ) - - check( - [ - "expr -- a", - " ^", - " error: use of undeclared identifier 'a'", - ] - ) - check( - [ - "expr -i 0 -o 0 -- a", - " ^", - " error: use of undeclared identifier 'a'", - ] - ) - - self.expect( - "expression --top-level -- template T FOO(T x) { return x/2;}" - ) - check( - [ - 'expression -- FOO("")', - " ^", - " note: in instantiation of function template specialization 'FOO' requested here", - "error: