🍒 [lldb] Patches implementing MultiBreakpoint

lldb/docs/resources/lldbgdbremote.md

@@ -632,6 +632,62 @@ running, then an error message is returned.
 do live tracing. Specifically, the name of the plug-in should match the name
 of the tracing technology returned by this packet.
 
+## jMultiBreakpoint
+
+This packet allows setting and removing multiple breakpoints in one go. It
+lists multiple `Z` and `z` packets using a JSON array of strings.
+Formally:
+
+```
+$jMultiBreakpoint:{"breakpoint_requests" : ["request"[,"request"]*]}
+```
+
+Where each `request` is one of:
+
+```
+* z0,addr,kind
+* z1,addr,kind
+* z2,addr,kind
+* z3,addr,kind
+* z4,addr,kind
+* Z0,addr,kind[;cond_list…][;cmds:persist,cmd_list…]
+* Z1,addr,kind[;cond_list…][;cmds:persist,cmd_list…]
+* Z2,addr,kind
+* Z3,addr,kind
+* Z4,addr,kind
+```
+
+Each field has the same meaning as the corresponding packet in the GDB Remote
+Protocol.
+
+For example, the packet below is a request to set one breakpoint and to remove
+two others:
+
+```
+$jMultiBreakpoint: {"breakpoint_requests": ["Z0,1025783e8,4", "z0,1025783ec,4", "z0,1025783e8,4"]}
+```
+
+The same address may be specified multiple times.
+
+The stub must execute the sequence of `request`s in the order they
+appear in the `jMultiBreakpoint` packet. This is not an atomic operation:
+individual requests may fail, and the stub must process subsequent requests
+regardless of previous failures.
+
+The reply consists of a JSON dictionary with a single entry, `results`, which
+is an array of strings, with the same contents allowed by a reply to a `z` or
+`Z` packet.
+
+```
+{"results": ["OK", "E03", "OK"]}
+```
+
+A stub that supports this packet must include `jMultiBreakpoint+` in the reply
+to `qSupported`.
+
+**Priority To Implement:** Low. This is a performance optimization, reducing
+the number of packets sent when manipulating breakpoints.
+
 ## jThreadExtendedInfo
 
 This packet, which takes its arguments as JSON and sends its reply as

lldb/include/lldb/Breakpoint/BreakpointSite.h

@@ -77,20 +77,6 @@ class BreakpointSite : public std::enable_shared_from_this<BreakpointSite>,
                        lldb::addr_t *intersect_addr, size_t *intersect_size,
                        size_t *opcode_offset) const;
 
-  /// Tells whether the current breakpoint site is enabled or not
-  ///
-  /// This is a low-level enable bit for the breakpoint sites.  If a
-  /// breakpoint site has no enabled constituents, it should just get removed.
-  /// This enable/disable is for the low-level target code to enable and disable
-  /// breakpoint sites when single stepping, etc.
-  bool IsEnabled() const;
-
-  /// Sets whether the current breakpoint site is enabled or not
-  ///
-  /// \param[in] enabled
-  ///    \b true if the breakpoint is enabled, \b false otherwise.
-  void SetEnabled(bool enabled);
-
   /// Enquires of the breakpoint locations that produced this breakpoint site
   /// whether we should stop at this location.
   ///
@@ -223,6 +209,12 @@ class BreakpointSite : public std::enable_shared_from_this<BreakpointSite>,
   size_t RemoveConstituent(lldb::break_id_t break_id,
                            lldb::break_id_t break_loc_id);
 
+  /// Sets whether the current breakpoint site is enabled or not.
+  ///
+  /// \param[in] enabled
+  ///    \b true if the breakpoint is enabled, \b false otherwise.
+  void SetEnabled(bool enabled);
+
   BreakpointSite::Type m_type; ///< The type of this breakpoint site.
   uint8_t m_saved_opcode[8]; ///< The saved opcode bytes if this breakpoint site
                              ///uses trap opcodes.

lldb/include/lldb/Target/Process.h

