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

[Pedro Alves <pedro@palves.net>]

On 11/10/20 9:46 PM, Simon Marchi via Gdb-patches wrote:

> --- /dev/null
> +++ b/gdb/displaced-stepping.h
> @@ -0,0 +1,46 @@
> +/* Displaced stepping related things.
> +
> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef DISPLACED_STEPPING_H
> +#define DISPLACED_STEPPING_H
> +
> +enum displaced_step_prepare_status
> +{
> +  /* A displaced stepping buffer was successfully allocated and prepared.  */
> +  DISPLACED_STEP_PREPARE_STATUS_OK,
> +
> +  /* Something bad happened.  */
> +  DISPLACED_STEP_PREPARE_STATUS_ERROR,
> +
> +  /* Not enough resources are available at this time, try again later.  */
> +  DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE,
> +};
> +
> +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.  */



> -/* 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_...

> -static int
> +   If the thread wasn't displaced stepping, return
> +   DISPLACED_STEP_PREPARE_STATUS_OK as well..  */

Here too.  Also, spurious double period "..".

> +
> +static displaced_step_finish_status
>  displaced_step_finish (thread_info *event_thread, enum gdb_signal signal)
>  {
>    struct displaced_step_inferior_state *displaced
>      = get_displaced_stepping_state (event_thread->inf);
> -  int ret;
>  
>    /* Was this event for the thread we displaced?  */
>    if (displaced->step_thread != event_thread)
> -    return 0;
> +    return DISPLACED_STEP_FINISH_STATUS_OK;
>  


Otherwise OK.