[Lldb-commits] [PATCH] D154542: Further refinements on reporting interruption in lldb

[Med Ismail Bennani via Phabricator via lldb-commits]

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