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

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.


================
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();
----------------
jingham wrote:
> bulbazord wrote:
> > 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.
> > 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.
> 
> I don't see how I can make the formatv option a StringRef.  The llvm::formatv API only offers a version that takes a `const char *`.  Anyway, these are formatv strings, they are almost universally going to be const strings.  Turning them into llvm::StringRef's and back out again to use in llvm::formatv seems odd.
> 
> But you are right, we should protect against someone passing in a null pointer for the function or format to InterruptRequested.   Since this is just logging, an assert seems overkill, I'll just add null pointer checks here and turn them into "UNKNOWN FUNCTION" and "Unknown message".
Oh... so `llvm::formatv` does take a `const char *`... My bad there, that makes sense, no need to change the type of `format` then.

Otherwise, I'm satisfied with the safety checks.


================
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()),
----------------
jingham wrote:
> bulbazord wrote:
> > To avoid some unnecessary copies
> > 
> > Could also do what Ismail is suggesting.
> This is a local that is copied to an ivar and never used again.  Do I really have to put move in there explicitly for the optimizer to know it can reuse the value?
Yes. I created a simple example on Godbolt: https://godbolt.org/z/G4ej76Enn

Observe that the constructor in the example invokes the `std::string` copy constructor. Add a `std::move` and it then invokes the move constructor.


================
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)...)) {}
----------------
bulbazord wrote:
> since we're passing `format` to `formatv`, I think it would make sense for its type to be `llvm::StringRef` up-front here.
As you've pointed out above, `formatv` takes a `const char *` so ignore this.


================
Comment at: lldb/source/Core/Debugger.cpp:1280
+    const llvm::formatv_object_base &payload) :  
+        m_function_name(function_name), 
+        m_interrupt_time(std::chrono::system_clock::now()),
----------------
You'll also want to `std::move(function_name)` here too, I think.


================
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));
----------------
jingham wrote:
> bulbazord wrote:
> > 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`.
> The point behind this is someone says "make me a target" and then in the process of making the target, the Target code does something slow (e.g. look for external debug files) that we want to interrupt.  So we need a way for the debugger to find the "in the process of being made" Targets.  
> 
>  "AddTargetInternal" is the API where the target gets added to the Debugger's TargetList, so after that method has run we will find the target in the regular TargetList.  But between CreateTargetInternal and AddTargetInternal, that target isn't stored anywhere that the Debugger knows how to query.  I added this list to hold the targets that are in the process of getting made, before they get added to the Debugger's TargetList.
Gotcha, that makes sense. Thanks for explaining!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154542