Re: [patch v8 24/24] record-btrace: add (reverse-)stepping support

[Pedro Alves <palves@redhat.com>]

On 12/17/2013 03:06 PM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: Metzger, Markus T
>> Sent: Tuesday, December 17, 2013 3:15 PM
>> To: Pedro Alves
> 
> 
>>>>>> +	  if (breakpoint_here_p (aspace, insn->pc))
>>>>>> +	    return btrace_step_stopped ();
>>>>>
>>>>> How is adjust_pc_after_break handled?
>>>>
>>>> I'm not doing anything special.  The PC register is writable,
>>>> so the PC can be adjusted.
>>>
>>> Eh, it's writable even when forward executing through history?
>>>
>>> I see this in patch 14:
>>>
>>>> +static void
>>>> +record_btrace_store_registers (struct target_ops *ops,
>>>> +			       struct regcache *regcache, int regno)
>>>> +{
>>>> +  struct target_ops *t;
>>>> +
>>>> +  if (record_btrace_is_replaying ())
>>>> +    error (_("This record target does not allow writing registers."));
>>>> +
>>>
>>> Was it changed in later patches?
>>
>> Oops.  You are right.  It is read-only.
>>
>> After some playing around, I also managed to trigger the error
>> when adjust_pc_after_break tries to write the adjusted PC.
>>
>> I'm inclined to simply allow it.  The adjusted PC will be ignored for
>> stepping, of course.  We just follow the recorded execution.
>>
>> Concerns?
> 
> Besides RIP, we also write orig_rax, so we still fail.
> 
> I'm beginning to wonder if we should actually adjust the PC for
> btrace replay.  The way I interpret the function, we got a SIGTRAP
> for executing the breakpoint instruction.  The PC is after the breakpoint
> instruction, so it is one byte after the actual breakpoint location.  We're
> now trying to undo the execution of the breakpoint instruction.
> Is this correct?
> 
> In that case, we shouldn't adjust the PC since we did not execute
> the breakpoint instruction in the trace.  We just asked whether there
> is a breakpoint at the current location.
> 
> This seems to be different for s/w record.  This is the first time, I need
> to distinguish between the different record targets.
> 
> I would add a new target method to_omit_pc_after_break_adjustment
> that would default to false if not present.  The btrace record target will
> provide it and return true if we're replaying.

My view here (and this is the same thing with most simulators or
some kind that implement breakpoints on their own, instead of having
the program execute trap instructions) is that gdb actually should
have known that it inserted a hardware breakpoint, not a
software breakpoint (and perhaps get a different TARGET_WAITKIND_FOO
for those).  But that'd be quite an overhaul.

I think better even would be to provide a to_decr_pc_after_break
target method whose default would be to return gdbarch_decr_pc_after_break.
Then, make btrace record override this target method when replaying.

-- 
Pedro Alves