labath added a comment.
In https://reviews.llvm.org/D25057#560325, @omjavaid wrote:
> @labath Referring to your email on the mailing list.
>
> Thanks for helping out with this work.
>
> I think we should push this fix, as you suggested this does not fix
> everything in a holistic way but it corrects the functionality that is
> currently available right now with limitations ofcourse.
>
> So here is functionality we are currently lacking:
>
> - Ability to put multiple watchpoints with same address range. This is more
> concerning because we cannot put a watch on say byte 0 and byte 7 in case of
> aarch64.
Agreed. However, I'd rephrase this as "ability to correctly handle a single
instruction triggering multiple watchpoints. If done properly, you will get the
previous item for free.
Also, apparently lldb has a bug, where it mishandles the case where you do a
(user-level) single step over an instruction triggering a watchpoint. That
would be pretty important to fix as well.
>
>
> - Ability to use single slot for multiple watchpoints. This is more like a
> nice to have and I think if we fix the above functionality this may well be
> fixed automatically.
I am not sure I agree with this, but I'd have to see the implementation to
tell. One of the issues I have with reusing same slot for multiple watchpoints
is that it does not work at all for the "read" case, and even the "write" case
is extremely dodgy. Normally a "write" watchpoint should trigger whenever the
memory is written to, but here we are treating it more like a "modify"
watchpoint, where we stop only if the write actually modifies the memory value.
Reusing a slot for "modify" watchpoints is easy, doing it correctly for the
"write" case is quite hard.
> This is what I think LLDB client or server has to do:
>
> - Keep a cache of watchpoint registers available and modify registers in
> cache when a set/remove request is made.
Sure, why not.
> - As pre-req for set/remove is to have the target in stopped state this will
> mean that when we set/remove we make changes to actual registers before we
> resume for continue or stepping.
OK, you cannot set watchpoint set/clear packets while the target is running
anyway.
> - I dont think keeping the cache and then updating on resume actually means
> we are lying to client. Cache will also remain limited and will behave like
> an actual write to the registers. It will serve us well to support the
> functionality we intend to support.
It depends, on whether the fact that we don't write to the registers
immediately has any observable effects for the client. In your previous patch,
it did. This happened because you were only syncing the registers on a resume,
so if the client did a single-step instead, the watch would not trigger. This
is what I consider "lying", and violating the gdb-remote protocol. If the
client cannot tell the difference, you are free to do whatever you want.
> In case of GDB this functionality is handled by gdbserver and gdb client is
> not aware of the fact that watchpoint registers are not actually written
> until we resume.
This is interesting. Can you tell me what is the packet sequence in the case
where the client has watchpoints set at 0x1000 and 0x1001 and an instruction
trips both of them?
> To implement this in LLDB client or server is a design decision and I just
> see that it should be easier to achieve on LLDB server side though it may
> require changes to the way we approach watchpoint for all targets but it will
> then remain restricted to the concerning target specific area.
I don't see how you can handle the case when an instruction trips two
watchpoints in different slots with server-side changes only. If it can be
done, then it is a design decision, but until then, I believe it is a decision
between a correct and a partial solution, and I'd go with the fully correct one.
> I am OOO till 16th If you are OK with this change I will push it whenever it
> gets a LGTM.
Some small nits on the patch you should fix before committing. Otherwise, looks
good.
> TestWatchpointMultipleSlots.py:35
> + @expectedFailureAndroid(archs=['arm', 'aarch64'])
> + @expectedFailureAll(
> + oslist=["windows"],
this is unnecesary, as the test is already skipped.
> TestWatchpointMultipleSlots.py:39
> + # Read-write watchpoints not supported on SystemZ
> + @expectedFailureAll(archs=['s390x'])
> + # This is a arm and aarch64 specific test case. No other architectures
> tested.
this is unnecesary, as the test is already skipped.
> NativeRegisterContextLinux_arm.cpp:579
> + }
> + else if (m_hwp_regs[i].address == addr) {
> + return LLDB_INVALID_INDEX32; // We do not support duplicate
> watchpoints.
The formatting here is incorrect. Please run clang-format on the patch.
> NativeRegisterContextLinux_arm64.cpp:547
> + }
> + else if (m_hwp_regs[i].address == addr) {
> + return LLDB_INVALID_INDEX32; // We do not support duplicate
> watchpoints.
ditto
https://reviews.llvm.org/D25057
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
