public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Andrew Burgess <aburgess@redhat.com>
To: John Baldwin <jhb@freebsd.org>
Cc: Simon Marchi <simark@simark.ca>, gdb-patches@sourceware.org
Subject: Re: [PATCH v2 06/13] inf-ptrace: Raise an internal_error if waitpid() fails.
Date: Mon, 6 Dec 2021 15:04:34 +0000	[thread overview]
Message-ID: <20211206150434.GC123597@redhat.com> (raw)
In-Reply-To: <7d57f548-396d-7c78-2a98-34043d5d26ce@FreeBSD.org>

* John Baldwin <jhb@FreeBSD.org> [2021-12-03 10:12:15 -0800]:

> On 12/1/21 4:04 AM, Andrew Burgess wrote:
> > * John Baldwin <jhb@FreeBSD.org> [2021-11-25 08:38:16 -0800]:
> > 
> > > On 11/25/21 2:30 AM, Andrew Burgess wrote:
> > > > * John Baldwin <jhb@FreeBSD.org> [2021-11-24 14:00:16 -0800]:
> > > > 
> > > > > On 11/24/21 12:16 PM, Simon Marchi wrote:
> > > > > > On 2021-11-24 9:16 a.m., Andrew Burgess via Gdb-patches wrote:
> > > > > > > * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:53 -0700]:
> > > > > > > 
> > > > > > > > Previously this returned a TARGET_WAITKIND_SIGNALLED event for
> > > > > > > > inferior_ptid.  Since the multi-target changes, inferior_ptid is now
> > > > > > > > invalid during ::wait() methods, so this triggered an assertion
> > > > > > > > further up the stack.
> > > > > > > > ---
> > > > > > > >     gdb/inf-ptrace.c | 11 +++--------
> > > > > > > >     1 file changed, 3 insertions(+), 8 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
> > > > > > > > index afa38de6ef7..1f8e72d1aca 100644
> > > > > > > > --- a/gdb/inf-ptrace.c
> > > > > > > > +++ b/gdb/inf-ptrace.c
> > > > > > > > @@ -319,14 +319,9 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
> > > > > > > > 
> > > > > > > >           if (pid == -1)
> > > > > > > >     	{
> > > > > > > > -	  fprintf_unfiltered (gdb_stderr,
> > > > > > > > -			      _("Child process unexpectedly missing: %s.\n"),
> > > > > > > > -			      safe_strerror (save_errno));
> > > > > > > > -
> > > > > > > > -	  /* Claim it exited with unknown signal.  */
> > > > > > > > -	  ourstatus->kind = TARGET_WAITKIND_SIGNALLED;
> > > > > > > > -	  ourstatus->value.sig = GDB_SIGNAL_UNKNOWN;
> > > > > > > > -	  return inferior_ptid;
> > > > > > > > +	  internal_error (__FILE__, __LINE__,
> > > > > > > > +			  _("Child process unexpectedly missing: %s.\n"),
> > > > > > > > +			  safe_strerror (save_errno));
> > > > > > > >     	}
> > > > > > > 
> > > > > > > Single statement if blocks should not have '{ ... }' around them.
> > > > > > > 
> > > > > > > I wonder if we could use perror_with_name here instead of
> > > > > > > internal_error, there's at least one place this is done in
> > > > > > > linux-nat.c.
> > > > > > 
> > > > > > If we are being pedantic about the uses of internal_error vs throwing an
> > > > > > error: factors external to GDB should not be able to cause an internal
> > > > > > error.  The kernel is an external factor: a bug in the kernel could
> > > > > > cause this branch to be taken, so it should probably not make GDB
> > > > > > internal error.  In that regard, perror_with_name would be better.
> > > > > > Although I would be fine with assuming that the kernel does not have
> > > > > > bugs (except when actively working around known bugs).
> > > > > > 
> > > > > > Otherwise, I would suggest using gdb_assert_not_reached instead of
> > > > > > internal_error, to achieve the same thing.
> > > > > 
> > > > > I think treating this as a kernel bug (and thus perror_with_name) is fine.
> > > > 
> > > > I don't think that perror_with_name implies this is a kernel bug.  The
> > > > perror function is just a wrapper around 'error' and safe_strerror -
> > > > so basically what you have already except using error, not
> > > > internal_error.
> > > > 
> > > > Though I guess if we get -1 back then this implies something has gone
> > > > wrong somewhere, right?  We expect to see some event for the child no
> > > > matter how the kernel kills it, so, if we get -1, that implies the
> > > > child disappeared without a trace, right?  But possibly the bug could
> > > > be that we already collected the event, and lost it somehow, so it
> > > > could be a GDB bug...
> > > 
> > > Yes, it could be either one.  I had chosen internal_error to replace the
> > > existing fprintf somewhat arbitrarily.  I don't have a strong opinion
> > > on what type of error it should be, only that it is best to throw the
> > > error closer to the real problem than to get a later assertion
> > > failure.
> > 
> > For this sort of situation we should assume GDB doesn't have bugs, and
> > that this situation must be caused by an external problem, so
> > `error`.  If we later find there are bugs in GDB, we'd then fix them.
> 
> To be clear, is perror_with_name what you'd prefer here or explicitly
> using error()?

