[lldb] Add boilerplate for debugger interrupts

[JDevlieghere]

Summary

Jim is looking into adding support for interrupts to the SB API. Part of that work requires clearing the interrupt bit once we return from the SB API. Luckily, this is already tracked by the API instrumentation. This patch adds the necessary interruption boilerplate to hook it up with the SB API instrumentation.

[jingham]

LGTM

[labath]

I don't know how this is intended to be used, but I find it a rather heavyweight approach towards implementing, well... anything. The idea that something as innocuous as SBAddress::IsValid() will end up iterating through _all_ SBDebugger objects seems wrong, regardless of whether it has a measurable performance impact (and I'd be surprised if it doesn't). I'd hope that the interrupter can at least know which debugger it is trying to interrupt.

It's also not clear how something like this can be used to reliably interrupt an activity, as it's prone to all sorts of race conditions. (If the interuptee has not started the blocking call yet, then the interrupt request can be "eaten" by an innocuous IsValid call, whereas if it has already finished, then the interrupt request can remain pending and interrupt a totally unrelated call).

Overall, I think it would be good to have a RFC on this.

[labath]

I've added you to D133410, as I think it demonstrates very well the difficulties in getting two threads to talk to one another. This code has been here since forever (and may have contributed to your desire to introduce interrupts), but we've only found the problem now, when Michał added some unit tests for this class.

[jingham]

To be clear, I'm not trying to implement a preemptive interrupt using these callbacks. There are so many places where lldb is doing work that you really can't interrupt - e.g. we can't drop symbol parsing and then pick it up again later - that that doesn't really even make sense.

I started out with the goal of extending the current InterruptCommand/WasInterrupted style of voluntary interruption to more places in lldb. A voluntary "I'm doing something in a loop that can be interrupted, so I'll check for interruption at the top of the loop" mechanism seems like the best fit for lldb, so that structure makes sense. But we only check for interruption a few place in the "target modules" commands. It would be useful to have other places check - e.g. if you did a bt and there were 40000 frames and you don't want to wait for us to print them all out... So I was going to look into adding more interrupt checks.

But then it seemed a shame that setting this interrupt only works when you are running command line commands, there's no reason this can't work for SB API calls as well, then UI's could also allow this sort of interruption. If the UI code is doing something in a loop, then it's up to the UI code to handle interrupting that operation. So all I'm trying to do is set the interrupt flag for use while the currently executing SB API is in flight, then turning it off when that call exits.

The debugger is the one setting the interrupt flag, so we always know who is sending the interrupt. The tricky bit is how to turn off the "WasInterrupted" flag after the API that was in flight when the flag was set finishes. That requires as a first step the ability to mark the "top level SB API" boundaries, and that's what Jonas was showing me how to do here. With these callbacks, some parts of this tracking will be straightforward, like by tracking enter and leave I can drop the interrupt flag immediately if there are no API's currently running. And if there were only one API in flight, turning the flag off at the right time is straight forward.

But you are right, there are tricky bits around handling two SB API's in flight on different threads. If they are for the same debugger, we probably do want to interrupt both. However, you don't want a short operation on thread A to turn off the interrupt flag before a long operation on thread B gets a chance to check it. So we'll probably want a counter and not just a flag. Also, it doesn't make sense for one Debugger's interrupt to interrupt the actions of another, so somehow we're going to have segregate API calls based on debugger, which I don't know how to do yet.

Anyway, this patch was mainly Jonas showing me how to get hook points for the enter and leave point for the top-level SB API calls so I could start to play with how to do this. TTTT I thought it would be more complex than this, and so it seemed worthwhile to add the general facility downstream. This is sufficiently straightforward, however, that I am fine with keeping it locally while I play with it.

This isn't really a new feature, it's just extending lldb's current voluntary interrupt mechanism to work that initiates from the SB API as well as from the command line. But once I get a better idea of how to do this, depending on how tricky it ends up being I'll either put up a patch or an RFC.

[labath]

In D133129#3778125, @jingham wrote:

To be clear, I'm not trying to implement a preemptive interrupt using these callbacks. There are so many places where lldb is doing work that you really can't interrupt - e.g. we can't drop symbol parsing and then pick it up again later - that that doesn't really even make sense.

I started out with the goal of extending the current InterruptCommand/WasInterrupted style of voluntary interruption to more places in lldb. A voluntary "I'm doing something in a loop that can be interrupted, so I'll check for interruption at the top of the loop" mechanism seems like the best fit for lldb, so that structure makes sense. But we only check for interruption a few place in the "target modules" commands. It would be useful to have other places check - e.g. if you did a bt and there were 40000 frames and you don't want to wait for us to print them all out... So I was going to look into adding more interrupt checks.