@@ -117,6 +117,7 @@ class ProcessProperties : public Properties {
   Args GetAlwaysRunThreadNames() const;
   FollowForkMode GetFollowForkMode() const;
   bool TrackMemoryCacheChanges() const;
+  bool GetUseDelayedBreakpoints() const;
 
 protected:
   Process *m_process; // Can be nullptr for global ProcessProperties
@@ -2236,6 +2237,9 @@ class Process : public std::enable_shared_from_this<Process>,
   // Process Breakpoints
   size_t GetSoftwareBreakpointTrapOpcode(BreakpointSite *bp_site);
 
+  enum class BreakpointAction { Enable, Disable };
+
+protected:
   virtual Status EnableBreakpointSite(BreakpointSite *bp_site) {
     return Status::FromErrorStringWithFormatv(
         "error: {0} does not support enabling breakpoints", GetPluginName());
@@ -2246,6 +2250,24 @@ class Process : public std::enable_shared_from_this<Process>,
         "error: {0} does not support disabling breakpoints", GetPluginName());
   }
 
+  /// Compare BreakpointSiteSPs by ID, so that iteration order is independent
+  /// of pointer addresses.
+  struct SiteIDCmp {
+    bool operator()(const lldb::BreakpointSiteSP &lhs,
+                    const lldb::BreakpointSiteSP &rhs) const {
+      return lhs->GetID() < rhs->GetID();
+    }
+  };
+  using BreakpointSiteToActionMap =
+      std::map<lldb::BreakpointSiteSP, BreakpointAction, SiteIDCmp>;
+
+  virtual llvm::Error
+  UpdateBreakpointSites(const BreakpointSiteToActionMap &site_to_action);
+
+public:
+  llvm::Error ExecuteBreakpointSiteAction(BreakpointSite &site,
+                                          Process::BreakpointAction action);
+
   // This is implemented completely using the lldb::Process API. Subclasses
   // don't need to implement this function unless the standard flow of read
   // existing opcode, write breakpoint opcode, verify breakpoint opcode doesn't
@@ -2274,6 +2296,17 @@ class Process : public std::enable_shared_from_this<Process>,
 
   Status EnableBreakpointSiteByID(lldb::user_id_t break_id);
 
