Page MenuHomePhabricator

Disable breakpoints before writing memory and re-enable after.
Needs ReviewPublic

Authored by tatyana-krasnukha on Nov 13 2017, 10:29 AM.

Details

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.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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 edited edge metadata.Nov 14 2017, 8:38 AM

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 requested changes to this revision.Nov 14 2017, 8:41 AM

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

This revision now requires changes to proceed.Nov 14 2017, 8:41 AM
tatyana-krasnukha updated this revision to Diff 122966.EditedNov 14 2017, 10:43 PM
tatyana-krasnukha edited edge metadata.

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

Same for comment in the header file.

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...

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()).

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

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).

tatyana-krasnukha removed rL LLVM as the repository for this revision.Jan 22 2018, 10:58 AM
tatyana-krasnukha removed a subscriber: llvm-commits.
labath added a subscriber: labath.Jan 23 2018, 1:43 AM

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.

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 added a comment.EditedJan 23 2018, 7:09 AM

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...

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?

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.

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.

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

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?)

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 retitled this revision from Refactoring of MemoryWrite function to Disable breakpoints before writing memory and re-enable after..Feb 22 2018, 12:54 AM

pass callbacks by const-reference

diff with more context

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 added inline comments.Feb 22 2018, 10:44 AM
include/lldb/Breakpoint/BreakpointSiteList.h
159–175

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

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...

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

zturner added inline comments.
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.

include/lldb/Breakpoint/BreakpointSiteList.h
159–175

Unfortunately, it is not movable

xiaobai added inline comments.
source/Breakpoint/BreakpointSiteList.cpp
193

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?

2435

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

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.

source/Breakpoint/BreakpointSiteList.cpp
193

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

xiaobai added inline comments.Feb 22 2018, 2:54 PM
source/Breakpoint/BreakpointSiteList.cpp
193

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

Update according suggestions above

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 added inline comments.Feb 22 2018, 3:31 PM
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>>

source/Target/Process.cpp
2435

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.

Remove Guard class completely

labath added inline comments.Feb 22 2018, 4:29 PM
include/lldb/Breakpoint/BreakpointSiteList.h
159–161

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

Remove Optional from return type of FindInRange

clayborg added inline comments.Feb 23 2018, 7:27 AM
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.

include/lldb/Breakpoint/BreakpointSiteList.h
130

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

Change ForEach return type (bool -> void)

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.

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".

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 added a subscriber: ted.Feb 23 2018, 1:16 PM

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.

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.

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

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

ted added a comment.Feb 23 2018, 1:39 PM

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.

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.