But then it seemed a shame that setting this interrupt only works when you are running command line commands, there's no reason this can't work for SB API calls as well, then UI's could also allow this sort of interruption. If the UI code is doing something in a loop, then it's up to the UI code to handle interrupting that operation.

I agree with all that.

So all I'm trying to do is set the interrupt flag for use while the currently executing SB API is in flight, then turning it off when that call exits.

The problem is that, even with a single SB call, it's very hard to tell the difference between "I am doing X" vs. "I am about to do X" vs. "I have just completed X (but haven't told anyone about it)". And I don't see a way to do that reliably through any kind of automatic API boundary tracking.

The debugger is the one setting the interrupt flag, so we always know who is sending the interrupt. The tricky bit is how to turn off the "WasInterrupted" flag after the API that was in flight when the flag was set finishes.

Maybe the solution is to not (automatically) turn off the flag -- but put it in the hands of the user instead? If the interrupt flag was sticky (with explicit SetInterrupt/ClearInterrupt actions), then one can handle all of the scenarios above fairly easily.
It doesn't matter that whether SetInterrupt is called before the blocking call or not -- the call is going to be interrupted anyway. And one can explicitly clear the interrupt flag after the blocking call returns (either successfully or because it was interrupted).

[jingham]

In D133129#3780028, @labath wrote:

In D133129#3778125, @jingham wrote:

To be clear, I'm not trying to implement a preemptive interrupt using these callbacks. There are so many places where lldb is doing work that you really can't interrupt - e.g. we can't drop symbol parsing and then pick it up again later - that that doesn't really even make sense.

I started out with the goal of extending the current InterruptCommand/WasInterrupted style of voluntary interruption to more places in lldb. A voluntary "I'm doing something in a loop that can be interrupted, so I'll check for interruption at the top of the loop" mechanism seems like the best fit for lldb, so that structure makes sense. But we only check for interruption a few place in the "target modules" commands. It would be useful to have other places check - e.g. if you did a bt and there were 40000 frames and you don't want to wait for us to print them all out... So I was going to look into adding more interrupt checks.

But then it seemed a shame that setting this interrupt only works when you are running command line commands, there's no reason this can't work for SB API calls as well, then UI's could also allow this sort of interruption. If the UI code is doing something in a loop, then it's up to the UI code to handle interrupting that operation.

I agree with all that.

So all I'm trying to do is set the interrupt flag for use while the currently executing SB API is in flight, then turning it off when that call exits.

The problem is that, even with a single SB call, it's very hard to tell the difference between "I am doing X" vs. "I am about to do X" vs. "I have just completed X (but haven't told anyone about it)". And I don't see a way to do that reliably through any kind of automatic API boundary tracking.

I didn't see it as our job to make this distinction. If an IDE is about to do X or is just done with X and the user hits the IDE's pause button, then it's up to the IDE to pause whatever it was doing before and after SB API calls. We're not going to be able to interrupt anything that goes on between SB API calls anyway, so I thought it would be easier for UI's to reason about if all we did was interrupt in the current in-flight API call, which we should be able to use boundary tracking for.

The debugger is the one setting the interrupt flag, so we always know who is sending the interrupt. The tricky bit is how to turn off the "WasInterrupted" flag after the API that was in flight when the flag was set finishes.

Maybe the solution is to not (automatically) turn off the flag -- but put it in the hands of the user instead? If the interrupt flag was sticky (with explicit SetInterrupt/ClearInterrupt actions), then one can handle all of the scenarios above fairly easily.
It doesn't matter that whether SetInterrupt is called before the blocking call or not -- the call is going to be interrupted anyway. And one can explicitly clear the interrupt flag after the blocking call returns (either successfully or because it was interrupted).

That's an interesting thought. If we did that we'd have to separate the command line interrupt flags from the SB API ones w.r.t. setting/unsetting: a sticky flag is not the behavior you want for commands, but then check both in WasInterrupted. I can't see a reason why we'd need to distinguish the two sources of interrupt at the point where we go to check. We'd also have to never set the flag ourselves, having UI's have to spam turning this off would be super annoying, so this would have to be strictly controlled from outside lldb. But that should be okay, if we want this behavior in the driver we can implement it outside the lldb library in the driver. That just leaves me with the problem (even more acute with a sticky flag) of not interrupting API calls for the Debugger that didn't issue the interrupt.

[JDevlieghere]

