[PATCH] Fix displaced stepping watchpoint check order

[Luis Machado]

On 8/18/21 10:31 PM, Simon Marchi wrote:
>> No side effects of the instruction are committed in this case. Memory and registers will have their old values as if the instruction didn't execute.
>>
>>  From reading the code, most architectures have non-steppable hardware watchpoints. GDB just disables the hardware watchpoints, single-steps past that instruction and then enables the hardware watchpoints again.
>>
>> GDB just disables all hardware watchpoints for the sake of simplicity. You can see this logic in infrun.c:handle_signal_stop, around this comment:
>>
>> /* If necessary, step over this watchpoint.  We'll be back to display
>>     it in a moment.  */
>> if (stopped_by_watchpoint
>>      && (target_have_steppable_watchpoint ()
>>          || gdbarch_have_nonsteppable_watchpoint (gdbarch)))
>>
>> It is actually best to disable all hardware watchpoints. If we end up disabling just one hardware watchpoint, and then we happen to have another hardware watchpoint that is active and also getting triggered, we might be stuck in an endless loop as well.
>>
> 
> Ok, thanks for the explanation.  Watchpoints are usually (always?)
> defined in some per-thread register, so unlike software breakpoints, I
> suppose it's safe to remove the watchpoints just for the thread we step.
> I suppose that is what we are doing?

Although hardware watchpoints are usually per-thread, I don't think GDB 
handles it in that level of detail. It seems to mostly ignore insertion 
of hardware watchpoints if we're trying to step over a PC that caused a 
hardware watchpoint trigger.

See, for example, breakpoint.c:should_be_inserted

   /* Don't insert watchpoints if we're trying to step past the
      instruction that triggered one.  */
   if ((bl->loc_type == bp_loc_hardware_watchpoint)
       && stepping_past_nonsteppable_watchpoint ())
     {
       infrun_debug_printf ("stepping past non-steppable watchpoint. "
                            "skipping watchpoint at %s:%d\n",
                            paddress (bl->gdbarch, bl->address), 
bl->length);
       return 0;
     }

 From what I recall, GDB replicates the hardware watchpoints to all 
threads, so disabling all hardware watchpoints for all threads (actually 
not inserting them) makes sense. GDB doesn't try to add thread-specific 
hardware watchpoints at the moment.

Having per-thread hardware watchpoints might be desirable in some cases, 
but it may complicate the user interaction a bit.

I haven't investigated if GDB, during a step-over operation, keeps 
threads running with hardware watchpoints disabled. If it doesn't handle 
it correctly, we may miss hardware watchpoints hits, unless GDB 
serializes the step-overs. But I'm guessing there is some serialization, 
mostly due to this comment:

"Presently GDB can only step over one breakpoint at any given time."

> 
>>>>> Can you clarify what you mean by "from now on"?  Can you indicate what
>>>>> change you are referring to?
>>>>>
>>>>
>>>>   From the following change (https://sourceware.org/pipermail/gdb-patches/2021-July/181095.html) onwards, we need to look at the load/store instruction to figure out the memory access size so we can reliably tell if a hardware watchpoint has triggered. This is due to how AArch64's spec defines how to provide a stopped data address, and the valid ranges.
>>>
>>> Ok, but that patch you linked isn't merged yet?  So it sounds strange to
>>> say "from now on", it sounds like there's a dependency between the two
>>
>> Absolutely. There is a dependency. My plan is to merge this fix first, and then merge the AArch64 hardware watchpoint detection fixes. I just didn't group those together, but that's the right order. I'll make sure to point out the dependency in the other patch.
> 
> Ok.
> 
>>> patches.  Let's say the current patch is merged before the other one,
>>> maybe it should say "but AArch64 will need to do it it an upcoming
>>> patch", and then you can given the link.
>>>
>>>> With the old code, if we try to fetch the instruction at PC, we will get a bogus value that is not the real instruction that caused the hardware watchpoint trigger. Hence why the patch moves the call to displaced_step_instruction_executed_successfully (...) up and before we restore the displaced stepping buffer.
>>>>
>>>> If a hardware watchpoint trigger takes place and GDB doesn't recognize it, then displaced_step_instruction_executed_successfully (...) will return true and GDB will move on and will attempt to execute the same instruction again, only to be halted due to the same hardware watchpoint trigger that it can't detect. So GDB gets into an infinite loop.
>>>>
>>>> More generally, if we ever fail to acknowledge a hardware watchpoint trigger on an architecture with non-steppable watchpoints and displaced stepping support, we will run into this infinite loop (as far as I can tell).
>>>>
>>>> Does that make sense?
>>>
>>> Yes, this help.  Please feel free to include in the commit message any
>>> additional detail that you gave here, since it might help somebody else
>>> in the future.
>>
>> I'll make the commit message more detailed.
> 
> Thanks,
> 
> Simon
>