[lldb] Make GetOutputStreamSP and GetErrorStreamSP protected #127682
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes GetOutputStreamSP and GetErrorStreamSP protected members of Debugger. Users who want to print to the debugger's stream should use GetAsyncOutputStreamSP and GetAsyncErrorStreamSP instead and the few remaining stragglers have been migrated.
|
More context/background in #126630. |
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesThis makes GetOutputStreamSP and GetErrorStreamSP protected members of Debugger. Users who want to print to the debugger's stream should use GetAsyncOutputStreamSP and GetAsyncErrorStreamSP instead and the few remaining stragglers have been migrated. Full diff: https://github.com/llvm/llvm-project/pull/127682.diff 13 Files Affected:
diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index d7751ca045bb2..7f08f3dd26106 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -131,17 +131,13 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
void SetAsyncExecution(bool async);
- lldb::FileSP GetInputFileSP() { return m_input_file_sp; }
-
- lldb::StreamFileSP GetOutputStreamSP() { return m_output_stream_sp; }
-
- lldb::StreamFileSP GetErrorStreamSP() { return m_error_stream_sp; }
-
File &GetInputFile() { return *m_input_file_sp; }
- File &GetOutputFile() { return m_output_stream_sp->GetFile(); }
+ lldb::FileSP GetInputFileSP() { return m_input_file_sp; }
+
+ lldb::FileSP GetOutputFileSP() { return m_output_stream_sp->GetFileSP(); }
- File &GetErrorFile() { return m_error_stream_sp->GetFile(); }
+ lldb::FileSP GetErrorFileSP() { return m_error_stream_sp->GetFileSP(); }
repro::DataRecorder *GetInputRecorder();
@@ -649,6 +645,14 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
void PrintProgress(const ProgressEventData &data);
+ /// Except for Debugger and IOHandler, GetOutputStreamSP and GetErrorStreamSP
+ /// should not be used directly. Use GetAsyncOutputStream and
+ /// GetAsyncErrorStream instead.
+ /// @{
+ lldb::StreamFileSP GetOutputStreamSP() { return m_output_stream_sp; }
+ lldb::StreamFileSP GetErrorStreamSP() { return m_error_stream_sp; }
+ /// @}
+
void PushIOHandler(const lldb::IOHandlerSP &reader_sp,
bool cancel_top_handler = true);
diff --git a/lldb/include/lldb/Target/ThreadPlanTracer.h b/lldb/include/lldb/Target/ThreadPlanTracer.h
index a6fd2f031dc22..7c45e213f94f1 100644
--- a/lldb/include/lldb/Target/ThreadPlanTracer.h
+++ b/lldb/include/lldb/Target/ThreadPlanTracer.h
@@ -56,7 +56,7 @@ class ThreadPlanTracer {
Process &m_process;
lldb::tid_t m_tid;
- Stream *GetLogStream();
+ lldb::StreamSP GetLogStreamSP();
virtual void Log();
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index bf19d2ff8333c..e646b09e05852 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -509,14 +509,14 @@ SBFile SBDebugger::GetInputFile() {
FILE *SBDebugger::GetOutputFileHandle() {
LLDB_INSTRUMENT_VA(this);
if (m_opaque_sp)
- return m_opaque_sp->GetOutputStreamSP()->GetFile().GetStream();
+ return m_opaque_sp->GetOutputFileSP()->GetStream();
return nullptr;
}
SBFile SBDebugger::GetOutputFile() {
LLDB_INSTRUMENT_VA(this);
if (m_opaque_sp)
- return SBFile(m_opaque_sp->GetOutputStreamSP()->GetFileSP());
+ return SBFile(m_opaque_sp->GetOutputFileSP());
return SBFile();
}
@@ -524,7 +524,7 @@ FILE *SBDebugger::GetErrorFileHandle() {
LLDB_INSTRUMENT_VA(this);
if (m_opaque_sp)
- return m_opaque_sp->GetErrorStreamSP()->GetFile().GetStream();
+ return m_opaque_sp->GetErrorFileSP()->GetStream();
return nullptr;
}
@@ -532,7 +532,7 @@ SBFile SBDebugger::GetErrorFile() {
LLDB_INSTRUMENT_VA(this);
SBFile file;
if (m_opaque_sp)
- return SBFile(m_opaque_sp->GetErrorStreamSP()->GetFileSP());
+ return SBFile(m_opaque_sp->GetErrorFileSP());
return SBFile();
}
@@ -573,8 +573,8 @@ void SBDebugger::HandleCommand(const char *command) {
sb_interpreter.HandleCommand(command, result, false);
- result.PutError(m_opaque_sp->GetErrorStreamSP()->GetFileSP());
- result.PutOutput(m_opaque_sp->GetOutputStreamSP()->GetFileSP());
+ result.PutError(m_opaque_sp->GetErrorFileSP());
+ result.PutOutput(m_opaque_sp->GetOutputFileSP());
if (!m_opaque_sp->GetAsyncExecution()) {
SBProcess process(GetCommandInterpreter().GetProcess());
diff --git a/lldb/source/Commands/CommandObjectGUI.cpp b/lldb/source/Commands/CommandObjectGUI.cpp
index b56e49b073b03..8630171bae9d1 100644
--- a/lldb/source/Commands/CommandObjectGUI.cpp
+++ b/lldb/source/Commands/CommandObjectGUI.cpp
@@ -28,10 +28,10 @@ void CommandObjectGUI::DoExecute(Args &args, CommandReturnObject &result) {
#if LLDB_ENABLE_CURSES
Debugger &debugger = GetDebugger();
- File &input = debugger.GetInputFile();
- File &output = debugger.GetOutputFile();
- if (input.GetStream() && output.GetStream() && input.GetIsRealTerminal() &&
- input.GetIsInteractive()) {
+ FileSP input_sp = debugger.GetInputFileSP();
+ FileSP output_sp = debugger.GetOutputFileSP();
+ if (input_sp->GetStream() && output_sp->GetStream() &&
+ input_sp->GetIsRealTerminal() && input_sp->GetIsInteractive()) {
IOHandlerSP io_handler_sp(new IOHandlerCursesGUI(debugger));
if (io_handler_sp)
debugger.RunIOHandlerAsync(io_handler_sp);
diff --git a/lldb/source/Commands/CommandObjectLog.cpp b/lldb/source/Commands/CommandObjectLog.cpp
index 5fb2dfaab8de0..17efae189b05e 100644
--- a/lldb/source/Commands/CommandObjectLog.cpp
+++ b/lldb/source/Commands/CommandObjectLog.cpp
@@ -394,7 +394,8 @@ class CommandObjectLogDump : public CommandObjectParsed {
(*file)->GetDescriptor(), /*shouldClose=*/true);
} else {
stream_up = std::make_unique<llvm::raw_fd_ostream>(
- GetDebugger().GetOutputFile().GetDescriptor(), /*shouldClose=*/false);
+ GetDebugger().GetOutputFileSP()->GetDescriptor(),
+ /*shouldClose=*/false);
}
const std::string channel = std::string(args[0].ref());
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 18569e155b517..18cdec4e0af73 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -947,7 +947,7 @@ Debugger::Debugger(lldb::LogOutputCallback log_callback, void *baton)
if (term && !strcmp(term, "dumb"))
SetUseColor(false);
// Turn off use-color if we don't write to a terminal with color support.
- if (!GetOutputFile().GetIsTerminalWithColors())
+ if (!GetOutputFileSP()->GetIsTerminalWithColors())
SetUseColor(false);
if (Diagnostics::Enabled()) {
@@ -1678,7 +1678,7 @@ bool Debugger::EnableLog(llvm::StringRef channel,
LLDB_LOG_OPTION_PREPEND_TIMESTAMP | LLDB_LOG_OPTION_PREPEND_THREAD_NAME;
} else if (log_file.empty()) {
log_handler_sp =
- CreateLogHandler(log_handler_kind, GetOutputFile().GetDescriptor(),
+ CreateLogHandler(log_handler_kind, GetOutputFileSP()->GetDescriptor(),
/*should_close=*/false, buffer_size);
} else {
auto pos = m_stream_handlers.find(log_file);
@@ -2111,8 +2111,8 @@ void Debugger::HandleProgressEvent(const lldb::EventSP &event_sp) {
// Determine whether the current output file is an interactive terminal with
// color support. We assume that if we support ANSI escape codes we support
// vt100 escape codes.
- File &file = GetOutputFile();
- if (!file.GetIsInteractive() || !file.GetIsTerminalWithColors())
+ FileSP file_sp = GetOutputFileSP();
+ if (!file_sp->GetIsInteractive() || !file_sp->GetIsTerminalWithColors())
return;
StreamSP output = GetAsyncOutputStream();
diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp
index acdec84a1689b..5346d5a2d162a 100644
--- a/lldb/source/Interpreter/CommandInterpreter.cpp
+++ b/lldb/source/Interpreter/CommandInterpreter.cpp
@@ -2837,8 +2837,8 @@ void CommandInterpreter::HandleCommandsFromFile(
}
if (flags & eHandleCommandFlagPrintResult) {
- debugger.GetOutputFile().Printf("Executing commands in '%s'.\n",
- cmd_file_path.c_str());
+ debugger.GetOutputFileSP()->Printf("Executing commands in '%s'.\n",
+ cmd_file_path.c_str());
}
// Used for inheriting the right settings when "command source" might
diff --git a/lldb/source/Interpreter/ScriptInterpreter.cpp b/lldb/source/Interpreter/ScriptInterpreter.cpp
index 8d10e5de01225..a392d5777a021 100644
--- a/lldb/source/Interpreter/ScriptInterpreter.cpp
+++ b/lldb/source/Interpreter/ScriptInterpreter.cpp
@@ -245,8 +245,8 @@ ScriptInterpreterIORedirect::ScriptInterpreterIORedirect(
if (outfile_handle)
::setbuf(outfile_handle, nullptr);
- result->SetImmediateOutputFile(debugger.GetOutputStreamSP()->GetFileSP());
- result->SetImmediateErrorFile(debugger.GetErrorStreamSP()->GetFileSP());
+ result->SetImmediateOutputFile(debugger.GetOutputFileSP());
+ result->SetImmediateErrorFile(debugger.GetErrorFileSP());
}
}
diff --git a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
index cff44b588e26e..1d4cda6c046b7 100644
--- a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -1193,7 +1193,7 @@ bool DynamicLoaderDarwinKernel::ReadKextSummaryHeader() {
m_kext_summary_header.version = data.GetU32(&offset);
if (m_kext_summary_header.version > 128) {
lldb::StreamSP s =
- m_process->GetTarget().GetDebugger().GetOutputStreamSP();
+ m_process->GetTarget().GetDebugger().GetAsyncOutputStream();
s->Printf("WARNING: Unable to read kext summary header, got "
"improbable version number %u\n",
m_kext_summary_header.version);
@@ -1208,7 +1208,7 @@ bool DynamicLoaderDarwinKernel::ReadKextSummaryHeader() {
// If we get an improbably large entry_size, we're probably
// getting bad memory.
lldb::StreamSP s =
- m_process->GetTarget().GetDebugger().GetOutputStreamSP();
+ m_process->GetTarget().GetDebugger().GetAsyncOutputStream();
s->Printf("WARNING: Unable to read kext summary header, got "
"improbable entry_size %u\n",
m_kext_summary_header.entry_size);
@@ -1226,7 +1226,7 @@ bool DynamicLoaderDarwinKernel::ReadKextSummaryHeader() {
// If we get an improbably large number of kexts, we're probably
// getting bad memory.
lldb::StreamSP s =
- m_process->GetTarget().GetDebugger().GetOutputStreamSP();
+ m_process->GetTarget().GetDebugger().GetAsyncOutputStream();
s->Printf("WARNING: Unable to read kext summary header, got "
"improbable number of kexts %u\n",
m_kext_summary_header.entry_count);
@@ -1330,7 +1330,8 @@ bool DynamicLoaderDarwinKernel::ParseKextSummaries(
number_of_old_kexts_being_removed == 0)
return true;
- lldb::StreamSP s = m_process->GetTarget().GetDebugger().GetOutputStreamSP();
+ lldb::StreamSP s =
+ m_process->GetTarget().GetDebugger().GetAsyncOutputStream();
if (load_kexts) {
if (number_of_new_kexts_being_added > 0 &&
number_of_old_kexts_being_removed > 0) {
diff --git a/lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp b/lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
index 8c2700cf21de9..c2db3540a797b 100644
--- a/lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
+++ b/lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
@@ -116,8 +116,6 @@ StructuredData::ObjectSP InstrumentationRuntimeUBSan::RetrieveReportData(
if (!frame_sp)
return StructuredData::ObjectSP();
- StreamFileSP Stream = target.GetDebugger().GetOutputStreamSP();
-
EvaluateExpressionOptions options;
options.SetUnwindOnError(true);
options.SetTryAllThreads(true);
diff --git a/lldb/source/Plugins/InstrumentationRuntime/Utility/ReportRetriever.cpp b/lldb/source/Plugins/InstrumentationRuntime/Utility/ReportRetriever.cpp
index 74e0fa7d49f82..d61c59776eee6 100644
--- a/lldb/source/Plugins/InstrumentationRuntime/Utility/ReportRetriever.cpp
+++ b/lldb/source/Plugins/InstrumentationRuntime/Utility/ReportRetriever.cpp
@@ -210,8 +210,8 @@ bool ReportRetriever::NotifyBreakpointHit(ProcessSP process_sp,
InstrumentationRuntimeStopInfo::CreateStopReasonWithInstrumentationData(
*thread_sp, description, report));
- if (StreamFileSP stream_sp = StreamFileSP(
- process_sp->GetTarget().GetDebugger().GetOutputStreamSP()))
+ if (StreamSP stream_sp =
+ process_sp->GetTarget().GetDebugger().GetAsyncOutputStream())
stream_sp->Printf("AddressSanitizer report breakpoint hit. Use 'thread "
"info -s' to get extended information about the "
"report.\n");
diff --git a/lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp b/lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp
index 7e8eee9f5aa4f..6d028e324ee4e 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp
@@ -45,8 +45,8 @@ class IOHandlerLuaInterpreter : public IOHandlerDelegate,
m_script_interpreter(script_interpreter),
m_active_io_handler(active_io_handler) {
llvm::cantFail(m_script_interpreter.GetLua().ChangeIO(
- debugger.GetOutputFile().GetStream(),
- debugger.GetErrorFile().GetStream()));
+ debugger.GetOutputFileSP()->GetStream(),
+ debugger.GetErrorFileSP()->GetStream()));
llvm::cantFail(m_script_interpreter.EnterSession(debugger.GetID()));
}
diff --git a/lldb/source/Target/ThreadPlanTracer.cpp b/lldb/source/Target/ThreadPlanTracer.cpp
index a119bf8589279..ab63cc7f6c223 100644
--- a/lldb/source/Target/ThreadPlanTracer.cpp
+++ b/lldb/source/Target/ThreadPlanTracer.cpp
@@ -27,6 +27,7 @@
#include "lldb/Utility/LLDBLog.h"
#include "lldb/Utility/Log.h"
#include "lldb/Utility/State.h"
+#include "lldb/lldb-forward.h"
using namespace lldb;
using namespace lldb_private;
@@ -41,13 +42,13 @@ ThreadPlanTracer::ThreadPlanTracer(Thread &thread)
: m_process(*thread.GetProcess().get()), m_tid(thread.GetID()),
m_enabled(false), m_stream_sp(), m_thread(nullptr) {}
-Stream *ThreadPlanTracer::GetLogStream() {
+StreamSP ThreadPlanTracer::GetLogStreamSP() {
if (m_stream_sp)
- return m_stream_sp.get();
+ return m_stream_sp;
else {
TargetSP target_sp(GetThread().CalculateTarget());
if (target_sp)
- return target_sp->GetDebugger().GetOutputStreamSP().get();
+ return target_sp->GetDebugger().GetAsyncOutputStream();
}
return nullptr;
}
@@ -65,12 +66,11 @@ void ThreadPlanTracer::Log() {
bool show_frame_index = false;
bool show_fullpaths = false;
- Stream *stream = GetLogStream();
- if (stream) {
- GetThread().GetStackFrameAtIndex(0)->Dump(stream, show_frame_index,
+ if (StreamSP stream_sp = GetLogStreamSP()) {
+ GetThread().GetStackFrameAtIndex(0)->Dump(stream_sp.get(), show_frame_index,
show_fullpaths);
- stream->Printf("\n");
- stream->Flush();
+ stream_sp->Printf("\n");
+ stream_sp->Flush();
}
}
@@ -129,9 +129,9 @@ void ThreadPlanAssemblyTracer::TracingStarted() {
void ThreadPlanAssemblyTracer::TracingEnded() { m_register_values.clear(); }
void ThreadPlanAssemblyTracer::Log() {
- Stream *stream = GetLogStream();
+ StreamSP stream_sp = GetLogStreamSP();
- if (!stream)
+ if (!stream_sp)
return;
RegisterContext *reg_ctx = GetThread().GetRegisterContext().get();
@@ -142,9 +142,10 @@ void ThreadPlanAssemblyTracer::Log() {
uint8_t buffer[16] = {0}; // Must be big enough for any single instruction
addr_valid = m_process.GetTarget().ResolveLoadAddress(pc, pc_addr);
- pc_addr.Dump(stream, &GetThread(), Address::DumpStyleResolvedDescription,
+ pc_addr.Dump(stream_sp.get(), &GetThread(),
+ Address::DumpStyleResolvedDescription,
Address::DumpStyleModuleWithFileAddress);
- stream->PutCString(" ");
+ stream_sp->PutCString(" ");
Disassembler *disassembler = GetDisassembler();
if (disassembler) {
@@ -175,7 +176,7 @@ void ThreadPlanAssemblyTracer::Log() {
instruction_list.GetInstructionAtIndex(0).get();
const FormatEntity::Entry *disassemble_format =
m_process.GetTarget().GetDebugger().GetDisassemblyFormat();
- instruction->Dump(stream, max_opcode_byte_size, show_address,
+ instruction->Dump(stream_sp.get(), max_opcode_byte_size, show_address,
show_bytes, show_control_flow_kind, nullptr, nullptr,
nullptr, disassemble_format, 0);
}
@@ -198,12 +199,12 @@ void ThreadPlanAssemblyTracer::Log() {
if (abi->GetArgumentValues(GetThread(), value_list)) {
for (int arg_index = 0; arg_index < num_args; ++arg_index) {
- stream->Printf(
+ stream_sp->Printf(
"\n\targ[%d]=%llx", arg_index,
value_list.GetValueAtIndex(arg_index)->GetScalar().ULongLong());
if (arg_index + 1 < num_args)
- stream->PutCString(", ");
+ stream_sp->PutCString(", ");
}
}
}
@@ -222,14 +223,14 @@ void ThreadPlanAssemblyTracer::Log() {
if (m_register_values[reg_num].GetType() == RegisterValue::eTypeInvalid ||
reg_value != m_register_values[reg_num]) {
if (reg_value.GetType() != RegisterValue::eTypeInvalid) {
- stream->PutCString("\n\t");
- DumpRegisterValue(reg_value, *stream, *reg_info, true, false,
+ stream_sp->PutCString("\n\t");
+ DumpRegisterValue(reg_value, *stream_sp, *reg_info, true, false,
eFormatDefault);
}
}
m_register_values[reg_num] = reg_value;
}
}
- stream->EOL();
- stream->Flush();
+ stream_sp->EOL();
+ stream_sp->Flush();
}
|
labath
approved these changes
Feb 19, 2025
There was a problem hiding this comment.
LGTM. I am not saying that you have to do anything about this, but there are two things that caught my eye:
- Debugger::GetOutputFileSP is now the biggest backdoor into our locking mechanism. In a way, it's even worse than GetOutputStreamSP, since it bypasses locking completely. And since it supports
PrintfI can totally see someone reaching for that without realizing the problem. - It would sure be nice if StreamAsync was more of an RAII thing. Like, at least stored in a unique pointer, and maybe even passed by value (if we can make it movable).
| @@ -2837,8 +2837,8 @@ void CommandInterpreter::HandleCommandsFromFile( | |||
| } | |||
|
|
|||
| if (flags & eHandleCommandFlagPrintResult) { | |||
| debugger.GetOutputFile().Printf("Executing commands in '%s'.\n", | |||
| cmd_file_path.c_str()); | |||
| debugger.GetOutputFileSP()->Printf("Executing commands in '%s'.\n", | |||
There was a problem hiding this comment.
This is one of those suspicious calls.
Yup. I was thinking about this too, and we definitely need one of the two, at least for the SB API . My reasoning was that handing out the lockable stream potentially gives you this false sense of security. As we probably can't prevent handing out the
Good idea! |
JDevlieghere
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Mar 27, 2025
JDevlieghere
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Mar 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes GetOutputStreamSP and GetErrorStreamSP protected members of Debugger. Users who want to print to the debugger's stream should use GetAsyncOutputStreamSP and GetAsyncErrorStreamSP instead and the few remaining stragglers have been migrated.