Jim is working on another way to tackle this.

lldb/include/lldb/Core/Debugger.h

public:
 
   lldb::BroadcasterManagerSP GetBroadcasterManager() {
     return m_broadcaster_manager_sp;
   }
 
   /// Shared thread poll. Use only with ThreadPoolTaskGroup.
   static llvm::ThreadPool &GetThreadPool();
 
+  /// Request an interrupt.
+  void RequestInterrupt();
+
+  /// Clear the request for an interrupt.
+  void ClearInterrupt();
+
+  /// Clear all interrupt requests.
+  static void ClearInterrupts();
+
   /// Report warning events.
   ///
   /// Progress events will be delivered to any debuggers that have listeners
   /// for the eBroadcastBitError.
   ///
   /// \param[in] message
   ///   The warning message to be reported.
   ///
protected:
   HostThread m_event_handler_thread;
   HostThread m_io_handler_thread;
   Broadcaster m_sync_broadcaster; ///< Private debugger synchronization.
   Broadcaster m_broadcaster;      ///< Public Debugger event broadcaster.
   lldb::ListenerSP m_forward_listener_sp;
   llvm::once_flag m_clear_once;
   lldb::TargetSP m_dummy_target_sp;
 
+  bool m_interrupt_requested;
+
   // Events for m_sync_broadcaster
   enum {
     eBroadcastBitEventThreadIsListening = (1 << 0),
   };
 
 private:
   // Use Debugger::CreateInstance() to get a shared pointer to a new debugger
   // object

lldb/include/lldb/Utility/Instrumentation.h

 }
 
 /// RAII object for instrumenting LLDB API functions.
 class Instrumenter {
 public:
   Instrumenter(llvm::StringRef pretty_func, std::string &&pretty_args = {});
   ~Instrumenter();
 
+  using Callback = std::function<void(void)>;
+
+  /// Register a callback that's called when entering the instrumentation
+  /// boundary.
+  static void RegisterEntryCallback(Callback callback);
+
+  /// Register a callback that's called when leaving the instrumentation
+  /// boundary.
+  static void RegisterExitCallback(Callback callback);
+
+  /// Unregister all entry and exit callbacks.
+  static void UnregisterCallbacks();
+
 private:
   void UpdateBoundary();
 
   llvm::StringRef m_pretty_func;
 
   /// Whether this function call was the one crossing the API boundary.
   bool m_local_boundary = false;
 };

lldb/source/Core/Debugger.cpp

 #include "lldb/Target/Process.h"
 #include "lldb/Target/StructuredDataPlugin.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/TargetList.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Target/ThreadList.h"
 #include "lldb/Utility/AnsiTerminal.h"
 #include "lldb/Utility/Event.h"
