[lldb] Introduce internal stop hooks

[yln]

Introduce the concept of internal stop hooks.
These are similar to LLDB's internal breakpoints:
LLDB itself will add them and users of LLDB will
not be able to add or remove them.

This change adds the following 3
independently-useful concepts:

• Maintain a list of internal stop hooks that will be populated by LLDB and cannot be added to or removed from by users. They are managed in a separate list in Target::m_internal_stop_hooks.
• StopHookKind:CodeBased and StopHookCoded represent a stop hook defined by a C++ code callback (instead of command line expressions or a Python class).
• Stop hooks that do not print any output can now also suppress the printing of their header and description when they are hit via StopHook::GetSuppressOutput.

Combining these 3 concepts we can model "internal
stop hooks" which serve the same function as
LLDB's internal breakpoints: executing built-in,
LLDB-defined behavior, leveraging the existing
mechanism of stop hooks.

This change also simplifies
Target::RunStopHooks. We already have to
materialize a new list for combining internal and
user stop hooks. Filter and only add active hooks to this list to avoid the need for "isActive?"
checks later on.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Julian Lettner (yln)

Changes

Introduce the concept of internal stop hooks.
These are similar to LLDB's internal breakpoints:
LLDB itself will add them and users of LLDB will
not be able to add or remove them.

This change adds the following 3
independently-useful concepts:

• Maintain a list of internal stop hooks that will be populated by LLDB and cannot be added to or removed from by users. They are managed in a separate list in Target::m_internal_stop_hooks.
• StopHookKind:CodeBased and StopHookCoded represent a stop hook defined by a C++ code callback (instead of command line expressions or a Python class).
• Stop hooks that do not print any output can now also suppress the printing of their header and description when they are hit via StopHook::GetSuppressOutput.

Combining these 3 concepts we can model "internal
stop hooks" which serve the same function as
LLDB's internal breakpoints: executing built-in,
LLDB-defined behavior, leveraging the existing
mechanism of stop hooks.

This change also simplifies
Target::RunStopHooks. We already have to
materialize a new list for combining internal and
user stop hooks. Filter and only add active hooks to this list to avoid the need for "isActive?"
checks later on.

---

Full diff: https://github.com/llvm/llvm-project/pull/164506.diff

2 Files Affected:

• (modified) lldb/include/lldb/Target/Target.h (+56-7)
• (modified) lldb/source/Target/Target.cpp (+35-25)

diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index f4a09237ce897..e745cb93eae9a 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1356,7 +1356,11 @@ class Target : public std::enable_shared_from_this<Target>,
     StopHook(const StopHook &rhs);
     virtual ~StopHook() = default;
 
-    enum class StopHookKind  : uint32_t { CommandBased = 0, ScriptBased };
+    enum class StopHookKind : uint32_t {
+      CommandBased = 0,
+      ScriptBased,
+      CodeBased,
+    };
     enum class StopHookResult : uint32_t {
       KeepStopped = 0,
       RequestContinue,
@@ -1403,6 +1407,12 @@ class Target : public std::enable_shared_from_this<Target>,
 
     bool GetRunAtInitialStop() const { return m_at_initial_stop; }
 
+    void SetSuppressOutput(bool suppress_output) {
+      m_suppress_output = suppress_output;
+    }
+
+    bool GetSuppressOutput() const { return m_suppress_output; }
+
     void GetDescription(Stream &s, lldb::DescriptionLevel level) const;
     virtual void GetSubclassDescription(Stream &s,
                                         lldb::DescriptionLevel level) const = 0;
@@ -1414,6 +1424,7 @@ class Target : public std::enable_shared_from_this<Target>,
     bool m_active = true;
     bool m_auto_continue = false;
     bool m_at_initial_stop = true;
+    bool m_suppress_output = false;
 
     StopHook(lldb::TargetSP target_sp, lldb::user_id_t uid);
   };
@@ -1433,7 +1444,7 @@ class Target : public std::enable_shared_from_this<Target>,
 
   private:
     StringList m_commands;
-    // Use CreateStopHook to make a new empty stop hook. The GetCommandPointer
+    // Use CreateStopHook to make a new empty stop hook. The SetActionFromString
     // and fill it with commands, and SetSpecifier to set the specifier shared
     // pointer (can be null, that will match anything.)
     StopHookCommandLine(lldb::TargetSP target_sp, lldb::user_id_t uid)
