mib added a comment. This looks very cool! I left "a few" comments on how I think we can simplify this patch, and also had some remarks regarding the `in_process_target` sanity checks.
================ Comment at: lldb/include/lldb/Core/Debugger.h:435-436 + ReportInterruption(InterruptionReport(cur_func, + llvm::formatv(formatv, + std::forward<Args>(args)...))); + } ---------------- If you use `LLVM_PRETTY_FUNCTION` you won't need this. ================ Comment at: lldb/include/lldb/Core/Debugger.h:446 +#define INTERRUPT_REQUESTED(debugger, ...) \ + (debugger).InterruptRequested(__func__, __VA_ARGS__) + ---------------- You could use `LLVM_PRETTY_FUNCTION` which will give you both the function signature and the argument well formatted. ================ Comment at: lldb/include/lldb/Core/Debugger.h:454 + public: + InterruptionReport(std::string function_name, std::string description) : + m_function_name(function_name), ---------------- ================ Comment at: lldb/include/lldb/Core/Debugger.h:460-468 + InterruptionReport(std::string function_name, + const llvm::formatv_object_base &payload); + + template <typename... Args> + InterruptionReport(std::string function_name, + const char *format, Args &&... args) : + InterruptionReport(function_name, llvm::formatv(format, std::forward<Args>(args)...)) {} ---------------- Using `LLVM_PRETTY_FUNCTION` we can simplify the class by removing the variadic constructors and replace to `std::string` with a `llvm::StringLiteral` ================ Comment at: lldb/include/lldb/Target/TargetList.h:192 + /// need to ask whetehr this target contains this module. + bool AnyTargetContainsModule(Module *module); ---------------- Can the module be null ? If not, can we pass a reference instead of a raw pointer here ? ================ Comment at: lldb/include/lldb/Target/TargetList.h:200 collection m_target_list; + std::unordered_set<Target *> m_in_process_target_list; mutable std::recursive_mutex m_target_list_mutex; ---------------- Same question as above, can any of the `in_process_target` be null ? If not, lets make it a `Target&` ================ Comment at: lldb/include/lldb/Target/TargetList.h:216 + void RegisterInProcessTarget(Target *target); + ---------------- ditto ================ Comment at: lldb/include/lldb/Target/TargetList.h:218 + + void UnregisterInProcessTarget(Target *target); + ---------------- ditto ================ Comment at: lldb/include/lldb/Target/TargetList.h:220 + + bool IsTargetInProcess(Target *target); + ---------------- ditto ================ Comment at: lldb/source/API/SBFrame.cpp:57-58 +#include <sstream> + using namespace lldb; ---------------- Is this still necessary ? ================ Comment at: lldb/source/Commands/CommandObjectTarget.cpp:67-68 +#include <sstream> + ---------------- Is this still necessary ? ================ Comment at: lldb/source/Core/Debugger.cpp:1279-1287 +Debugger::InterruptionReport::InterruptionReport(std::string function_name, + const llvm::formatv_object_base &payload) : + m_function_name(function_name), + m_interrupt_time(std::chrono::system_clock::now()), + m_thread_id(llvm::get_threadid()) +{ + llvm::raw_string_ostream desc(m_description); ---------------- You can get rid of this is you use `LLVM_PRETTY_FUNCTION`. ================ Comment at: lldb/source/Core/Debugger.cpp:1289-1293 +void Debugger::ReportInterruption(const InterruptionReport &report) { + // For now, just log the description: + Log *log = GetLog(LLDBLog::Host); + LLDB_LOG(log, "Interruption: {0}", report.m_description); +} ---------------- It would be nice if `InterruptionReport` had a `Dump` method. ================ Comment at: lldb/source/Core/Module.cpp:1071-1072 if (!m_did_load_symfile.load() && can_create) { + Debugger::DebuggerList interruptors + = DebuggersOwningModuleRequestingInterruption(this); + if (!interruptors.empty()) { ---------------- You can get the Module reference by dereferencing `this`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154542/new/ https://reviews.llvm.org/D154542 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits