Disable breakpoints before writing memory and re-enable after.

[tatyana-krasnukha]

Summary

Comments in this function didn't match code. Moved it in right place and grouped cases, that need only WriteMemoryPrivate call, at the top of the function.
Applied clang-format.

[tatyana-krasnukha]

source/Target/Process.cpp

2462

This comment says that function will return the number of written bytes, but really it will return zero in this case

2484

This comment relates to line 2476 here

[clayborg]

I believe we normally just clang format the changes. This is done with:

svn diff --diff-cmd=diff -x-U0 | $llvm_src/llvm/tools/clang/tools/clang-format/clang-format-diff.py -i -binary $llvm_build/bin/clang-format
git diff -U0 HEAD^ | $llvm_src/llvm/tools/clang/tools/clang-format/clang-format-diff.py -i -binary $llvm_build/bin/clang-format

You can set $llvm_src and $llvm_build correctly for your system.

With all of the reindentation and extra clang formatting I can't tell what has changed here. Did you change anything?

[clayborg]

Please clang format and check in without any changes to the functionality and then make a patch that only has your functional changes.

[tatyana-krasnukha]

Sorry for wrong formatting, I've removed it.

What I've actually done:

• grouped cases returning WriteMemoryPrivate in the top of function;
• moved last comment to code it relates;
• changed other comment according what really will be returned in that case.

I hope that these changes improve readability and will help to avoid errors like I tried to fix in D39969

[tatyana-krasnukha]

Same for comment in the header file.

[tatyana-krasnukha]

Still have a feeling that it is not enough to clarify for function users that "if (bytes_written != bytes_to_write)" is not always criteria of error, as well as "if (error.Success())" doesn't mean that whole area was written to memory...

[tatyana-krasnukha]

What about having 2 versions:

1. WriteMemory(..., bool force = false), that fails if there are breakpoints in the area. Let user decide what to do.

If force == true just call DoWriteMemory without checking on breakpoints. It is needed while writing on stack, for example (and to all sections beside .text ?).
All usages should be followed with if (status.Success()).

2. WriteMemorySavingBreakpoints for that cases where it is really needed. It will not return bytes_written, only status.

[clayborg]

This will need a test, preferable covering all of the cases:

• breakpoint right at start with extra data after
• breakpoint right at start with no extra data after
• breakpoint in middle of ranges with data before and after
• breakpoint at end of range with no data after
• multiple breakpoints tests with two breakpoints in a row with no data between with no data before first or after last
• multiple breakpoints tests with two breakpoints in a row with no data between with data before first and after last
• multiple breakpoints tests with two discontiguous breakpoints data between them with data before first and after last

Also, when submitted a diff, please add more context using the -U option:

% svn diff -x -U99999 ...
% git diff -U99999 ...

This allows us to see the entire context (I tried to see the context for Process.h, but was unable to see more).

[labath]

A couple of thoughts come to mind:

• Does this functionality really belong in the client? In case of memory reads, it's the server who patches out the breakpoint opcodes (NativeProcessProtocol::ReadMemoryWithoutTrap). I think it would make sense to do this in the same place. How does gdb handle this?
• Another interesting test would be to read back the memory after writing it and make sure it returns sensible data (interaction with server-side patching).
• As for implementation, wouldn't it be simpler to just disable all breakpoint locations within the range, write the memory, and then re-enable them? I don't expect performance to matter much here.

[tatyana-krasnukha]

Does this functionality really belong in the client? In case of memory reads, it's the server who patches out the breakpoint opcodes (NativeProcessProtocol::ReadMemoryWithoutTrap). I think it would make sense to do this in the same place.

Will not work for gdb-remote mode, other servers treat this just as a block of memory.
I might be wrong, but gdb inserts a breakpoint right before execution of instruction range, containing this breakpoint, and removes right after stop.

What is the need to save breakpoints in general? Because when memory is overwritten, breakpoints may have no sense anymore at their locations...

[clayborg]

In D39967#984989, @tatyana-krasnukha wrote:

Does this functionality really belong in the client? In case of memory reads, it's the server who patches out the breakpoint opcodes (NativeProcessProtocol::ReadMemoryWithoutTrap). I think it would make sense to do this in the same place.

Will not work for gdb-remote mode, other servers treat this just as a block of memory.
I might be wrong, but gdb inserts a breakpoint right before execution of instruction range, containing this breakpoint, and removes right after stop.

That is why GDB doesn't do well if you set thousands of breakpoints. It makes debugging so slow and painful that it is useless. With LLDB we have the ability to set many breakpoints and still be able to debug quite well. LLDB is being used for program guided optimizations where we set a breakpoint on every function in a shared library and as each breakpoint location gets hit, we disable it. So while the GDB method makes things easier, it is quite inefficient when it is so easy to work around by knowing how to work around software breakpoints. So I like the way LLDB does it over GDB. Debugging on remote devices (like and Apple Watch) is quite slow to begin with and every packet counts. If we even set 20 breakpoints, that can easily add over 1 second per stop and 1 second per resume due to packets being limited to 20-40 packets per second.

What is the need to save breakpoints in general? Because when memory is overwritten, breakpoints may have no sense anymore at their locations...

[tatyana-krasnukha]

I completely agree with your point, but why isn't enough just to return an error about breakpoints in the area user wants to write? Or to disable breakpoints forcibly?

[clayborg]

In D39967#985119, @tatyana-krasnukha wrote:

I completely agree with your point, but why isn't enough just to return an error about breakpoints in the area user wants to write?