perror_with_name is my preference.

Sorry for not being clear.

Thanks,
Andrew


  reply	other threads:[~2021-12-06 15:04 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-03 18:49 [PATCH v2 00/13] FreeBSD target async mode and related refactoring John Baldwin
2021-08-03 18:49 ` [PATCH v2 01/13] gdbsupport: Add an event-pipe class John Baldwin
2021-10-11 21:39   ` Lancelot SIX
2021-11-24 13:04   ` Andrew Burgess
2021-08-03 18:49 ` [PATCH v2 02/13] gdb linux-nat: Convert linux_nat_event_pipe to the event_pipe class John Baldwin
2021-11-24 13:07   ` Andrew Burgess
2021-08-03 18:49 ` [PATCH v2 03/13] gdbserver linux-low: Convert linux_event_pipe " John Baldwin
2021-11-24 13:10   ` Andrew Burgess
2021-08-03 18:49 ` [PATCH v2 04/13] linux-nat: Don't enable async mode at the end of linux_nat_target::resume John Baldwin
2021-11-24 13:24   ` Andrew Burgess
2021-11-24 16:03     ` John Baldwin
2021-12-01 12:02       ` Andrew Burgess
2021-12-01 20:03         ` John Baldwin
2021-12-02 11:19           ` Andrew Burgess
2021-12-03  1:21             ` John Baldwin
2021-12-03 10:43               ` Andrew Burgess
2021-12-07 18:47               ` Tom Tromey
2021-08-03 18:49 ` [PATCH v2 05/13] do_target_wait_1: Clear TARGET_WNOHANG if the target isn't async John Baldwin
2021-11-24 13:26   ` Andrew Burgess
2021-08-03 18:49 ` [PATCH v2 06/13] inf-ptrace: Raise an internal_error if waitpid() fails John Baldwin
2021-11-24 14:16   ` Andrew Burgess
2021-11-24 20:16     ` Simon Marchi
2021-11-24 22:00       ` John Baldwin
2021-11-25 10:30         ` Andrew Burgess
2021-11-25 16:38           ` John Baldwin
2021-12-01 12:04             ` Andrew Burgess
2021-12-03 18:12               ` John Baldwin
2021-12-06 15:04                 ` Andrew Burgess [this message]
2021-08-03 18:49 ` [PATCH v2 07/13] inf-ptrace: Support async targets in inf_ptrace_target::wait John Baldwin
2021-11-24 14:31   ` Andrew Burgess
2021-11-24 16:05     ` John Baldwin
2021-08-03 18:49 ` [PATCH v2 08/13] fbsd-nat: Implement async target support John Baldwin
2021-11-24 15:00   ` Andrew Burgess
2021-11-24 22:17     ` John Baldwin
2021-08-03 18:49 ` [PATCH v2 09/13] fbsd-nat: Include ptrace operation in error messages John Baldwin
2021-11-24 15:02   ` Andrew Burgess
2021-08-03 18:49 ` [PATCH v2 10/13] fbsd-nat: Switch fbsd_nat_target::resume entry debug message from lwp to nat John Baldwin
2021-11-24 15:05   ` Andrew Burgess
2021-11-24 22:19     ` John Baldwin
2021-08-03 18:49 ` [PATCH v2 11/13] fbsd-nat: Return NULL rather than failing thread_alive John Baldwin
2021-11-24 15:12   ` Andrew Burgess
2021-11-24 21:29     ` John Baldwin
2021-12-01 12:15       ` Andrew Burgess
2021-08-03 18:49 ` [PATCH v2 12/13] Enable async mode in the target in attach_cmd John Baldwin
2021-11-24 15:16   ` Andrew Burgess
2021-11-24 22:42     ` John Baldwin
2021-12-01 12:21       ` Andrew Burgess
2021-08-03 18:50 ` [PATCH v2 13/13] inf-ptrace: Add an event_pipe to be used for async mode in subclasses John Baldwin
2021-11-24 15:30   ` Andrew Burgess
2021-11-25  0:14     ` John Baldwin
2021-11-25  0:31       ` John Baldwin
2021-10-06 15:43 ` [PING] [PATCH v2 00/13] FreeBSD target async mode and related refactoring John Baldwin

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=20211206150434.GC123597@redhat.com \
    --to=aburgess@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=jhb@freebsd.org \
    --cc=simark@simark.ca \
    /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).