Re: [PATCH 06/12] gdb: introduce status enum for displaced step prepare/finish

Simon Marchi via Gdb-patches
2020-11-25T13:21:04Z

On 2020-11-24 8:30 p.m., Pedro Alves wrote:
> On 11/10/20 9:46 PM, Simon Marchi via Gdb-patches wrote:
>> +enum displaced_step_finish_status
>> +{
>> + /* Either the instruction was stepped and fixed up, or the specified thread
>> + wasn't executing a displaced step (in which case there's nothing to
>> + finish). */
>> + DISPLACED_STEP_FINISH_STATUS_OK,
>> +
>> + /* The thread was executing a displaced step, but didn't complete it. */
>> + DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED,
>
> There's an ambiguity here that gave me pause -- the enumeration is
> called "not executed", but the comment says "was executing". I'd suggest:
>
> /* The thread started a displaced step, but didn't complete it. */

Fixed.

>> -/* If we displaced stepped an instruction successfully, adjust
>> - registers and memory to yield the same effect the instruction would
>> - have had if we had executed it at its original address, and return
>> - 1. If the instruction didn't complete, relocate the PC and return
>> - -1. If the thread wasn't displaced stepping, return 0. */
>> +/* If we displaced stepped an instruction successfully, adjust registers and
>> + memory to yield the same effect the instruction would have had if we had
>> + executed it at its original address, and return
>> + DISPLACED_STEP_PREPARE_STATUS_OK. If the instruction didn't complete,
>> + relocate the PC and return DISPLACED_STEP_PREPARE_STATUS_NOT_EXECUTED.
>
> DISPLACED_STEP_PREPARE_... -> DISPLACED_STEP_FINISH_...

Fixed.

>
>> -static int
>> + If the thread wasn't displaced stepping, return
>> + DISPLACED_STEP_PREPARE_STATUS_OK as well.. */
>
> Here too. Also, spurious double period "..".

Fixed.

Thanks,

Simon