The reason I don't like this is there is no way for a user to look at the error returned and do anything sensible. The only thing they can do is scrape the error string to see if it contains something about breakpoints? I am a firm believer that the debugger needs to do what is right for the API call to work no matter what it is doing for breakpoints since the breakpoints are a side effect of using the debugger.

Or to disable breakpoints forcibly?

That would be an easy fix for the ObjectFile::Load(), we could see if there are any breakpoints in the range we are trying to write, get a list of them, disable them all, write memory and re-enable. But this doesn't stop a user from trying doing something like:

(lldb) script lldb.process.WriteMemory(...)

And they should expect this to just work no matter where they write memory.

[labath]

In D39967#985171, @clayborg wrote:

That would be an easy fix for the ObjectFile::Load(), we could see if there are any breakpoints in the range we are trying to write, get a list of them, disable them all, write memory and re-enable. But this doesn't stop a user from trying doing something like:

(lldb) script lldb.process.WriteMemory(...)

And they should expect this to just work no matter where they write memory.

Couldn't WriteMemory do the same thing as well? I would expect that 99% of WriteMemory calls will not intersect any breakpoints so the overhead of disabling and re-enabling should be negligible.

[clayborg]

In D39967#985181, @labath wrote:

In D39967#985171, @clayborg wrote:

That would be an easy fix for the ObjectFile::Load(), we could see if there are any breakpoints in the range we are trying to write, get a list of them, disable them all, write memory and re-enable. But this doesn't stop a user from trying doing something like:

(lldb) script lldb.process.WriteMemory(...)

And they should expect this to just work no matter where they write memory.

Couldn't WriteMemory do the same thing as well? I would expect that 99% of WriteMemory calls will not intersect any breakpoints so the overhead of disabling and re-enabling should be negligible.

Indeed the fix could do this to make this easier and I agree that would be a cleaner fix. We already have all the APIs we need for this. Tatyana, let us know what you think of changing this patch to get a list of overlapping breakpoints, disable, write memory, re-enable. All from within Process::WriteMemory

[tatyana-krasnukha]

Cannot promise that I'll do it (with all tests) quickly, but I'll do.

One more question: what are the cases when intersection can happen, beside user forgot to disable it manually? (Will not it be annoying for user to get breakpoints in unpredictable locations after such situation?)

[tatyana-krasnukha]

Disable breakpoints before writing memory and re-enable after.

I found that bp_site_list filled by BreakpointSiteList::FindInRange has its own mutex and doesn’t hold mutex of original list. Thus changed return type to Guard to avoid potential races.

Also, working on tests, I found that the trick (when breakpoint overlaps bottom end of the range) in BreakpointSiteList::FindInRange doesn’t work if that breakpoint is last in container: in this case m_bp_site_list.lower_bound returns end and iterator doesn’t pass next check. So, I moved the check after code that grabs previous element.

[tatyana-krasnukha]

pass callbacks by const-reference

[tatyana-krasnukha]

diff with more context

[clayborg]

So one issue with this is some things, like GDB servers, might not need you to disable breakpoints in a range because they actually set the breakpoint for you and you don't know what opcode they used. I am hoping this works with that and doesn't cause us to manually write to memory when we don't need to by forcing every process subclass to always to the manual re-writing of the breakpoint opcodes.

include/lldb/Breakpoint/BreakpointSiteList.h

129–130

Have this function return a bool where true means to keep iterating, and false means to stop iterating. This is how many other ForEach functions are done in LLDB and allows the ForEach to be used to linearly search a list and stop when done.

132–133

Ditto

[labath]

include/lldb/Breakpoint/BreakpointSiteList.h

159–175

How is this class different from a std::unique_lock ?

[tatyana-krasnukha]

In D39967#1015860, @clayborg wrote:

So one issue with this is some things, like GDB servers, might not need you to disable breakpoints in a range because they actually set the breakpoint for you and you don't know what opcode they used. I am hoping this works with that and doesn't cause us to manually write to memory when we don't need to by forcing every process subclass to always to the manual re-writing of the breakpoint opcodes.

It seems I had to use Disable|EnableBreakpointSite instead of Disable|EnableSoftwareBreakpoint...

include/lldb/Breakpoint/BreakpointSiteList.h

159–175

std::unique_lock allows to call lock() and unlock() manually. Such flexibility makes code more bug-prone, so I try to avoid it if it is not really needed.

I might inherit Guard from std::unique_lock and hide these functions, if it looks better...

[tatyana-krasnukha]

Use Disable|EnableBreakpointSite instead of Disable|EnableSoftwareBreakpoint.
Make BreakpointSiteList::ForEach callbacks return bool.

[zturner]

include/lldb/Breakpoint/BreakpointSiteList.h

159–175

In that case use std::lock_guard. it is the same as unique_lock but doesn't allow calling lock and unlock. So it sounds like exactly what you want.

[tatyana-krasnukha]

include/lldb/Breakpoint/BreakpointSiteList.h

159–175

Unfortunately, it is not movable

[xiaobai]

source/Breakpoint/BreakpointSiteList.cpp

197

If m_bp_site_list is empty, prev_pos could be m_bp_site_list.end(), I believe, so you would have to check for that here. This is assuming that this method can be invoked when m_bp_site_list is empty, which I'm not entirely sure about.

source/Target/Process.cpp

1815–1816

Good opportunity to delete dead code?

2436

If disabling any breakpoint fails, the status's error string will be "failed to re-enable one or more breakpoints". This is kind of confusing, imo.

unittests/Process/common/ProcessWriteMemoryTest.cpp

2

ProcessTraceMonitorTest.cpp -> ProcessWriteMemoryTest.cpp

[zturner]