@@ -1460,19 +1471,56 @@ class Target : public std::enable_shared_from_this<Target>,
     StructuredDataImpl m_extra_args;
     lldb::ScriptedStopHookInterfaceSP m_interface_sp;
 
-    /// Use CreateStopHook to make a new empty stop hook. The GetCommandPointer
-    /// and fill it with commands, and SetSpecifier to set the specifier shared
-    /// pointer (can be null, that will match anything.)
+    /// Use CreateStopHook to make a new empty stop hook. Use SetScriptCallback
+    /// to set the script to execute, and SetSpecifier to set the specifier
+    /// shared pointer (can be null, that will match anything.)
     StopHookScripted(lldb::TargetSP target_sp, lldb::user_id_t uid)
         : StopHook(target_sp, uid) {}
     friend class Target;
   };
 
+  class StopHookCoded : public StopHook {
+  public:
+    ~StopHookCoded() override = default;
+
+    using HandleStopCallback = StopHookResult(ExecutionContext &exc_ctx,
+                                              lldb::StreamSP output);
+
+    void SetCallback(llvm::StringRef name, HandleStopCallback *callback) {
+      m_name = name;
+      m_callback = callback;
+    }
+
+    StopHookResult HandleStop(ExecutionContext &exc_ctx,
+                              lldb::StreamSP output) override {
+      return m_callback(exc_ctx, output);
+    }
+
+    void GetSubclassDescription(Stream &s,
+                                lldb::DescriptionLevel level) const override {
+      s.Indent();
+      s.Printf("%s (built-in)\n", m_name.c_str());
+    }
+
+  private:
+    std::string m_name;
+    HandleStopCallback *m_callback;
+
+    /// Use CreateStopHook to make a new empty stop hook. Use SetCallback to set
+    /// the callback to execute, and SetSpecifier to set the specifier shared
+    /// pointer (can be null, that will match anything.)
+    StopHookCoded(lldb::TargetSP target_sp, lldb::user_id_t uid)
+        : StopHook(target_sp, uid) {}
+    friend class Target;
+  };
+
+  void RegisterInternalStopHooks();
+
   typedef std::shared_ptr<StopHook> StopHookSP;
 
   /// Add an empty stop hook to the Target's stop hook list, and returns a
-  /// shared pointer to it in new_hook. Returns the id of the new hook.
-  StopHookSP CreateStopHook(StopHook::StopHookKind kind);
+  /// shared pointer to it in new_hook.
+  StopHookSP CreateStopHook(StopHook::StopHookKind kind, bool internal = false);
 
   /// If you tried to create a stop hook, and that failed, call this to
   /// remove the stop hook, as it will also reset the stop hook counter.
@@ -1656,6 +1704,7 @@ class Target : public std::enable_shared_from_this<Target>,
   typedef std::map<lldb::user_id_t, StopHookSP> StopHookCollection;
   StopHookCollection m_stop_hooks;
   lldb::user_id_t m_stop_hook_next_id;
