Re: [PATCH] gdb: invalidate register cache after monitor commands

[Andrew Burgess <aburgess@redhat.com>]

Jerry Zhang Jian <jerry.zhangjian@sifive.com> writes:

> A monitor command is opaque to GDB: the stub can halt, resume, or
> reset the target behind GDB's back, even if it later reports an
> error, and the remote protocol has no way to tell GDB that happened.

I don't find any of these example particularly clear.  They all kind of
hint towards a problem, but it would be nice to have at least one fully
explained case.

Take "halt".  Do you mean the target is running in async mode, but GDB's
"interrupt" command doesn't do what you need?  Or does "halt" mean
something different in this context?

Or "resume".  Why would GDB's normal resumption commands not be
sufficient?  If you resume the inferior via a monitor command that's
going to leave GDB thinking the inferior is stopped when it's actually
running, that's going to break you debug session, right?

The "reset" example is by far the most obvious.  The target is stopped
an you want to restore it to some initial state.  It's still stopped,
but the register state has changed.

> For example, "monitor reset halt" was leaving GDB reporting the
> pre-reset $pc until a later step/continue forced a refetch.

Do you mean "monitor reset halt" here?  Even if you do due to some
detail of your specific setup, is this really needed in the commit
message?  Wouldn't it be clearer just to pretend that the command was
"monitor reset" as it feels (to me) like it is more obvious what this
means.

>
> Invalidate the register cache after every monitor command via
> SCOPE_EXIT, so it still runs on the error path.  Scope it to the
> inferior's own process_stratum_target, matching registers_changed_thread()
> and the target_wait()/target_stop() lookup pattern elsewhere in this
> file, rather than wiping every inferior's cache with registers_changed().
> Hold a strong reference to the target across the call in case it gets
> unpushed/detached, the same idiom used in target_detach().

That all makes sense.

>
> Signed-off-by: Jerry Zhang Jian <jerry.zhangjian@sifive.com>

Please remove the 'Signed-off-by' tag, these are not used by GDB right
now, but might be in the future.

> ---
>  gdb/target.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/gdb/target.c b/gdb/target.c
> index 5d937f3ae85..5c4684d81b5 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -4262,6 +4262,18 @@ default_rcmd (struct target_ops *self, const char *command,
>  static void
>  do_monitor_command (const char *cmd, int from_tty)
>  {
> +  process_target_ops_ref proc_target_ref;
> +  if (process_stratum_target *proc_target
> +	= current_inferior ()->process_target ())

This treats a pointer as a bool.  It would be clearer to just split the
assignment out from the `if` and just have the `if` condition be
`proc_target != nullptr`.

Thanks,
Andrew

> +    proc_target_ref = process_target_ops_ref::new_reference (proc_target);
> +
> +  /* Monitor commands may change target state behind GDB's back.  */
> +  SCOPE_EXIT
> +    {
> +      if (proc_target_ref != nullptr)
> +	registers_changed_ptid (proc_target_ref.get (), minus_one_ptid);
> +    };
> +
>    target_rcmd (cmd, gdb_stdtarg);
>  }
>  
> -- 
> 2.53.0