Personally I think it would be clearer to just use std::unique_lock<>. Already it's locking the mutex twice, once with a lock_guard and once when creating a BreakpointSiteList::Guard. Which works I guess because it's a recursive mutex, but it's still confusing. I would vote to just make a std::unique_lock and then return std::move(guard);

It might be true that allowing the use of manual lock and unlock is unsafe, but adding additional code also has some cost.

[tatyana-krasnukha]

source/Breakpoint/BreakpointSiteList.cpp

197

If m_bp_site_list is empty, lower == begin() and we never fall in this code, isn't it?

[xiaobai]

source/Breakpoint/BreakpointSiteList.cpp

197

Oh, I think you're correct here. I forgot to account for that, apologies for the noise.

[tatyana-krasnukha]

Update according suggestions above

[tatyana-krasnukha]

In D39967#1016533, @zturner wrote:

It might be true that allowing the use of manual lock and unlock is unsafe, but adding additional code also has some cost.

Just look: only 3 additional lines of code;)

[zturner]

include/lldb/Breakpoint/BreakpointSiteList.h

159–161

Err, I meant to just deleted the Guard class entirely and return llvm::Optional<std::unique_lock<std::recursive_mutex>>

[tatyana-krasnukha]

source/Target/Process.cpp

2436

Didn't found anything better than use word "update" here. Introducing additional flag looks ugly...

I hope, that this information is enough when user tries to write memory. More detailed description can be found in log.

[tatyana-krasnukha]

Remove Guard class completely

[tatyana-krasnukha]

Fix naming

[labath]

include/lldb/Breakpoint/BreakpointSiteList.h

159–161

You don't even need the Optional class, unique_lock already has an uninitialized state.

[tatyana-krasnukha]

Remove Optional from return type of FindInRange

[clayborg]

include/lldb/Breakpoint/BreakpointSiteList.h

130

I was a bit vague with my explanations. Only ModifyingCallback needs to return a bool. No need for the ForEach function itself to return a bool, just the callback.

133

Ditto, no need to return bool from ForEach.

[tatyana-krasnukha]

include/lldb/Breakpoint/BreakpointSiteList.h

130

If ForEach didn't apply callback for each actually, shouldn't it report an error?

[tatyana-krasnukha]

Change ForEach return type (bool -> void)

[clayborg]

So the question still remains: does this need to be done when debugging with debugserver or lldb-server? I can't remember if memory write works around breakpoint opcodes in debugserver and/or lldb-server. If it does, then this code is only needed for targets that don't work around software breakpoints.

[clayborg]

If any of our lldb_private::Process subclasses handle this already, then we need a new virtual function on lldb_private::Process like:

virtual bool lldb_private::Process::MemoryWorksAroundSoftwareBreakpoints(bool read);

If "read" is true then we are asking about memory reads and if "read" is false, then we are asking about writes. Then the code you added to disable breakpoint locations and re-enable them only needs to happen if they return false to calling this function with "read == false".

[tatyana-krasnukha]

May be I misunderstand your question, but I checked, that all targets of both lldb-server and debugserver read opcode from memory and save it, when enable software breakpoint.

[ted]

Traditionally, remote gdbservers handle memory operations that overlap a software breakpoint. Writes get the new value saved off, and reads get the breakpoint opcode replaced by the saved value.

[clayborg]

In D39967#1017732, @ted wrote:

Traditionally, remote gdbservers handle memory operations that overlap a software breakpoint. Writes get the new value saved off, and reads get the breakpoint opcode replaced by the saved value.

Indeed. Breakpoints are set using the "Z" and "z" packets to the GDB server so we don't even know which opcode gets used for the breakpoint instruction. My guess is when we use the "Z" and "z" packets, we don't have any saved breakpoint opcode in the breakpoint site info and this code won't do anything. Would be good to verify this.

[tatyana-krasnukha]

Got it. Yes, you are right. At least debugserver works around breakpoints by himself while writing memory.
I will handle this case.

[labath]

I believe lldb-server does not work around breakpoint opcodes during writing (it does for reading), but that can be fixed, of course.

[ted]

In D39967#1017754, @labath wrote:

I believe lldb-server does not work around breakpoint opcodes during writing (it does for reading), but that can be fixed, of course.

It should. If it doesn't, then a write over a breakpoint would clobber it.

What should happen is lldb-server finds all breakpoint locations in a write, and replaces the saved opcode for each such breakpoint with the corresponding value in the write, then writes the data to memory. Breakpoint addresses can either be skipped, or have the opcode in them replaced by the breakpoint opcode before the write.

[tatyana-krasnukha]

Have no idea how to know about working around breakpoints for read and write separately. Just rely on support of Z-packets for now.
This expectation could be fair, however...

In D39967#1017764, @ted wrote:

It should. If it doesn't, then a write over a breakpoint would clobber it.

it doesn't for now.

include/lldb/Breakpoint/BreakpointSiteList.h

 #ifndef liblldb_BreakpointSiteList_h_
 #define liblldb_BreakpointSiteList_h_
 
 // C Includes
 // C++ Includes
 #include <functional>
 #include <map>
 #include <mutex>
