[5/6] gdb: Remove cleanup from linux-fork.c:inferior_call_waitpid

  * Andrew Burgess <andrew.burgess@embecosm.com> [2019-01-09 13:33:23 +0000]:

> * Pedro Alves <palves@redhat.com> [2019-01-09 12:54:59 +0000]:
> 
> > On 01/01/2019 10:45 PM, Andrew Burgess wrote:
> > 
> > > +/* Temporarily switch to the infrun state stored on the fork_info
> > > +   identified by a given ptid_t.  When this object goes out of scope,
> > > +   restore the currently selected infrun state.   */
> > > +
> > > +class scoped_switch_fork_info
> > >  {
> > > -  struct fork_info *oldfp = (struct fork_info *) fp;
> > > +public:
> > > +  /* Switch to the infrun state held on the fork_info identified by
> > > +     PPTID.  If PPTID is the current inferior then no switch is done.  */
> > > +  scoped_switch_fork_info (ptid_t pptid)
> > 
> > (Nit, "explicit scoped_switch_fork_info")
> > 
> > > +    : m_oldfp (nullptr)
> > > +  {
> > > +    if (pptid != inferior_ptid)
> > > +      {
> > > +	struct fork_info *newfp = nullptr;
> > > +
> > > +	/* Switch to pptid.  */
> > > +	m_oldfp = find_fork_ptid (inferior_ptid);
> > > +	gdb_assert (m_oldfp != nullptr);
> > > +	newfp = find_fork_ptid (pptid);
> > > +	gdb_assert (newfp != nullptr);
> > > +	fork_save_infrun_state (m_oldfp, 1);
> > > +	remove_breakpoints ();
> > > +	fork_load_infrun_state (newfp);
> > > +	insert_breakpoints ();
> > > +      }
> > > +  }
> > >  
> > > -  if (oldfp)
> > > -    {
> > > -      /* Switch back to inferior_ptid.  */
> > > -      remove_breakpoints ();
> > > -      fork_load_infrun_state (oldfp);
> > > -      insert_breakpoints ();
> > > -    }
> > > -}
> > > +  /* Restore the previously selected infrun state.  If the constructor
> > > +     didn't need to switch states, then nothing is done here either.  */
> > > +  ~scoped_switch_fork_info ()
> > > +  {
> > > +    if (m_oldfp != nullptr)
> > > +      {
> > > +	/* Switch back to inferior_ptid.  */
> > > +	remove_breakpoints ();
> > > +	fork_load_infrun_state (m_oldfp);
> > > +	insert_breakpoints ();
> > > +      }
> > 
> > When writing destructors, we need to keep in mind that if an
> > exception escapes, gdb is terminated on the spot.
> > 
> > Things aren't usually correct in the cleanup version either,
> > since an exception escaping while running cleanups leaves
> > the cleanup chain with dangling cleanups, but I think that
> > these conversions are the ideal time to fix it up.
> > 
> > The solution will usually be to swallow exceptions in the
> > dtor with try/catch(...) and try to limp along.
> 
> Is it worth using `internal_warning` rather than silently dropping the
> exception?

Here's a patch using internal_warning.  If you don't think that's a
good idea then I can commit without internal_warning and place this
comment in the catch instead:

    CATCH (ex, RETURN_MASK_ERROR)
      {
        /* It's not clear how we should recover from an exception at
           this point, so for now ignore the error and push on.  */
      }

--

gdb: Improve scoped_switch_fork_info class

After committing this patch I got this feedback:

   https://sourceware.org/ml/gdb-patches/2019-01/msg00181.html

This patch makes the constructor of scoped_switch_fork_info explicit,
and wraps the core of the destructor in a TRY/CATCH block.

I've run this through the testsuite on X86-64/GNU Linux, however, this
code is not exercised, so this patch is untested.

gdb/ChangeLog:

	* linux-fork.c (scoped_switch_fork_info)
	<scoped_switch_fork_info>: Make explicit.
	<~scoped_switch_fork_info>: Wrap core in try/catch.
---
 gdb/ChangeLog    |  6 ++++++
 gdb/linux-fork.c | 18 ++++++++++++++----
 2 files changed, 20 insertions(+), 4 deletions(-)
  

On 01/09/2019 02:10 PM, Andrew Burgess wrote:
> * Andrew Burgess <andrew.burgess@embecosm.com> [2019-01-09 13:33:23 +0000]:
> 
>> * Pedro Alves <palves@redhat.com> [2019-01-09 12:54:59 +0000]:
>>
>>> On 01/01/2019 10:45 PM, Andrew Burgess wrote:

>>> When writing destructors, we need to keep in mind that if an
>>> exception escapes, gdb is terminated on the spot.
>>>
>>> Things aren't usually correct in the cleanup version either,
>>> since an exception escaping while running cleanups leaves
>>> the cleanup chain with dangling cleanups, but I think that
>>> these conversions are the ideal time to fix it up.
>>>
>>> The solution will usually be to swallow exceptions in the
>>> dtor with try/catch(...) and try to limp along.
>>
>> Is it worth using `internal_warning` rather than silently dropping the
>> exception?
> 
> Here's a patch using internal_warning.  If you don't think that's a
> good idea then I can commit without internal_warning and place this
> comment in the catch instead:

I don't think this would be an internal issue?  For example, something
external to GDB could SIGKILL the inferior process, and then calling lseek
in the inferior within fork_load_infrun_state, could throw, for example.

If we print something, it'd be pedantically better to not talk about
gdb functions.

Maybe something around:

    warning (_("Couldn't restore checkpoint state in %s: %s",
	     target_pid_to_str (fp->ptid), ex.message);

?

> 
>     CATCH (ex, RETURN_MASK_ERROR)

Use RETURN_MASK_ALL, since if a Quit/Ctrl-C escapes, RETURN_MASK_ERROR
won't catch it, and GDB dies.

Pedantically, raw C++ try/catch(...), would catch any kind of exception, even
non-GDB ones, but that would mean losing the ex object, unless you complicate
this some more.  We don't throw around non-GDB exceptions (maybe a
std::logic_error could be possible somewhere, but I'd call that a bug),
so I think
  CATCH (ex, RETURN_MASK_ALL) 
is good enough.

>       {
>         /* It's not clear how we should recover from an exception at
>            this point, so for now ignore the error and push on.  */
>       }
That's fine too.  It's what I done in other similar spots, though I confess
that I had assumed that warning() writes to gdb_stdout, and thus could
paginate and cause another exception (ctrl-c/Quit), but now that I look,
I see that warning() writes to gdb_stderr which doesn't paginate.
There's even a comment about that:

/* Print a warning message.  The first argument STRING is the warning
   message, used as an fprintf format string, the second is the
   va_list of arguments for that string.  A warning is unfiltered (not
   paginated) so that the user does not need to page through each
   screen full of warnings when there are lots of them.  */

Eh.  So yeah, a warning seems good.

Thanks,
Pedro Alves
  

* Pedro Alves <palves@redhat.com> [2019-01-09 18:44:54 +0000]:

> On 01/09/2019 02:10 PM, Andrew Burgess wrote:
> > * Andrew Burgess <andrew.burgess@embecosm.com> [2019-01-09 13:33:23 +0000]:
> > 
> >> * Pedro Alves <palves@redhat.com> [2019-01-09 12:54:59 +0000]:
> >>
> >>> On 01/01/2019 10:45 PM, Andrew Burgess wrote:
> 
> >>> When writing destructors, we need to keep in mind that if an
> >>> exception escapes, gdb is terminated on the spot.
> >>>
> >>> Things aren't usually correct in the cleanup version either,
> >>> since an exception escaping while running cleanups leaves
> >>> the cleanup chain with dangling cleanups, but I think that
> >>> these conversions are the ideal time to fix it up.
> >>>
> >>> The solution will usually be to swallow exceptions in the
> >>> dtor with try/catch(...) and try to limp along.
> >>
> >> Is it worth using `internal_warning` rather than silently dropping the
> >> exception?
> > 
> > Here's a patch using internal_warning.  If you don't think that's a
> > good idea then I can commit without internal_warning and place this
> > comment in the catch instead:
> 
> I don't think this would be an internal issue?  For example, something
> external to GDB could SIGKILL the inferior process, and then calling lseek
> in the inferior within fork_load_infrun_state, could throw, for example.
> 
> If we print something, it'd be pedantically better to not talk about
> gdb functions.
> 
> Maybe something around:
> 
>     warning (_("Couldn't restore checkpoint state in %s: %s",
> 	     target_pid_to_str (fp->ptid), ex.message);
> 
> ?
> 
> > 
> >     CATCH (ex, RETURN_MASK_ERROR)
> 
> Use RETURN_MASK_ALL, since if a Quit/Ctrl-C escapes, RETURN_MASK_ERROR
> won't catch it, and GDB dies.
> 
> Pedantically, raw C++ try/catch(...), would catch any kind of exception, even
> non-GDB ones, but that would mean losing the ex object, unless you complicate
> this some more.  We don't throw around non-GDB exceptions (maybe a
> std::logic_error could be possible somewhere, but I'd call that a bug),
> so I think
>   CATCH (ex, RETURN_MASK_ALL) 
> is good enough.
> 
> >       {
> >         /* It's not clear how we should recover from an exception at
> >            this point, so for now ignore the error and push on.  */
> >       }
> That's fine too.  It's what I done in other similar spots, though I confess
> that I had assumed that warning() writes to gdb_stdout, and thus could
> paginate and cause another exception (ctrl-c/Quit), but now that I look,
> I see that warning() writes to gdb_stderr which doesn't paginate.
> There's even a comment about that:
> 
> /* Print a warning message.  The first argument STRING is the warning
>    message, used as an fprintf format string, the second is the
>    va_list of arguments for that string.  A warning is unfiltered (not
>    paginated) so that the user does not need to page through each
>    screen full of warnings when there are lots of them.  */
> 
> Eh.  So yeah, a warning seems good.

I pushed this patch with the warning as you suggested.

General apology to the list - I fluffed the commit, and had to send a
small fix-up patch immediately afterwards.  Sorry for that.

Thanks,
Andrew