tatyana-krasnukha updated this revision to Diff 135370.
tatyana-krasnukha added a comment.
Herald added a subscriber: mgorny.
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
d921c2f326fd...@reviews.llvm.org; Tatyana
> Krasnukha via Phabricator <revi...@reviews.llvm.org>
> Cc: tatyana.krasnu...@synopsys.com; clayb...@gmail.com; lldb-
> comm...@lists.llvm.org
> Subject: Re: [Lldb-commits] [PATCH] D39967: Refactoring of MemoryWrite
> function
>
> It seems
It seems to me better to remove breakpoint sites under any memory that you are
going to overwrite. The old breakpoints aren't guaranteed to make any sense
and, for instance, on x86 could do harm (they could end up in the middle of an
instruction in the new TEXT.)
If you want to do this
tatyana-krasnukha added a comment.
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
clayborg added a comment.
In https://reviews.llvm.org/D39967#985181, @labath wrote:
> In https://reviews.llvm.org/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
clayborg added a comment.
In https://reviews.llvm.org/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
tatyana-krasnukha added a comment.
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?
https://reviews.llvm.org/D39967
___
clayborg added a comment.
In https://reviews.llvm.org/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
tatyana-krasnukha added a comment.
> 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
labath added a comment.
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
clayborg added a comment.
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
tatyana-krasnukha added a comment.
Herald added a subscriber: llvm-commits.
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
tatyana-krasnukha added a comment.
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...
Repository:
rL
tatyana-krasnukha updated this revision to Diff 122972.
tatyana-krasnukha added a comment.
Same for comment in the header file.
Repository:
rL LLVM
https://reviews.llvm.org/D39967
Files:
include/lldb/Target/Process.h
source/Target/Process.cpp
Index: source/Target/Process.cpp
tatyana-krasnukha updated this revision to Diff 122966.
tatyana-krasnukha added a comment.
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;
- change other comment
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Please clang format and check in without any changes to the functionality and
then make a patch that only has your functional changes.
Repository:
rL LLVM
clayborg added a comment.
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^ |
tatyana-krasnukha added inline comments.
Comment at: source/Target/Process.cpp:2462
- // 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.
tatyana-krasnukha created this revision.
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.
Repository:
rL LLVM
https://reviews.llvm.org/D39967
Files:
19 matches
Mail list logo
