[Lldb-commits] [PATCH] D25057: Fix ARM/AArch64 Step-Over watchpoint issue remove provision for duplicate watchpoints

[Jim Ingham via lldb-commits]

Yeah, sorry, that was an oversight on my part, but I think it's trivial to fix.  The WatchpointSentry was being set up before the little single step over the watchpoint dance, and it was accidentally being used to get the watchpoint disabled.  That's not the main point of the sentry, which is to ensure that the right thing happens whatever the actions do.  OTOH, the single-step dance doesn't need such a fancy mechanism, it can just by-hand disable/single-step/enable, then set up the WatchpointSentry to handle the more complex task of running the actions.

I'm testing this out now and see how it goes.

Jim


> On Oct 21, 2016, at 5:02 AM, Pavel Labath <labath at google.com> wrote:
> 
> Ok, after the latest round of changes, I was not able to reproduce the
> behaviour where we would continually be-continually re-hitting a
> watchpoint. However, I did manage to get the lldb-server to hang if I
> set two watchpoints close to each other and trigger them both with a
> single instruction. I added it as TestMultipleHits.py. However, this
> seems like a lldb-server problem, so it's probably not going to be
> interesting to you. But maybe Omair would take a look (?)
> 
> To get watchpoints working at all, I had to revert your latest patch
> -- it was causing watchpoints to be re-enabled before we actually
> stepped-over them, causing a different kind of a loop. Let me know if
> you have questions about that.
> 
> cheers,
> pl
> 
> On 20 October 2016 at 19:43, Jim Ingham <jingham at apple.com> wrote:
>> Ah, I see, thanks.  If you can make a test case that fails on ARM in the current state of things, I would be interested in taking a look at it.  We had to do an analogous little dance for stepping from one breakpoint to another.  Probably not the same solution, but we can surely get this to work.
>> 
>> Jim
>> 
>> 
>>> On Oct 20, 2016, at 3:12 AM, Pavel Labath <labath at google.com> wrote:
>>> 
>>> The reason you could not see this is that the issue is specific to arm
>>> (or any target that reports watchpoint hits *before* executing the
>>> instruction tripping the watch). In this case, we have special logic
>>> in the client which removes the watchpoint, does a single step, and
>>> reinstates the watchpoint. This code does not correctly handle the
>>> case when you hit *another* watchpoint while "stepping over" the first
>>> one.
>>> 
>>> I was not able to reproduce the exact original problem now (I was
>>> doing it with one of Omair's previous changes applied, so it may not
>>> be possible to reproduce on trunk), but here is another way this
>>> problem manifests itself:
>>> 
>>> 
>>> Process 6009 stopped
>>> * thread #1: tid = 6009, 0xaac67324 a.out`main + 16 at a.c:2, name =
>>> 'a.out', stop reason = breakpoint 1.1
>>>   frame #0: 0xaac67324 a.out`main + 16 at a.c:2
>>>  1   int x;
>>> -> 2   int main () { x = 47; return 0; }
>>> (lldb) fr var &x
>>> (int *) &x = 0xaac69004
>>> (lldb) watchpoint set expression -s 1 -- 0xaac69004
>>> Watchpoint created: Watchpoint 6: addr = 0xaac69004 size = 1 state =
>>> enabled type = w
>>>   new value: 0
>>> (lldb) watchpoint set expression -s 1 -- 0xaac69005
>>> Watchpoint created: Watchpoint 7: addr = 0xaac69005 size = 1 state =
>>> enabled type = w
>>>   new value: 0
>>> (lldb) c
>>> Process 6009 resuming
>>> 
>>> At this point, it appears as if the inferior keeps running and never
>>> hits the watchpoint, but if you check the packet log you will see that
>>> LLDB is continuously busy sending vCont packets trying to step over
>>> the watchpoint hit.
>>> Note that after Omair's change it will not be possible anymore to
>>> reproduce the problem this easily, as he forbids the creation of
>>> watchpoints within the same block, but you can reproduce still
>>> reproduce it by having a signle instruction that writes to two blocks
>>> (stp is a good candidate for that).
>>> 
>>> If you are more than curious and want to dig into that I can make a
>>> better repro case for it.
>>> 
>>> pl
>>> 
>>> 
>>> 
>>> On 19 October 2016 at 18:34, Jim Ingham <jingham at apple.com> wrote:
>>>> 
>>>>> On Oct 4, 2016, at 9:28 AM, Pavel Labath via lldb-commits <lldb-commits at lists.llvm.org> wrote:
>>>>> 
>>>>> 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.
>>>> 
>>>> Do you have a reference for this?  In a trivial case, it works correctly on MacOS so far as I can tell:
>>>> 
>>>> (lldb) si
>>>> Process 99358 stopped
>>>> * thread #1: tid = 0x148d67e, function: main , stop reason = instruction step into
>>>>   frame #0: 0x0000000100000f5e changeit`main at changeit.c:9
>>>>  6
>>>>  7      printf("my_var is: %d.\n", my_var);
>>>>  8
>>>> -> 9      my_var = 20;
>>>>  10
>>>>  11     printf("my_var is: %d.\n", my_var);
>>>>  12
>>>> changeit`main:
>>>> ->  0x100000f5e <+46>: movl   $0x14, -0x8(%rbp)
>>>>   0x100000f65 <+53>: movl   -0x8(%rbp), %esi
>>>>   0x100000f68 <+56>: movl   %eax, -0xc(%rbp)
>>>>   0x100000f6b <+59>: movb   $0x0, %al
>>>> (lldb) si
>>>> 
>>>> Watchpoint 1 hit:
>>>> old value: 10
>>>> new value: 20
>>>> Process 99358 stopped
>>>> * thread #1: tid = 0x148d67e, function: main , stop reason = watchpoint 1
>>>>   frame #0: 0x0000000100000f65 changeit`main at changeit.c:11
>>>>  8
>>>>  9      my_var = 20;
>>>>  10
>>>> -> 11     printf("my_var is: %d.\n", my_var);
>>>>  12
>>>>  13     return 0;
>>>>  14   }
>>>> changeit`main:
>>>> ->  0x100000f65 <+53>: movl   -0x8(%rbp), %esi
>>>>   0x100000f68 <+56>: movl   %eax, -0xc(%rbp)
>>>>   0x100000f6b <+59>: movb   $0x0, %al
>>>>   0x100000f6d <+61>: callq  0x100000f80               ; symbol stub for: printf
>>>> 
>>>> Jim
>>>> 
>>