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

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

I really like the change to the interface, I especially like that it can report what was interrupted and how much work actually done. I had a lot of the same feedback as Ismail but also had some questions to help me understand all the details.



================
Comment at: lldb/include/lldb/Core/Debugger.h:430-431
+  template <typename... Args>
+  bool InterruptRequested(const char *cur_func, 
+                          const char *formatv, Args &&... args) {
+    bool ret_val = InterruptRequested();
----------------
I think it would make more sense to have `cur_func` and `formatv` be of type `llvm::StringRef`. Concretely, if you construct a `std::string` from `nullptr` (like you're doing below when you make an InterruptionReport object), you will crash.

I know the recommendation is to use `INTERRUPT_REQUESTED` instead of filling this manually, but inevitably somebody will go against the advice and make a mistake.


================
Comment at: lldb/include/lldb/Core/Debugger.h:446
+#define INTERRUPT_REQUESTED(debugger, ...) \
+    (debugger).InterruptRequested(__func__, __VA_ARGS__)
+
----------------
mib wrote:
> You could use `LLVM_PRETTY_FUNCTION` which will give you both the function signature and the argument well formatted.
+1


================
Comment at: lldb/include/lldb/Core/Debugger.h:455-456
+    InterruptionReport(std::string function_name, std::string description) :
+        m_function_name(function_name), 
+        m_description(description),
+        m_interrupt_time(std::chrono::system_clock::now()),
----------------
To avoid some unnecessary copies

Could also do what Ismail is suggesting.


================
Comment at: lldb/include/lldb/Core/Debugger.h:465
+  InterruptionReport(std::string function_name,
+              const char *format, Args &&... args) :
+    InterruptionReport(function_name, llvm::formatv(format, std::forward<Args>(args)...)) {}
----------------
since we're passing `format` to `formatv`, I think it would make sense for its type to be `llvm::StringRef` up-front here.


================
Comment at: lldb/include/lldb/Target/TargetList.h:191
+  ///  these not yet added target, but for interruption purposes, we might
+  ///  need to ask whetehr this target contains this module. 
+  bool AnyTargetContainsModule(Module *module);
----------------



================
Comment at: lldb/include/lldb/Target/TargetList.h:192
+  ///  need to ask whetehr this target contains this module. 
+  bool AnyTargetContainsModule(Module *module);
 
----------------
mib wrote:
> Can the module be null ? If not, can we pass a reference instead of a raw pointer here ?
+1

Alternatively, if these are being pulled out of shared pointers, pass the shared pointer directly so we can avoid the issue of dangling raw pointers if the module ever is freed by the shared pointer.


================
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;
----------------
mib wrote:
> Same question as above, can any of the `in_process_target` be null ? If not, lets make it a `Target&`
+1

Same as above, let's not circumvent shared pointer semantics since we're storing references/pointers to these objects.


================
Comment at: lldb/include/lldb/Target/TargetList.h:216-221
+  void RegisterInProcessTarget(Target *target);
+  
+  void UnregisterInProcessTarget(Target *target);
+  
+  bool IsTargetInProcess(Target *target);
+  
----------------
mib wrote:
> ditto
Same as my comment above.


================
Comment at: lldb/source/API/SBFrame.cpp:57
 
+#include <sstream>
+
----------------
What is this used for?


================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:67-68
 
+#include <sstream>
+
 
----------------
mib wrote:
> Is this still necessary ?
Where is this used?


================
Comment at: lldb/source/Core/Debugger.cpp:1266-1267
 
-bool Debugger::InterruptRequested() {
+bool Debugger::InterruptRequested() 
+{
   // This is the one we should call internally.  This will return true either
----------------
Did clang-format give this to you?


================
Comment at: lldb/source/Core/Debugger.cpp:1291-1292
+    // For now, just log the description:
+  Log *log = GetLog(LLDBLog::Host);
+  LLDB_LOG(log, "Interruption: {0}", report.m_description);
+}
----------------



================
Comment at: lldb/source/Core/Module.cpp:1075
+        for (auto debugger_sp : interruptors) {
+          REPORT_INTERRUPTION(*(debugger_sp.get()), 
+                              "Interrupted fetching symbols for module {0}", 
----------------



================
Comment at: lldb/source/Core/Module.cpp:1077
+                              "Interrupted fetching symbols for module {0}", 
+                              this->GetFileSpec());
+        }
----------------
I don't think you need to specify `this->`?


================
Comment at: lldb/source/Target/StackFrameList.cpp:31
 #include <memory>
+#include <sstream>
 
----------------
Where is this used?


================
Comment at: lldb/source/Target/TargetList.cpp:518
              "target already exists it the list");
+  UnregisterInProcessTarget(target_sp.get());
   m_target_list.push_back(std::move(target_sp));
----------------
I'm somewhat puzzled by this. Why are we unregistering the target in `AddTargetInternal` when we're registering it in `CreateTargetInternal`? Maybe I don't understand the intent behind `{Register,Unregister}InProcessTarget`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154542/new/

https://reviews.llvm.org/D154542