+  bool IsBreakpointSiteEnabled(const BreakpointSite &site);
+
+  bool IsBreakpointSitePhysicallyEnabled(const BreakpointSite &site);
+
+  /// Reports whether this process should delay physically enabling/disabling
+  /// breakpoints until the process is about to resume. The default honors the
+  /// user-facing `target.process.use-delayed-breakpoints` setting.
+  virtual bool ShouldUseDelayedBreakpoints() const {
+    return GetUseDelayedBreakpoints();
+  }
+
   // BreakpointLocations use RemoveConstituentFromBreakpointSite to remove
   // themselves from the constituent's list of this breakpoint sites.
   void RemoveConstituentFromBreakpointSite(lldb::user_id_t site_id,
@@ -3535,6 +3568,21 @@ void PruneThreadPlans();
   /// GetExtendedCrashInformation.
   StructuredData::DictionarySP m_crash_info_dict_sp;
 
+  struct DelayedBreakpointCache {
+    void Enqueue(lldb::BreakpointSiteSP site, BreakpointAction action);
+    void RemoveSite(lldb::BreakpointSiteSP site) {
+      m_site_to_action.erase(site);
+    }
+    void Clear() { m_site_to_action.clear(); }
+
+    BreakpointSiteToActionMap m_site_to_action;
+  };
+
+  DelayedBreakpointCache m_delayed_breakpoints;
+  std::recursive_mutex m_delayed_breakpoints_mutex;
+
+  llvm::Error FlushDelayedBreakpoints();
+
   size_t RemoveBreakpointOpcodesFromBuffer(lldb::addr_t addr, size_t size,
                                            uint8_t *buf) const;
 
@@ -3616,6 +3664,13 @@ void PruneThreadPlans();
 
   void SetAddressableBitMasks(AddressableBits bit_masks);
 
+  // Updates the state of site.
+  // This should be used by derived Process classes after they have changed the
+  // state of a site.
+  void SetBreakpointSiteEnabled(BreakpointSite &site, bool is_enabled = true) {
+    site.SetEnabled(is_enabled);
+  }
+
 private:
   Status DestroyImpl(bool force_kill);
 

lldb/include/lldb/Utility/GDBRemote.h

@@ -43,6 +43,9 @@ class StreamGDBRemote : public StreamString {
   ///     Number of bytes written.
   // TODO: Convert this function to take ArrayRef<uint8_t>
   int PutEscapedBytes(const void *s, size_t src_len);
+
+  /// Equivalent to PutEscapedBytes(str.data(), str.size());
+  int PutEscapedBytes(llvm::StringRef str);
 };
 
 /// GDB remote packet as used by the GDB remote communication history. Packets

lldb/include/lldb/Utility/Stream.h

@@ -225,6 +225,16 @@ class Stream {
   ///     in one statement.
   Stream &operator<<(char ch);
 
+  /// Output the result of a formatv expression to the stream.
+  ///
+  /// \param[in] obj
+  ///     A formatv_object_base produced by llvm::formatv().
+  ///
+  /// \return
+  ///     A reference to this class so multiple things can be streamed
+  ///     in one statement.
+  Stream &operator<<(const llvm::formatv_object_base &obj);
+
   Stream &operator<<(uint8_t uval) = delete;
   Stream &operator<<(uint16_t uval) = delete;
   Stream &operator<<(uint32_t uval) = delete;
@@ -350,8 +360,10 @@ class Stream {
 
   size_t PrintfVarArg(const char *format, va_list args);
 
+  /// Forwards the arguments to llvm::formatv and writes to the stream.
+  /// FIXME: instead of this API, consider using llvm::formatv directly.
   template <typename... Args> void Format(const char *format, Args &&... args) {
-    PutCString(llvm::formatv(format, std::forward<Args>(args)...).str());
+    *this << llvm::formatv(format, std::forward<Args>(args)...);
   }
 
   /// Output a quoted C string value to the stream.

lldb/include/lldb/Utility/StringExtractorGDBRemote.h

@@ -171,6 +171,7 @@ class StringExtractorGDBRemote : public StringExtractor {
     eServerPacketType_jLLDBTraceStop,
     eServerPacketType_jLLDBTraceGetState,
     eServerPacketType_jLLDBTraceGetBinaryData,
+    eServerPacketType_jMultiBreakpoint,
 
     eServerPacketType_qMemTags, // read memory tags
     eServerPacketType_QMemTags, // write memory tags

lldb/packages/Python/lldbsuite/test/lldbreverse.py

@@ -5,6 +5,7 @@
 from lldbsuite.test.gdbclientutils import *
 from lldbsuite.test.lldbgdbproxy import *
 import lldbgdbserverutils
+import json
 import re
 
 
@@ -132,6 +133,10 @@ def respond(self, packet):
             if reply == "OK":
                 self.update_breakpoints(packet)
             return reply
+        if packet.startswith("jMultiBreakpoint:"):
+            reply = self.pass_through(packet)
+            self.update_multi_breakpoints(packet, reply)
+            return reply
         return GDBProxyTestBase.respond(self, packet)
 
     def start_recording(self):
@@ -161,6 +166,23 @@ def update_breakpoints(self, packet):
         else:
             self.breakpoints[t].discard((addr, kind))
 
+    def update_multi_breakpoints(self, packet, reply):
+        # Remove the final ], which is binary-escaping the }.
+        packet = packet[:-1]
+        reply = reply[:-1]
+        body = packet[len("jMultiBreakpoint:") :]
+        requests = json.loads(body)["breakpoint_requests"]
+        try:
+            results = json.loads(reply)["results"]
+        except (ValueError, KeyError):
+            # Empty/unsupported/error reply: nothing was installed.
+            return
+        if len(requests) != len(results):
+            raise ValueError("jMultiBreakpoint response count mismatch")
+        for inner_packet, result in zip(requests, results):
+            if result == "OK":
+                self.update_breakpoints(inner_packet)
+
     def breakpoint_triggered_at(self, pc):
         if any(addr == pc for addr, kind in self.breakpoints[SOFTWARE_BREAKPOINTS]):
             return True

lldb/packages/Python/lldbsuite/test/lldbutil.py

@@ -18,6 +18,7 @@
 import lldb
 from . import lldbtest_config
 from . import configuration
+from lldbsuite.test.gdbclientutils import escape_binary
 
 # How often failed simulator process launches are retried.
 SIMULATOR_RETRY = 3
@@ -1936,3 +1937,24 @@ def ignore_swift_stdlib_when_stepping(platform, tester):
             "settings set "
             "target.process.thread.step-avoid-libraries {}".format(lib_name)
         )
+
+# Binary escapes `packet_str`, sends it to the remote and returns the reply.
+def send_packet_get_reply(test, packet_str):
+    packet_str = escape_binary(packet_str)
+    test.runCmd(f"proc plugin packet send '{packet_str}'", check=False)
+    # The output is of the form:
+    #  packet: <packet_str>
+    #  response: <response>
+    output = test.res.GetOutput()
+    reply = output.split("\n")
+    packet = reply[0].strip()
+    response = reply[1].strip()
+
+    test.assertTrue(packet.startswith("packet: "), output)
+    test.assertTrue(response.startswith("response: "), output)
+    return response[len("response: ") :].strip()
+
+
+def get_qsupported_capabilities(test):
+    reply = send_packet_get_reply(test, "qSupported")
+    return reply.strip().split(";")

lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py

@@ -925,6 +925,7 @@ def add_qSupported_packets(self, client_features=[]):
         "SupportedWatchpointTypes",
         "SupportedCompressions",
         "MultiMemRead",
+        "jMultiBreakpoint",
     ]
 
     def parse_qSupported_response(self, context):

lldb/source/Breakpoint/BreakpointSite.cpp

@@ -135,8 +135,6 @@ const uint8_t *BreakpointSite::GetSavedOpcodeBytes() const {
   return &m_saved_opcode[0];
 }
 
-bool BreakpointSite::IsEnabled() const { return m_enabled; }
-
 void BreakpointSite::SetEnabled(bool enabled) { m_enabled = enabled; }
 
 void BreakpointSite::AddConstituent(const BreakpointLocationSP &constituent) {

lldb/source/Core/UserSettingsController.cpp

@@ -55,7 +55,7 @@ void Properties::DumpAllPropertyValues(const ExecutionContext *exe_ctx,
                                        bool is_json) {
   if (is_json) {
     llvm::json::Value json = m_collection_sp->ToJSON(exe_ctx);
-    strm.Printf("%s", llvm::formatv("{0:2}", json).str().c_str());
+    strm << llvm::formatv("{0:2}", json);
   } else
     m_collection_sp->DumpValue(exe_ctx, strm, dump_mask);
 }

lldb/source/Interpreter/OptionValueProperties.cpp

@@ -364,11 +364,9 @@ Status OptionValueProperties::DumpPropertyValue(const ExecutionContext *exe_ctx,
       if (dump_mask & ~eDumpOptionName)
         strm.PutChar(' ');
     }
-    if (is_json) {
-      strm.Printf(
-          "%s",
-          llvm::formatv("{0:2}", value_sp->ToJSON(exe_ctx)).str().c_str());
-    } else
+    if (is_json)
+      strm << llvm::formatv("{0:2}", value_sp->ToJSON(exe_ctx));
+    else
       value_sp->DumpValue(exe_ctx, strm, dump_mask);
   }
   return error;

lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp

@@ -635,9 +635,9 @@ Status ProcessKDP::EnableBreakpointSite(BreakpointSite *bp_site) {
 
   if (m_comm.LocalBreakpointsAreSupported()) {
     Status error;
-    if (!bp_site->IsEnabled()) {
+    if (!IsBreakpointSitePhysicallyEnabled(*bp_site)) {
       if (m_comm.SendRequestBreakpoint(true, bp_site->GetLoadAddress())) {
-        bp_site->SetEnabled(true);
+        SetBreakpointSiteEnabled(*bp_site);
         bp_site->SetType(BreakpointSite::eExternal);
       } else {
         return Status::FromErrorString("KDP set breakpoint failed");
@@ -651,15 +651,15 @@ Status ProcessKDP::EnableBreakpointSite(BreakpointSite *bp_site) {
 Status ProcessKDP::DisableBreakpointSite(BreakpointSite *bp_site) {
   if (m_comm.LocalBreakpointsAreSupported()) {
     Status error;
-    if (bp_site->IsEnabled()) {
+    if (IsBreakpointSitePhysicallyEnabled(*bp_site)) {
       BreakpointSite::Type bp_type = bp_site->GetType();
       if (bp_type == BreakpointSite::eExternal) {
         if (m_destroy_in_process && m_comm.IsRunning()) {
           // We are trying to destroy our connection and we are running
-          bp_site->SetEnabled(false);
+          SetBreakpointSiteEnabled(*bp_site, false);
         } else {
           if (m_comm.SendRequestBreakpoint(false, bp_site->GetLoadAddress()))
-            bp_site->SetEnabled(false);
+            SetBreakpointSiteEnabled(*bp_site, false);
           else
             return Status::FromErrorString("KDP remove breakpoint failed");
         }