+#include "lldb/Utility/Instrumentation.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Listener.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Reproducer.h"
 #include "lldb/Utility/ReproducerProvider.h"
 #include "lldb/Utility/State.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/StreamString.h"

 
 void Debugger::Initialize(LoadPluginCallbackType load_plugin_callback) {
   assert(g_debugger_list_ptr == nullptr &&
          "Debugger::Initialize called more than once!");
   g_debugger_list_mutex_ptr = new std::recursive_mutex();
   g_debugger_list_ptr = new DebuggerList();
   g_thread_pool = new llvm::ThreadPool(llvm::optimal_concurrency());
   g_load_plugin_callback = load_plugin_callback;
+  instrumentation::Instrumenter::RegisterExitCallback([]() { Debugger::ClearInterrupts(); });
 }
 
 void Debugger::Terminate() {
   assert(g_debugger_list_ptr &&
          "Debugger::Terminate called without a matching Debugger::Initialize!");
 
   if (g_thread_pool) {
     // The destructor will wait for all the threads to complete.
void Debugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
                                   void *baton) {
   // For simplicity's sake, I am not going to deal with how to close down any
   // open logging streams, I just redirect everything from here on out to the
   // callback.
   m_callback_handler_sp =
       std::make_shared<CallbackLogHandler>(log_callback, baton);
 }
 
+void Debugger::RequestInterrupt() {
+  m_interrupt_requested = true;
+}
+
+void Debugger::ClearInterrupt() {
+  m_interrupt_requested = false;
+}
+
+void Debugger::ClearInterrupts() {
+  if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) {
+    std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr);
+    DebuggerList::iterator pos, end = g_debugger_list_ptr->end();
+    for (pos = g_debugger_list_ptr->begin(); pos != end; ++pos)
+      (*pos)->ClearInterrupt();
+  }
+}
+
 static void PrivateReportProgress(Debugger &debugger, uint64_t progress_id,
                                   const std::string &message,
                                   uint64_t completed, uint64_t total,
                                   bool is_debugger_specific) {
   // Only deliver progress events if we have any progress listeners.
   const uint32_t event_type = Debugger::eBroadcastBitProgress;
   if (!debugger.GetBroadcaster().EventTypeHasListeners(event_type))
     return;

lldb/source/Utility/Instrumentation.cpp

 using namespace lldb_private::instrumentation;
 
 // Whether we're currently across the API boundary.
 static thread_local bool g_global_boundary = false;
 
 // Instrument SB API calls with singposts when supported.
 static llvm::ManagedStatic<llvm::SignpostEmitter> g_api_signposts;
 
+// Callbacks for entering the instrumentation boundary.
+static std::vector<Instrumenter::Callback> g_entry_callbacks;
+
+// Callbacks for leaving the instrumentation boundary.
+static std::vector<Instrumenter::Callback> g_exit_callbacks;
+
 Instrumenter::Instrumenter(llvm::StringRef pretty_func,
                            std::string &&pretty_args)
     : m_pretty_func(pretty_func) {
   if (!g_global_boundary) {
     g_global_boundary = true;
     m_local_boundary = true;
+    for (Callback callback : g_entry_callbacks)
+      callback();
     g_api_signposts->startInterval(this, m_pretty_func);
   }
   LLDB_LOG(GetLog(LLDBLog::API), "[{0}] {1} ({2})",
            m_local_boundary ? "external" : "internal", m_pretty_func,
            pretty_args);
 }
 
 Instrumenter::~Instrumenter() {
   if (m_local_boundary) {
     g_global_boundary = false;
+    for (Callback callback : g_exit_callbacks)
+      callback();
     g_api_signposts->endInterval(this, m_pretty_func);
   }
 }
+
+void Instrumenter::RegisterEntryCallback(Instrumenter::Callback callback) {
+  g_entry_callbacks.emplace_back(std::move(callback));
+}
+
+void Instrumenter::RegisterExitCallback(Instrumenter::Callback callback) {
+  g_exit_callbacks.emplace_back(std::move(callback));
+}
+
+void Instrumenter::UnregisterCallbacks() {
+  g_entry_callbacks.clear();
+  g_exit_callbacks.clear();
+}

lldb/unittests/Utility/CMakeLists.txt

 add_lldb_unittest(UtilityTests
   AnsiTerminalTest.cpp
   ArgsTest.cpp
   OptionsWithRawTest.cpp
   ArchSpecTest.cpp
   BroadcasterTest.cpp
   ConstStringTest.cpp
   CompletionRequestTest.cpp
   DataBufferTest.cpp
   DataEncoderTest.cpp
   DataExtractorTest.cpp
   EnvironmentTest.cpp
   EventTest.cpp
   FileSpecTest.cpp
   FlagsTest.cpp
+  InstrumentationTest.cpp
   ListenerTest.cpp
   LogTest.cpp
   NameMatchesTest.cpp
   PredicateTest.cpp
   ProcessInfoTest.cpp
   ProcessInstanceInfoTest.cpp
   RangeMapTest.cpp
   RangeTest.cpp

lldb/unittests/Utility/InstrumentationTest.cpp

+//===-- InstrumentationTest.cpp -------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/Instrumentation.h"
+
+using namespace llvm;
+using namespace lldb_private;
+using namespace lldb_private::instrumentation;
+
+TEST(InsturmentationTest, Callback) {
+  size_t entry = 0;
+  size_t exit = 0;
+
+  Instrumenter::UnregisterCallbacks();
+
+  Instrumenter::RegisterEntryCallback([&]() { entry++; });
+  Instrumenter::RegisterExitCallback([&]() { exit++; });
+
+  EXPECT_EQ(entry, 0UL);
+  EXPECT_EQ(exit, 0UL);
+
+  {
+    Instrumenter foo("foo");
+    EXPECT_EQ(entry, 1UL);
+    EXPECT_EQ(exit, 0UL);
+  }
+
+  EXPECT_EQ(entry, 1UL);
+  EXPECT_EQ(exit, 1UL);
+
+  {
+    Instrumenter bar("bar");
+    EXPECT_EQ(entry, 2UL);
+    EXPECT_EQ(exit, 1UL);
+  }
+
+  EXPECT_EQ(entry, 2UL);
+  EXPECT_EQ(exit, 2UL);
+
+  Instrumenter::UnregisterCallbacks();
+}