public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Pedro Alves <pedro@palves.net>
To: Andrew Burgess <aburgess@redhat.com>,
	Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>
Cc: Simon Marchi <simon.marchi@efficios.com>
Subject: Re: [PATCH] gdb: error out if architecture does not implement any "return_value" hook
Date: Tue, 28 Feb 2023 20:20:00 +0000	[thread overview]
Message-ID: <f15a9016-d035-d558-9b69-bd3569d9a78e@palves.net> (raw)
In-Reply-To: <87fsapj13v.fsf@redhat.com>

On 2023-02-28 2:50 p.m., Andrew Burgess via Gdb-patches wrote:

> Or maybe we shouldn't be throwing an error in some of these cases, maybe
> in some cases we could warn and continue, just without the return value
> fetching?

+1

It seems a bit too harsh for "finish" to error out, when it could just
not print the return value.  get_return_value already returns NULL in
a couple scenarios where we can't get at the return value, for instance.

For the "return" command, the patch is erroring out _after_ poping the frame.
There are legitimate cases where GDB won't be able to retrieve the return
value, like in the TYPE_NO_RETURN+query case handled in the function, which is
checked _before_ frame_pop.  I'd think that if we are to error out when the arch
can't write the return value, we should do it before frame_pop too.  Or we should
warn that we can't write the return value, and proceed anyhow.

For ifuncs, calling the resolver and then erroring out is probably messing
up run control.  It would probably be better to not try to resolve the ifunc
at all.

For infcalls, I don't see a reason to error out if the user did "call func()"
instead of "print func()".

> 
> As I said above, I don't actually like GDB trying to handling this case
> at all, I'd much rather we just force the port to stub these functions
> during bring up.

There are legitimate cases when GDB isn't able to extract the return value.
Say, the function does not follow normal ABI.  So we should already have
code in place that gracefully handles not being able to get at the return value.
If we code it right, then I guess in most cases an extra check for whether the
arch implements the return value hook wouldn't end up complicating the code,
it would just "fit right in".  I'm not sure it's worth the bother, though.
I kind of tend to agree with you.

  parent reply	other threads:[~2023-02-28 20:19 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-27 21:28 Simon Marchi
2023-02-28 14:50 ` Andrew Burgess
2023-02-28 16:53   ` Andrew Burgess
2023-02-28 19:53   ` Simon Marchi
2023-02-28 20:20   ` Pedro Alves [this message]
2023-03-01  3:14     ` Simon Marchi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f15a9016-d035-d558-9b69-bd3569d9a78e@palves.net \
    --to=pedro@palves.net \
    --cc=aburgess@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=simon.marchi@efficios.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).