+#include <type_traits>
 
 // Other libraries and framework includes
 // Project includes
 #include "lldb/Breakpoint/BreakpointSite.h"
 
 namespace lldb_private {
 
 //----------------------------------------------------------------------
 /// @class BreakpointSiteList BreakpointSiteList.h
 /// "lldb/Breakpoint/BreakpointSiteList.h"
 /// @brief Class that manages lists of BreakpointSite shared pointers.
 //----------------------------------------------------------------------
 class BreakpointSiteList {
-  // At present Process directly accesses the map of BreakpointSites so it can
-  // do quick lookups into the map (using GetMap).
-  // FIXME: Find a better interface for this.
-  friend class Process;
-
 public:
   //------------------------------------------------------------------
   /// Default constructor makes an empty list.
   //------------------------------------------------------------------
-  BreakpointSiteList();
+  BreakpointSiteList() = default;
 
   //------------------------------------------------------------------
   /// Destructor, currently does nothing.
   //------------------------------------------------------------------
-  ~BreakpointSiteList();
+  ~BreakpointSiteList() = default;
 
   //------------------------------------------------------------------
   /// Add a BreakpointSite to the list.
   ///
   /// @param[in] bp_site_sp
   ///    A shared pointer to a breakpoint site being added to the list.
   ///
   /// @return
public:
   ///
   /// @result
   ///   True if \a bp_site_id exists in the site list AND \a bp_id is one of the
   ///   owners of that site.
   //------------------------------------------------------------------
   bool BreakpointSiteContainsBreakpoint(lldb::break_id_t bp_site_id,
                                         lldb::break_id_t bp_id);
 
-  void ForEach(std::function<void(BreakpointSite *)> const &callback);
+  using ModifyingCallback = std::function<bool(BreakpointSite &)>;
+  bool ForEach(const ModifyingCallback &callback);
+
+  using Callback = std::function<bool(const BreakpointSite &)>;
+  bool ForEach(const Callback &callback) const;
 
   //------------------------------------------------------------------
   /// Removes the breakpoint site given by \b breakID from this list.
   ///
   /// @param[in] breakID
   ///   The breakpoint site index to remove.
   ///
   /// @result
   ///   \b true if the breakpoint site \a breakID was in the list.
   //------------------------------------------------------------------
   bool Remove(lldb::break_id_t breakID);
 
   //------------------------------------------------------------------
   /// Removes the breakpoint site at address \a addr from this list.
   ///
   /// @param[in] addr
   ///   The address from which to remove a breakpoint site.
   ///
   /// @result
   ///   \b true if \a addr had a breakpoint site to remove from the list.
   //------------------------------------------------------------------
   bool RemoveByAddress(lldb::addr_t addr);
 
-  bool FindInRange(lldb::addr_t lower_bound, lldb::addr_t upper_bound,
-                   BreakpointSiteList &bp_site_list) const;
-
-  typedef void (*BreakpointSiteSPMapFunc)(lldb::BreakpointSiteSP &bp,
-                                          void *baton);
+  using Range = std::list<lldb::BreakpointSiteSP>;
+  using Filter = std::add_pointer<bool(const BreakpointSite &)>::type;
+  class Guard : private std::unique_lock<std::recursive_mutex> {
+    using std::unique_lock<std::recursive_mutex>::unique_lock;
+  };
 
+  llvm::Optional<Guard> FindInRange(lldb::addr_t lower_bound,
+                                    lldb::addr_t upper_bound,
+                                    Range &bp_site_list,
+                                    Filter filter = nullptr) const;
   //------------------------------------------------------------------
   /// Enquires of the breakpoint site on in this list with ID \a breakID whether
   /// we should stop for the breakpoint or not.
   ///
   /// @param[in] context
   ///    This contains the information about this stop.
   ///
   /// @param[in] breakID
   ///    This break ID that we hit.
   ///
   /// @return
   ///    \b true if we should stop, \b false otherwise.
   //------------------------------------------------------------------
   bool ShouldStop(StoppointCallbackContext *context, lldb::break_id_t breakID);
 
   //------------------------------------------------------------------
   /// Returns the number of elements in the list.

source/Breakpoint/BreakpointSiteList.cpp

 //===-- BreakpointSiteList.cpp ----------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Breakpoint/BreakpointSiteList.h"
 
 // C Includes
 // C++ Includes
+#include <algorithm>
+#include <iterator>
 // Other libraries and framework includes
 // Project includes
 #include "lldb/Utility/Stream.h"
-#include <algorithm>
 
 using namespace lldb;
 using namespace lldb_private;
 
-BreakpointSiteList::BreakpointSiteList() : m_mutex(), m_bp_site_list() {}
-
-BreakpointSiteList::~BreakpointSiteList() {}
-
 // Add breakpoint site to the list.  However, if the element already exists in
-// the
-// list, then we don't add it, and return LLDB_INVALID_BREAK_ID.
+// the list, then we don't add it, and return LLDB_INVALID_BREAK_ID.
 
 lldb::break_id_t BreakpointSiteList::Add(const BreakpointSiteSP &bp) {
   lldb::addr_t bp_site_load_addr = bp->GetLoadAddress();
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   collection::iterator iter = m_bp_site_list.find(bp_site_load_addr);
 
   if (iter == m_bp_site_list.end()) {
     m_bp_site_list.insert(iter, collection::value_type(bp_site_load_addr, bp));

 }
 
 void BreakpointSiteList::Dump(Stream *s) const {
   s->Printf("%p: ", static_cast<const void *>(this));
   // s->Indent();
   s->Printf("BreakpointSiteList with %u BreakpointSites:\n",
             (uint32_t)m_bp_site_list.size());
   s->IndentMore();
-  collection::const_iterator pos;
-  collection::const_iterator end = m_bp_site_list.end();
-  for (pos = m_bp_site_list.begin(); pos != end; ++pos)
-    pos->second.get()->Dump(s);
+
+  for (const auto &pair : m_bp_site_list)
+    pair.second->Dump(s);
   s->IndentLess();
 }
 
-void BreakpointSiteList::ForEach(
-    std::function<void(BreakpointSite *)> const &callback) {
+bool BreakpointSiteList::ForEach(const ModifyingCallback &callback) {
+  std::lock_guard<std::recursive_mutex> guard(m_mutex);
+  for (auto &pair : m_bp_site_list)
+    if (!callback(*pair.second))
+      return false;
+
+  return true;
+}
+
+bool BreakpointSiteList::ForEach(const Callback &callback) const {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
-  for (auto pair : m_bp_site_list)
-    callback(pair.second.get());
+  for (const auto &pair : m_bp_site_list)
+    if (!callback(*pair.second))
+      return false;
+
+  return true;
 }
 
-bool BreakpointSiteList::FindInRange(lldb::addr_t lower_bound,
-                                     lldb::addr_t upper_bound,
-                                     BreakpointSiteList &bp_site_list) const {
+llvm::Optional<BreakpointSiteList::Guard>
+BreakpointSiteList::FindInRange(lldb::addr_t lower_bound,
+                                BreakpointSiteList::Range &bp_site_list,
+                                BreakpointSiteList::Filter filter) const {
   if (lower_bound > upper_bound)
-    return false;
+    return llvm::None;
 
-  std::lock_guard<std::recursive_mutex> guard(m_mutex);
-  collection::const_iterator lower, upper, pos;
-  lower = m_bp_site_list.lower_bound(lower_bound);
-  if (lower == m_bp_site_list.end() || (*lower).first >= upper_bound)
-    return false;
+  Guard guard(m_mutex);
+  auto lower = m_bp_site_list.lower_bound(lower_bound);
 
   // This is one tricky bit.  The breakpoint might overlap the bottom end of the
-  // range.  So we grab the
-  // breakpoint prior to the lower bound, and check that that + its byte size
-  // isn't in our range.
+  // range.  So we grab the breakpoint prior to the lower bound,
+  // and check that that + its byte size isn't in our range.
   if (lower != m_bp_site_list.begin()) {
-    collection::const_iterator prev_pos = lower;
-    prev_pos--;
+    auto prev_pos = std::prev(lower);
     const BreakpointSiteSP &prev_bp = (*prev_pos).second;
     if (prev_bp->GetLoadAddress() + prev_bp->GetByteSize() > lower_bound)
-      bp_site_list.Add(prev_bp);
+      lower = prev_pos;
   }
 
-  upper = m_bp_site_list.upper_bound(upper_bound);
+  if (lower == m_bp_site_list.end() || (*lower).first >= upper_bound)
+    return llvm::None;
+
+  auto upper = m_bp_site_list.upper_bound(upper_bound);
 
-  for (pos = lower; pos != upper; pos++) {
-    bp_site_list.Add((*pos).second);
+  for (auto pos = lower; pos != upper; ++pos) {
+    if (nullptr == filter || filter(*(*pos).second))
+      bp_site_list.push_back((*pos).second);
   }
-  return true;
+
+  return std::move(guard);
 }

source/Target/Process.cpp

 BreakpointSiteList &Process::GetBreakpointSiteList() {
   return m_breakpoint_site_list;
 }
 
 const BreakpointSiteList &Process::GetBreakpointSiteList() const {
   return m_breakpoint_site_list;
 }
 
 void Process::DisableAllBreakpointSites() {
-  m_breakpoint_site_list.ForEach([this](BreakpointSite *bp_site) -> void {
-    //        bp_site->SetEnabled(true);
-    DisableBreakpointSite(bp_site);
+  m_breakpoint_site_list.ForEach([this](BreakpointSite &bp_site) {
+    return DisableBreakpointSite(&bp_site).Success();
   });
 }
 
 Status Process::ClearBreakpointSiteByID(lldb::user_id_t break_id) {
   Status error(DisableBreakpointSiteByID(break_id));
 
   if (error.Success())
     m_breakpoint_site_list.Remove(break_id);
if (IsAlive())
       DisableBreakpointSite(bp_site_sp.get());
     m_breakpoint_site_list.RemoveByAddress(bp_site_sp->GetLoadAddress());
   }
 }
 
 size_t Process::RemoveBreakpointOpcodesFromBuffer(addr_t bp_addr, size_t size,
                                                   uint8_t *buf) const {
   size_t bytes_removed = 0;
-  BreakpointSiteList bp_sites_in_range;
+  BreakpointSiteList::Range bp_sites_in_range;
 
-  if (m_breakpoint_site_list.FindInRange(bp_addr, bp_addr + size,
-                                         bp_sites_in_range)) {
-    bp_sites_in_range.ForEach([bp_addr, size,
-                               buf](BreakpointSite *bp_site) -> void {
-      if (bp_site->GetType() == BreakpointSite::eSoftware) {
-        addr_t intersect_addr;
-        size_t intersect_size;
-        size_t opcode_offset;
-        if (bp_site->IntersectsRange(bp_addr, size, &intersect_addr,
-                                     &intersect_size, &opcode_offset)) {
-          assert(bp_addr <= intersect_addr && intersect_addr < bp_addr + size);
-          assert(bp_addr < intersect_addr + intersect_size &&
-                 intersect_addr + intersect_size <= bp_addr + size);
-          assert(opcode_offset + intersect_size <= bp_site->GetByteSize());
-          size_t buf_offset = intersect_addr - bp_addr;
-          ::memcpy(buf + buf_offset,
-                   bp_site->GetSavedOpcodeBytes() + opcode_offset,
-                   intersect_size);
-        }
-      }
-    });
-  }
+  auto optional_lock = m_breakpoint_site_list.FindInRange(
+      bp_addr, bp_addr + size, bp_sites_in_range,
+      [](const BreakpointSite &bp_site) { return bp_site.IsEnabled(); });
+
+  for (auto &bp_site : bp_sites_in_range) {
+      ::memcpy(buf + buf_offset, bp_site->GetSavedOpcodeBytes() + opcode_offset,
   return bytes_removed;
 }
 
 size_t Process::GetSoftwareBreakpointTrapOpcode(BreakpointSite *bp_site) {
   PlatformSP platform_sp(GetTarget().GetPlatform());
   if (platform_sp)
     return platform_sp->GetSoftwareBreakpointTrapOpcode(GetTarget(), bp_site);
   return 0;
while (bytes_written < size) {
     bytes_written += curr_bytes_written;
     if (curr_bytes_written == curr_size || curr_bytes_written == 0)
       break;
   }
   return bytes_written;
 }
 
 size_t Process::WriteMemory(addr_t addr, const void *buf, size_t size,
-                            Status &error) {
+                            Status &status) {
 #if defined(ENABLE_MEMORY_CACHING)
   m_memory_cache.Flush(addr, size);
 #endif
 
   if (buf == nullptr || size == 0)
     return 0;
 
   m_mod_id.BumpMemoryID();
 
-  // We need to write any data that would go where any current software traps
-  // (enabled software breakpoints) any software traps (breakpoints) that we
-  // may have placed in our tasks memory.
-
-  BreakpointSiteList bp_sites_in_range;
-
-  if (m_breakpoint_site_list.FindInRange(addr, addr + size,
-                                         bp_sites_in_range)) {
-    // No breakpoint sites overlap
-    if (bp_sites_in_range.IsEmpty())
-      return WriteMemoryPrivate(addr, buf, size, error);
-    else {
-      const uint8_t *ubuf = (const uint8_t *)buf;
-      uint64_t bytes_written = 0;
-
-      bp_sites_in_range.ForEach([this, addr, size, &bytes_written, &ubuf,
-                                 &error](BreakpointSite *bp) -> void {
+  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
+  auto update_status = [&status, log, this](const Status &op_status) {
+    if (op_status.Fail()) {
+      if (log)
+        log->Printf("Process pid %" PRIu64 ": %s", GetID(),
+                    op_status.AsCString());
 
-        if (error.Success()) {
-          addr_t intersect_addr;
-          size_t intersect_size;
-          size_t opcode_offset;
-          const bool intersects = bp->IntersectsRange(
-              addr, size, &intersect_addr, &intersect_size, &opcode_offset);
-          UNUSED_IF_ASSERT_DISABLED(intersects);
-          assert(intersects);
-          assert(addr <= intersect_addr && intersect_addr < addr + size);
-          assert(addr < intersect_addr + intersect_size &&
-                 intersect_addr + intersect_size <= addr + size);
-          assert(opcode_offset + intersect_size <= bp->GetByteSize());
-
-          // Check for bytes before this breakpoint
-          const addr_t curr_addr = addr + bytes_written;
-          if (intersect_addr > curr_addr) {
-            // There are some bytes before this breakpoint that we need to
-            // just write to memory
-            size_t curr_size = intersect_addr - curr_addr;
-            size_t curr_bytes_written = WriteMemoryPrivate(
-                curr_addr, ubuf + bytes_written, curr_size, error);
-            bytes_written += curr_bytes_written;
-            if (curr_bytes_written != curr_size) {
-              // We weren't able to write all of the requested bytes, we
-              // are done looping and will return the number of bytes that
-              // we have written so far.
-              if (error.Success())
-                error.SetErrorToGenericError();
-            }
-          }
-          // Now write any bytes that would cover up any software breakpoints
-          // directly into the breakpoint opcode buffer
-          ::memcpy(bp->GetSavedOpcodeBytes() + opcode_offset,
-                   ubuf + bytes_written, intersect_size);
-          bytes_written += intersect_size;
-        }
-      });
+      if (!status.Fail())
+        status.SetErrorString(
+            "failed to update one or more breakpoints in range");
+  };
 
-      if (bytes_written < size)
-        WriteMemoryPrivate(addr + bytes_written, ubuf + bytes_written,
-                           size - bytes_written, error);
-    }
-  } else {
-    return WriteMemoryPrivate(addr, buf, size, error);
+  BreakpointSiteList::Range enabled_bp_sites_in_range;
+
+  auto optional_lock = m_breakpoint_site_list.FindInRange(
+      addr, addr + size, enabled_bp_sites_in_range,
+      [](const BreakpointSite &bp_site) { return bp_site.IsEnabled(); });
+
+  for (auto &bp_site : enabled_bp_sites_in_range)
+    update_status(DisableBreakpointSite(bp_site.get()));
+
+  size_t written = WriteMemoryPrivate(addr, buf, size, status);
+
+  if (status.Success()) {
+    for (auto &bp_site : enabled_bp_sites_in_range)
+      update_status(EnableBreakpointSite(bp_site.get()));
   }
 
-  // Write any remaining bytes after the last breakpoint if we have any left
-  return 0; // bytes_written;
+  return written;
 }
 
 size_t Process::WriteScalarToMemory(addr_t addr, const Scalar &scalar,
                                     size_t byte_size, Status &error) {
   if (byte_size == UINT32_MAX)
     byte_size = scalar.GetByteSize();
   if (byte_size > 0) {
     uint8_t buf[32];

unittests/Process/CMakeLists.txt

+add_subdirectory(common)
 add_subdirectory(gdb-remote)
 if (CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
   add_subdirectory(Linux)
 endif()
 add_subdirectory(minidump)

unittests/Process/common/CMakeLists.txt

+add_lldb_unittest(LLDBProcessTests
+  ProcessWriteMemoryTest.cpp
+
+  LINK_LIBS
+    lldbBreakpoint
+    lldbTarget
+  )
 No newline at end of file

unittests/Process/common/ProcessWriteMemoryTest.cpp

+//===-- ProcessorWriteMemoryTest.cpp -------------------------- -*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Breakpoint/BreakpointLocation.h"
+#include "lldb/Breakpoint/BreakpointResolver.h"
+#include "lldb/Core/Debugger.h"
+#include "lldb/Core/PluginManager.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/Process.h"
+
+#include "gtest/gtest.h"
+
+#include <array>
+#include <cstring>
+#include <memory>
+#include <vector>
+
+using Status = lldb_private::Status;
+
+class FakeProcess : public lldb_private::Process {
+  using lldb_private::Process::Process;
+
+public:
+  static lldb_private::ConstString GetName() {
+    return lldb_private::ConstString("fake-process");
+  }
+
+  size_t ReadMemoryAsIs(lldb::addr_t addr, void *buf, size_t size,
+                        Status &error) {
+    return DoReadMemory(addr, buf, size, error);
+  }
+
+private:
+  lldb_private::ConstString GetPluginName() override { return GetName(); }
+
+  uint32_t GetPluginVersion() override { return 0; }
+
+  bool CanDebug(lldb::TargetSP, bool) override { return true; }
+  Status DoDestroy() override { return Status(); }
+  void RefreshStateAfterStop() override {}
+
+  size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
+                      Status &error) override {
+    auto mem_size = m_memory.size();
+    auto to_copy =
+        addr + size <= mem_size ? size : addr >= mem_size ? 0 : mem_size - addr;
+    memcpy(buf, m_memory.data() + addr, to_copy);
+    memset(static_cast<uint8_t *>(buf) + to_copy, 0, size - to_copy);
+    return size;
+  }
+
+  size_t DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size,
+                       Status &error) override {
+    auto mem_size = m_memory.size();
+    if (addr + size > mem_size)
+      m_memory.resize(addr + size);
+
+    memcpy(m_memory.data() + addr, buf, size);
+    return size;
+  }
+
+  Status EnableBreakpointSite(lldb_private::BreakpointSite *bp_site) override {
+    return EnableSoftwareBreakpoint(bp_site);
+  }
+
+  Status DisableBreakpointSite(lldb_private::BreakpointSite *bp_site) override {
+    return DisableSoftwareBreakpoint(bp_site);
+  }
+
+  bool UpdateThreadList(lldb_private::ThreadList &old_thread_list,
+                        lldb_private::ThreadList &new_thread_list) override {
+    new_thread_list.Update(old_thread_list);
+    return true;
+  }
+
+  std::vector<uint8_t> m_memory;
+};
+
+using FakeProcessSP = std::shared_ptr<FakeProcess>;
+
+class FakePlatform : public lldb_private::Platform {
+public:
+  FakePlatform() : lldb_private::Platform(false) {}
+
+  using TrapOpcode = std::array<uint8_t, 4>;
+  static const TrapOpcode &GetTrapOpcode() {
+    static const TrapOpcode trap_opcode = {0x1, 0x2, 0x3, 0x4};
+    return trap_opcode;
+  }
+
+private:
+  size_t GetSoftwareBreakpointTrapOpcode(
+      lldb_private::Target &target,
+      lldb_private::BreakpointSite *bp_site) override {
+    assert(bp_site);
+    auto &trap_opcode = GetTrapOpcode();
+    return bp_site->SetTrapOpcode(trap_opcode.data(), trap_opcode.size())
+               ? trap_opcode.size()
+               : 0;
+  }
+  const char *GetDescription() override { return ""; }
+
+  lldb_private::ConstString GetPluginName() override {
+    return lldb_private::ConstString();
+  }
+
+  uint32_t GetPluginVersion() override { return 0; }
+
+  bool GetSupportedArchitectureAtIndex(uint32_t,
+                                       lldb_private::ArchSpec &) override {
+    return false;
+  }
+
+  lldb::ProcessSP Attach(lldb_private::ProcessAttachInfo &,
+                         lldb_private::Debugger &debugger,
+                         lldb_private::Target *target, Status &error) {
+    return lldb::ProcessSP();
+  }
+
+  void CalculateTrapHandlerSymbolNames() override {}
+};
+
+struct WriteMemoryTest : public ::testing::Test {
+  void SetUp() override {
+    lldb_private::HostInfo::Initialize();
+
+    lldb_private::PluginManager::RegisterPlugin(
+        FakeProcess::GetName(), nullptr,
+        [](lldb::TargetSP target, lldb::ListenerSP listener,
+           const lldb_private::FileSpec *) {
+          return lldb::ProcessSP(new FakeProcess(target, listener));
+        });
+
+    platform = std::make_shared<FakePlatform>();
+    lldb_private::Platform::SetHostPlatform(platform);
+
+    debugger = lldb_private::Debugger::CreateInstance(nullptr, nullptr);
+    debugger->GetPlatformList().SetSelectedPlatform(platform);
+    debugger->GetTargetList().CreateTarget(*debugger, llvm::StringRef(),
+                                           lldb_private::ArchSpec(), false,
+                                           platform, target);
+
+    ASSERT_TRUE(target);
+
+    process = std::static_pointer_cast<FakeProcess>(
+        target->CreateProcess(debugger->GetListener(),
+                              FakeProcess::GetName().GetStringRef(), nullptr));
+    ASSERT_TRUE(process);
+    InitializeBuffers();
+  }
+
+  void TearDown() override { lldb_private::HostInfo::Terminate(); }
+
+  void SetBreakpoint(lldb::addr_t addr) {
+    ASSERT_TRUE(
+        target->CreateBreakpoint(lldb_private::Address(addr), false, false));
+  }
+
+  void CheckSavedOpcodes();
+  void CheckTrapOpcode(lldb::addr_t addr);
+  void WriteAndCheckData(lldb::addr_t addr, size_t count);
+
+  void InitializeBuffers() {
+    wbuf.fill(0xffu);
+    rbuf.fill(0u);
+  }
+
+  lldb::DebuggerSP debugger;
+  lldb::PlatformSP platform;
+  lldb::TargetSP target;
+  FakeProcessSP process;
+  std::array<uint8_t, 16> wbuf, rbuf;
+  const FakePlatform::TrapOpcode &trap_opcode = FakePlatform::GetTrapOpcode();
+};
+
+void WriteMemoryTest::CheckSavedOpcodes() {
+  auto result = process->GetBreakpointSiteList().ForEach(
+      [this](const lldb_private::BreakpointSite &bp_site) {
+        bool are_same =
+            (0 == memcmp(bp_site.GetSavedOpcodeBytes(),
+                         wbuf.data() + bp_site.GetLoadAddress(),
+                         bp_site.GetByteSize()));
+        return are_same;
+      });
+  ASSERT_TRUE(result);
+}
+
+void WriteMemoryTest::CheckTrapOpcode(lldb::addr_t addr) {
+  Status status;
+  auto read =
+      process->ReadMemoryAsIs(addr, rbuf.data(), trap_opcode.size(), status);
+  ASSERT_TRUE(status.Success() && trap_opcode.size() == read);
+  ASSERT_TRUE(0 == memcmp(trap_opcode.data(), rbuf.data(), trap_opcode.size()));
+}
+
+void WriteMemoryTest::WriteAndCheckData(lldb::addr_t addr, size_t count) {
+  Status status;
+  auto written = process->WriteMemory(addr, wbuf.data() + addr, count, status);
+  ASSERT_TRUE(status.Success() && count == written);
+
+  auto read = process->ReadMemory(0, rbuf.data(), rbuf.size(), status);
+  ASSERT_TRUE(status.Success() && rbuf.size() == read);
+
+  ASSERT_TRUE(0 == memcmp(wbuf.data(), rbuf.data(), rbuf.size()));
+}
+
+TEST_F(WriteMemoryTest, NoBreakpointsInRange) {
+  // Without any breakpoints.
+  WriteAndCheckData(0, wbuf.size());
+
+  // Breakpoints are located beyond the area that will be re-written.
+  const size_t offset = trap_opcode.size();
+  const size_t count = wbuf.size() - 2 * offset;
+
+  const lldb::addr_t bp1_addr = 0;
+  SetBreakpoint(bp1_addr);
+  const lldb::addr_t bp2_addr = wbuf.size() - offset;
+  SetBreakpoint(bp2_addr);
+
+  // SetBreakpoint is not fake;)
+  ASSERT_EQ(2u, target->GetBreakpointList().GetSize());
+  ASSERT_EQ(2u, process->GetBreakpointSiteList().GetSize());
+
+  // Write and check memory between breakpoints.
+  for (size_t i = offset; i < count; ++i)
+    --wbuf[i];
+  WriteAndCheckData(offset, count);
+
+  // Breakpoints still here.
+  ASSERT_EQ(2u, process->GetBreakpointSiteList().GetSize());
+
+  // Breakpoint opcode before area is not damaged.
+  CheckTrapOpcode(bp1_addr);
+
+  // Breakpoint opcode after area is not damaged.
+  CheckTrapOpcode(bp2_addr);
+
+  // Saved opcodes are updated.
+  CheckSavedOpcodes();
+}
+
+TEST_F(WriteMemoryTest, BreakpointOverlapsLowerBound) {
+  WriteAndCheckData(0, wbuf.size());
+
+  const lldb::addr_t bp_addr = 0;
+  const lldb::addr_t bp_size = trap_opcode.size();
+  const lldb::addr_t offset = bp_addr + bp_size / 2;
+  SetBreakpoint(bp_addr);
+
+  // Write memory starting from middle of breakpoint.
+  for (size_t i = offset; i < bp_size + offset; ++i)
+    --wbuf[i];
+  WriteAndCheckData(offset, bp_size);
+
+  // Breakpoint still here.
+  ASSERT_EQ(1u, process->GetBreakpointSiteList().GetSize());
+
+  // Breakpoint opcode is not damaged.
+  CheckTrapOpcode(bp_addr);
+
+  // A half of saved opcode is updated. Quite absurd and dangerous situation...
+  CheckSavedOpcodes();
+}
+
+TEST_F(WriteMemoryTest, BreakpointInRange) {
+  WriteAndCheckData(0, wbuf.size());
+
+  const lldb::addr_t bp_addr = wbuf.size() / 2;
+  const lldb::addr_t bp_size = trap_opcode.size();
+  SetBreakpoint(bp_addr);
+
+  wbuf.fill(0u);
+  WriteAndCheckData(0, wbuf.size());
+
+  // Breakpoint still here.
+  ASSERT_EQ(1u, process->GetBreakpointSiteList().GetSize());
+
+  // Breakpoint opcode is not damaged.
+  CheckTrapOpcode(bp_addr);
+
+  // Saved opcode is updated.
+  CheckSavedOpcodes();
+}