tatyana-krasnukha updated this revision to Diff 135711.
tatyana-krasnukha added a comment.
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
ted added a comment.
In https://reviews.llvm.org/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.
labath added a comment.
I believe lldb-server does not work around breakpoint opcodes during writing
(it does for reading), but that can be fixed, of course.
https://reviews.llvm.org/D39967
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
tatyana-krasnukha added a comment.
Got it. Yes, you are right. At least debugserver works around breakpoints by
himself while writing memory.
I will handle this case.
https://reviews.llvm.org/D39967
___
lldb-commits mailing list
clayborg added a comment.
In https://reviews.llvm.org/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.
ted added a comment.
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.
https://reviews.llvm.org/D39967
tatyana-krasnukha added a comment.
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.
https://reviews.llvm.org/D39967
___
clayborg added a comment.
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
clayborg added a comment.
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
tatyana-krasnukha updated this revision to Diff 135674.
tatyana-krasnukha added a comment.
Change ForEach return type (bool -> void)
https://reviews.llvm.org/D39967
Files:
include/lldb/Breakpoint/BreakpointSiteList.h
source/Breakpoint/BreakpointSiteList.cpp
source/Target/Process.cpp
tatyana-krasnukha added inline comments.
Comment at: include/lldb/Breakpoint/BreakpointSiteList.h:130
+ using ModifyingCallback = std::function;
+ bool ForEach(const ModifyingCallback );
clayborg wrote:
> I was a bit vague with my
labath added inline comments.
Comment at: include/lldb/Breakpoint/BreakpointSiteList.h:159-161
+ class Guard : private std::unique_lock {
+using std::unique_lock::unique_lock;
+ };
zturner wrote:
> Err, I meant to just deleted the `Guard` class entirely
tatyana-krasnukha updated this revision to Diff 135537.
tatyana-krasnukha added a comment.
Fix naming
https://reviews.llvm.org/D39967
Files:
include/lldb/Breakpoint/BreakpointSiteList.h
source/Breakpoint/BreakpointSiteList.cpp
source/Target/Process.cpp
unittests/Process/CMakeLists.txt
tatyana-krasnukha updated this revision to Diff 135533.
tatyana-krasnukha added a comment.
Remove Guard class completely
https://reviews.llvm.org/D39967
Files:
include/lldb/Breakpoint/BreakpointSiteList.h
source/Breakpoint/BreakpointSiteList.cpp
source/Target/Process.cpp
tatyana-krasnukha added inline comments.
Comment at: source/Target/Process.cpp:2437
+ for (auto _site : enabled_bp_sites_in_range)
+update_status(DisableSoftwareBreakpoint(bp_site.get()));
xiaobai wrote:
> If disabling any breakpoint fails, the status's
zturner added inline comments.
Comment at: include/lldb/Breakpoint/BreakpointSiteList.h:159-161
+ class Guard : private std::unique_lock {
+using std::unique_lock::unique_lock;
+ };
Err, I meant to just deleted the `Guard` class entirely and return
tatyana-krasnukha added a comment.
In https://reviews.llvm.org/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;)
tatyana-krasnukha updated this revision to Diff 135527.
tatyana-krasnukha added a comment.
Update according suggestions above
https://reviews.llvm.org/D39967
Files:
include/lldb/Breakpoint/BreakpointSiteList.h
source/Breakpoint/BreakpointSiteList.cpp
source/Target/Process.cpp
xiaobai added inline comments.
Comment at: source/Breakpoint/BreakpointSiteList.cpp:191
if (lower != m_bp_site_list.begin()) {
-collection::const_iterator prev_pos = lower;
-prev_pos--;
+auto prev_pos = std::prev(lower);
const BreakpointSiteSP _bp =
zturner added a comment.
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.
xiaobai added inline comments.
Comment at: source/Breakpoint/BreakpointSiteList.cpp:191
if (lower != m_bp_site_list.begin()) {
-collection::const_iterator prev_pos = lower;
-prev_pos--;
+auto prev_pos = std::prev(lower);
const BreakpointSiteSP _bp =
zturner added inline comments.
Comment at: include/lldb/Breakpoint/BreakpointSiteList.h:159-175
+ class Guard final {
+std::recursive_mutex *m_mutex;
- typedef void (*BreakpointSiteSPMapFunc)(lldb::BreakpointSiteSP ,
- void
tatyana-krasnukha added a comment.
In https://reviews.llvm.org/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
tatyana-krasnukha updated this revision to Diff 135518.
tatyana-krasnukha added a comment.
Use Disable|EnableBreakpointSite instead of Disable|EnableSoftwareBreakpoint.
Make BreakpointSiteList::ForEach callbacks return bool.
https://reviews.llvm.org/D39967
Files:
labath added inline comments.
Comment at: include/lldb/Breakpoint/BreakpointSiteList.h:159-175
+ class Guard final {
+std::recursive_mutex *m_mutex;
- typedef void (*BreakpointSiteSPMapFunc)(lldb::BreakpointSiteSP ,
- void
clayborg added a comment.
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
tatyana-krasnukha updated this revision to Diff 135397.
tatyana-krasnukha added a comment.
diff with more context
https://reviews.llvm.org/D39967
Files:
include/lldb/Breakpoint/BreakpointSiteList.h
source/Breakpoint/BreakpointSiteList.cpp
source/Target/Process.cpp
tatyana-krasnukha updated this revision to Diff 135387.
tatyana-krasnukha added a comment.
pass callbacks by const-reference
https://reviews.llvm.org/D39967
Files:
include/lldb/Breakpoint/BreakpointSiteList.h
source/Breakpoint/BreakpointSiteList.cpp
source/Target/Process.cpp
28 matches
Mail list logo