Re: [PATCH v2 3/6] Catch and (re)throw gdb_exception_quit

[Pedro Alves <pedro@palves.net>]

On 2021-08-23 12:20 a.m., Kevin Buettner wrote:

> There is one case that I examined which I could not figure out how to
> handle.  It is the scoped_switch_fork_info destructor in
> gdb/linux-fork.c.  It looks looks like this:
> 
>   ~scoped_switch_fork_info ()
>   {
>     if (m_oldfp != nullptr)
>       {
> 	/* Switch back to inferior_ptid.  */
> 	try
> 	  {
> 	    remove_breakpoints ();
> 	    fork_load_infrun_state (m_oldfp);
> 	    insert_breakpoints ();
> 	  }
> 	catch (const gdb_exception &ex)
> 	  {
> 	    warning (_("Couldn't restore checkpoint state in %s: %s"),
> 		     target_pid_to_str (m_oldfp->ptid).c_str (),
> 		     ex.what ());
> 	  }
>       }
>   }
> 
> The static analysis tool determined that there is a call path
> to maybe_quit():
> 
>   "_ZN23scoped_switch_fork_infoD2Ev"
>     -> "_Z18remove_breakpointsv"
>     -> "_ZL17remove_breakpointP11bp_location"
>     -> "_ZL19remove_breakpoint_1P11bp_location16remove_bp_reason"
>     -> "_Z26memory_validate_breakpointP7gdbarchP14bp_target_info"
>     -> "_Z18target_read_memorymPhl"
>     -> "_Z11target_readP10target_ops13target_objectPKcPhml"
>     -> "_Z10maybe_quitv";
> 
> The catch block simply prints a warning, thus (potentially) swallowing
> a QUIT exception.  It's not clear to me whether rethrowing the
> exception is safe here.


No, it's not safe -- you'd crash gdb.  Destructors by default are noexcept, so if you let
an exception escape a destructor the C++ runtime calls std::terminate().

I'm thinking we could instead do:

~foo::foo ()
{
  try
    {
       ... destruction code here ...
    }
  catch (const gdb_exception_quit &ex)
    {
      /* Can't let this escape the dtor, but we also don't want to lose it.
         Set the global quit flag so that we handle it later once we
         get to a QUIT.  */
      set_quit_flag ();
    }
  catch (const gdb_exception &ex)
    {
       warning (....);
    }
}

we could even wrap this in a function that take gdb::function_view to avoid
duplicating the try/catch all over the place.  Like:

void
wrap_dtor (gdb::function_view<void ()> body)
{
  try
    {
       body ();
    }
  catch (const gdb_exception_quit &ex)
    {
      /* Can't handle now.  Set the global flag so that we handle it
         later once we get to a QUIT.  */
      set_quit_flag ();
    }
  catch (const gdb_exception &ex)
    {
       warning (....);
    }
}

~foo::foo ()
{
   wrap_dtor ([] ()
    {
       ... destruction code here ...
    });
}


> ---
>  gdb/ada-lang.c           |  4 ++++
>  gdb/breakpoint.c         | 15 +++++++++++++++
>  gdb/i386-linux-tdep.c    |  4 ++++
>  gdb/infrun.c             |  4 ++++
>  gdb/jit.c                |  4 ++++
>  gdb/objc-lang.c          |  4 ++++
>  gdb/parse.c              |  4 ++++
>  gdb/printcmd.c           |  4 ++++
>  gdb/record-btrace.c      |  4 ++++
>  gdb/solib.c              |  4 ++++
>  gdb/sparc64-linux-tdep.c |  4 ++++
>  11 files changed, 55 insertions(+)
> 
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index b098991612d..f2f6acf0bfe 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -11679,6 +11679,10 @@ should_stop_exception (const struct bp_location *bl)
>        stop = value_true (evaluate_expression (ada_loc->excep_cond_expr.get ()));
>        value_free_to_mark (mark);
>      }
> +  catch (const gdb_exception_quit &ex)
> +    {
> +      throw;
> +    }
>    catch (const gdb_exception &ex)

Why not just replace the existing:

    catch (const gdb_exception &ex)

with:

    catch (const gdb_exception_error &ex)

?

>      {
>        exception_fprintf (gdb_stderr, ex,
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 5cc37430e36..6271545073b 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -2699,6 +2699,9 @@ insert_bp_location (struct bp_location *bl,
>  	{
>  	  /* Can't set the breakpoint.  */
>  
> +	  if (bp_excpt.reason == RETURN_QUIT)
> +	    throw bp_excpt;
> +
>  	  /* If the target has closed then it will have deleted any
>  	     breakpoints inserted within the target inferior, as a result
>  	     any further attempts to interact with the breakpoint objects
> @@ -5082,6 +5085,10 @@ bpstat_check_watchpoint (bpstat bs)
>  	    {
>  	      e = watchpoint_check (bs);
>  	    }
> +	  catch (const gdb_exception_quit &ex)
> +	    {
> +	      throw;
> +	    }
>  	  catch (const gdb_exception &ex)

Same question here.

>  	    {
>  	      exception_fprintf (gdb_stderr, ex,
> @@ -5317,6 +5324,10 @@ bpstat_check_breakpoint_conditions (bpstat bs, thread_info *thread)
>  	    {
>  	      condition_result = breakpoint_cond_eval (cond);
>  	    }
> +	  catch (const gdb_exception_quit &ex)
> +	    {
> +	      throw;
> +	    }
>  	  catch (const gdb_exception &ex)

Ditto.

>  	    {
>  	      exception_fprintf (gdb_stderr, ex,
> @@ -14385,6 +14396,10 @@ enable_breakpoint_disp (struct breakpoint *bpt, enum bpdisp disposition,
>  	  bpt->enable_state = bp_enabled;
>  	  update_watchpoint (w, 1 /* reparse */);
>  	}
> +      catch (const gdb_exception_quit &ex)
> +	{
> +	  throw;
> +	}
>        catch (const gdb_exception &e)

Ditto.  Same for the remainder of the patch.