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

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

* 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(-)

diff --git a/gdb/linux-fork.c b/gdb/linux-fork.c
index f3231bae048..a71a429678c 100644
--- a/gdb/linux-fork.c
+++ b/gdb/linux-fork.c
@@ -446,7 +446,7 @@ class scoped_switch_fork_info
 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)
+  explicit scoped_switch_fork_info (ptid_t pptid)
     : m_oldfp (nullptr)
   {
     if (pptid != inferior_ptid)
@@ -472,9 +472,19 @@ public:
     if (m_oldfp != nullptr)
       {
 	/* Switch back to inferior_ptid.  */
-	remove_breakpoints ();
-	fork_load_infrun_state (m_oldfp);
-	insert_breakpoints ();
+	TRY
+	  {
+	    remove_breakpoints ();
+	    fork_load_infrun_state (m_oldfp);
+	    insert_breakpoints ();
+	  }
+	CATCH (ex, RETURN_MASK_ERROR)
+	  {
+	    internal_warning (__FILE__, __LINE__,
+			      "error during scoped_switch_fork_info cleanup: %s",
+			      ex.message);
+	  }
+	END_CATCH
       }
   }
 
-- 
2.14.5