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

Pedro Alves
2020-11-25T01:30:38Z

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.