+  std::vector<StopHookSP> m_internal_stop_hooks;
   uint32_t m_latest_stop_hook_id; /// This records the last natural stop at
                                   /// which we ran a stop-hook.
   bool m_valid;
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index e224a12e33463..abfcee6e2c34b 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -183,8 +183,8 @@ Target::Target(Debugger &debugger, const ArchSpec &target_arch,
       m_watchpoint_list(), m_process_sp(), m_search_filter_sp(),
       m_image_search_paths(ImageSearchPathsChanged, this),
       m_source_manager_up(), m_stop_hooks(), m_stop_hook_next_id(0),
-      m_latest_stop_hook_id(0), m_valid(true), m_suppress_stop_hooks(false),
-      m_is_dummy_target(is_dummy_target),
+      m_internal_stop_hooks(), m_latest_stop_hook_id(0), m_valid(true),
+      m_suppress_stop_hooks(false), m_is_dummy_target(is_dummy_target),
       m_target_unique_id(g_target_unique_id++),
       m_frame_recognizer_manager_up(
           std::make_unique<StackFrameRecognizerManager>()) {
@@ -217,6 +217,7 @@ Target::~Target() {
 void Target::PrimeFromDummyTarget(Target &target) {
   m_stop_hooks = target.m_stop_hooks;
   m_stop_hook_next_id = target.m_stop_hook_next_id;
+  m_internal_stop_hooks = target.m_internal_stop_hooks;
 
   for (const auto &breakpoint_sp : target.m_breakpoint_list.Breakpoints()) {
     if (breakpoint_sp->IsInternal())
@@ -383,6 +384,7 @@ void Target::Destroy() {
   m_image_search_paths.Clear(notify);
   m_stop_hooks.clear();
   m_stop_hook_next_id = 0;
+  m_internal_stop_hooks.clear();
   m_suppress_stop_hooks = false;
   m_repl_map.clear();
   Args signal_args;
@@ -3041,8 +3043,9 @@ SourceManager &Target::GetSourceManager() {
   return *m_source_manager_up;
 }
 
-Target::StopHookSP Target::CreateStopHook(StopHook::StopHookKind kind) {
-  lldb::user_id_t new_uid = ++m_stop_hook_next_id;
+Target::StopHookSP Target::CreateStopHook(StopHook::StopHookKind kind,
+                                          bool internal) {
+  user_id_t new_uid = (internal ? LLDB_INVALID_UID : ++m_stop_hook_next_id);
   Target::StopHookSP stop_hook_sp;
   switch (kind) {
   case StopHook::StopHookKind::CommandBased:
@@ -3051,8 +3054,14 @@ Target::StopHookSP Target::CreateStopHook(StopHook::StopHookKind kind) {
   case StopHook::StopHookKind::ScriptBased:
     stop_hook_sp.reset(new StopHookScripted(shared_from_this(), new_uid));
     break;
+  case StopHook::StopHookKind::CodeBased:
+    stop_hook_sp.reset(new StopHookCoded(shared_from_this(), new_uid));
+    break;
   }
-  m_stop_hooks[new_uid] = stop_hook_sp;
+  if (internal)
+    m_internal_stop_hooks.push_back(stop_hook_sp);
+  else
+    m_stop_hooks[new_uid] = stop_hook_sp;
   return stop_hook_sp;
 }
 
@@ -3111,16 +3120,20 @@ bool Target::RunStopHooks(bool at_initial_stop) {
   if (!(state == eStateStopped || state == eStateAttaching))
     return false;
 
-  if (m_stop_hooks.empty())
-    return false;
+  auto is_active = [at_initial_stop](StopHookSP hook) {
+    bool should_run_now = (!at_initial_stop || hook->GetRunAtInitialStop());
+    return hook->IsActive() && should_run_now;
+  };
 
-  bool no_active_hooks =
-      llvm::none_of(m_stop_hooks, [at_initial_stop](auto &p) {
-        bool should_run_now =
-            !at_initial_stop || p.second->GetRunAtInitialStop();
-        return p.second->IsActive() && should_run_now;
-      });
-  if (no_active_hooks)
+  // Create list of active internal and user stop hooks.
+  std::vector<StopHookSP> active_hooks;
+  llvm::copy_if(m_internal_stop_hooks, std::back_inserter(active_hooks),
+                is_active);
+  for (auto &[_, hook] : m_stop_hooks) {
+    if (is_active(hook))
+      active_hooks.push_back(hook);
+  }
+  if (active_hooks.empty())
     return false;
 
   // Make sure we check that we are not stopped because of us running a user
@@ -3169,24 +3182,21 @@ bool Target::RunStopHooks(bool at_initial_stop) {
   StreamSP output_sp = m_debugger.GetAsyncOutputStream();
   auto on_exit = llvm::make_scope_exit([output_sp] { output_sp->Flush(); });
 
-  bool print_hook_header = (m_stop_hooks.size() != 1);
-  bool print_thread_header = (num_exe_ctx != 1);
+  size_t num_hooks_with_output = llvm::count_if(
+      active_hooks, [](auto h) { return !h->GetSuppressOutput(); });
+  bool print_hook_header = (num_hooks_with_output > 1);
+  bool print_thread_header = (num_exe_ctx > 1);
   bool should_stop = false;
   bool requested_continue = false;
 
-  for (auto stop_entry : m_stop_hooks) {
-    StopHookSP cur_hook_sp = stop_entry.second;
-    if (!cur_hook_sp->IsActive())
-      continue;
-    if (at_initial_stop && !cur_hook_sp->GetRunAtInitialStop())
-      continue;
-
+  for (auto cur_hook_sp : active_hooks) {
     bool any_thread_matched = false;
     for (auto exc_ctx : exc_ctx_with_reasons) {
       if (!cur_hook_sp->ExecutionContextPasses(exc_ctx))
         continue;
 
-      if (print_hook_header && !any_thread_matched) {
+      bool suppress_output = cur_hook_sp->GetSuppressOutput();
+      if (print_hook_header && !any_thread_matched && !suppress_output) {
         StreamString s;
         cur_hook_sp->GetDescription(s, eDescriptionLevelBrief);
         if (s.GetSize() != 0)
@@ -3197,7 +3207,7 @@ bool Target::RunStopHooks(bool at_initial_stop) {
         any_thread_matched = true;
       }
 
-      if (print_thread_header)
+      if (print_thread_header && !suppress_output)
         output_sp->Printf("-- Thread %d\n",
                           exc_ctx.GetThreadPtr()->GetIndexID());
 

[DavidSpickett]

I have used stop hooks as a user but never had to deal with their implementation. So my first question here is "but why?".

The strongest point here seems to be:

Combining these 3 concepts we can model "internal
stop hooks" which serve the same function as
LLDB's internal breakpoints: executing built-in,
LLDB-defined behavior, leveraging the existing
mechanism of stop hooks.

And maybe those who have worked on them will think that is enough to make this change.

Is there a bigger motivation for this?

[jimingham]

That is the reason for introducing this concept. Note that stop hooks are orthogonal to breakpoints, in that breakpoints allow you to react to the program running certain code, whereas stop hooks allow you to react to stop reasons in a pluggable manner. So this allows a plugin to react to a stop reason without having to put the behavior in the generic StopInfo::ShouldStop and PerformAction methods. So they are a complement to what you can to with internal breakpoints.
Also, though this patch doesn't add the capability, this is the first step to making an SB way to add C++ backed stop-hooks. There's no good reason why you shouldn't be able to write stop hooks in C++ as well as Python & Command based. Julian didn't need that for his use case, but when someone does it will be that much easier to add.

[jimingham]

This looks pretty good.

It seems odd to be able to make stop hooks with a StopHookKind but then not have the StopHook's remember and report their Kind. I can't think of an explicit use at this point, but it does seem like something you might end up wanting to know...

I also think for debugging purposes, we'll want to have target stop-hook list -i. When somebody asks "why isn't this behavior (that we know is backed by an internal stop hook) working" we'll be really happy that we can ask them to run target stop-hook list -i and see "For some reason that stop hook didn't get loaded"...

[DavidSpickett]

That is the reason for introducing this concept. Note that stop hooks are orthogonal to breakpoints, in that breakpoints allow you to react to the program running certain code, whereas stop hooks allow you to react to stop reasons in a pluggable manner.

So stop hooks are great for users, they would be good for LLDB itself. Thanks for explaining.

[yln]

I also think for debugging purposes, we'll want to have target stop-hook list -I.

I've added target stop-hook list --internal:

[jimingham]

Are you sure that using the iterator of the map will preserve the order of the stop-hooks in this listing? I'm not concerned about internal stop-hooks, but it wouldn't be good if you added a bunch of stop hooks, then deleted some, and then the order of the stop-hooks in the listing got scrambled.
Might be good to add a test to ensure that this ordering is maintained to ensure that.

[yln]

typedef std::map<lldb::user_id_t, StopHookSP> StopHookCollection;
StopHookCollection m_stop_hooks;

Iteration order of std::map is defined by its keys (ascending).

Previously, we were also using std::map iteration order, just in a really convoluted/inefficient way:

size_t GetNumStopHooks() const { return m_stop_hooks.size(); }

StopHookSP GetStopHookAtIndex(size_t index) {  // <<< this is 0-based index, not the "user ID"
  if (index >= GetNumStopHooks())
    return StopHookSP();
  StopHookCollection::iterator pos = m_stop_hooks.begin();   // <<< std::map iterator

  while (index > 0) {
    pos++;
    index--;
  }
  return (*pos).second;
}

// Call sites:
  size_t num_hooks = target.GetNumStopHooks(); 
  for (size_t i = 0; i < num_hooks; i++) {
    Target::StopHookSP hook = target.GetStopHookAtIndex(i);
    ...
  }

[yln]

Added new test to pin down the behavior: 27b1441
Made sure it goes red when order is messed up, for example:

for (auto &[_, hook] : llvm::reverse(m_stop_hooks))
    stop_hooks.push_back(hook);

[jimingham]

LGTM