Re: [PATCHv3 0/3] AMD64 Displaced Stepping Fix

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simark@simark.ca> writes:

> On 3/27/23 08:32, Andrew Burgess via Gdb-patches wrote:
>> In V3:
>> 
>>   - Change of direction since v2.  As Pedro pointed out, using the $pc
>>     value to decide if the displaced step succeeded is not good
>>     enough.  This new version switches back to using the
>>     target_waitstatus value.
>> 
>>   - The target_waitstatus is examined, and the result (a bool) is
>>     passed to each architecture's fixup routine.
>> 
>>   - Just like in v2, each fixup routine is updated, with amd64 being
>>     "fixed", while aarch64, ppc, and s390 (which aren't broken) just
>>     have an early return case added to handle the unsuccessful
>>     displaced step case.
>> 
>>   - As with v2, ARM is left broken, though this is no more broken than
>>     it was before this patch series.
>> 
>>   - As Simon suggested, the debug has now moved to
>>     displaced_step_prepare_throw, so the (not yet upstreamed)
>>     displaced stepping for the AMD GPU target should still see the
>>     debug output,
>> 
>>   - The debug code now handles a failure to disassemble better:
>>     there's nothing worse than enabling debug to try and solve a
>>     problem, and having GDB crash in a different way.  If an
>>     instruction fails to disassemble GDB will now print a basic debug
>>     message and skip the rest of the debug output,
>> 
>>   - As suggested, I've moved the displaced_step_dump_bytes helper
>>     function into gdbsupport/ and given it a better name,
>> 
>>   - I have NOT tried to implement the improvement Simon suggested
>>     where the architecture backend tells GDB core how many bytes the
>>     replacement instruction(s) occupied.  This still means that in
>>     some cases we will disassemble the entire displaced step buffer
>>     unnecessarily, but I don't see that as a huge problem.  Fixing
>>     this just to reduce some debug output a little seems excessive.
>>     Let me know if you feel this is a blocker for this work being
>>     merged and I can take another look at it.
>> 
>> Thanks,
>> Andrew
>> 
>> ---
>> 
>> Andrew Burgess (3):
>>   gdb: more debug output for displaced stepping
>>   gdb: move displaced_step_dump_bytes into gdbsupport (and rename)
>>   gdb: fix reg corruption from displaced stepping on amd64
>
> When applying the last one:
>
>
> Applying: gdb: fix reg corruption from displaced stepping on amd64
> .git/rebase-apply/patch:591: new blank line at EOF.
> +
> .git/rebase-apply/patch:995: new blank line at EOF.
> +

Thanks, I fixed these locally.

Andrew