[patch] try 2 - Do not allow unwinder to terminate inferior in an inferior function call if a C++ exception handler cannot be found.

[patch] try 2 - Do not allow unwinder to terminate inferior in an inferior function call if a C++ exception handler cannot be found.

[Phil Muldoon]

[-- Attachment #1: Type: text/plain, Size: 2196 bytes --]


All,
> This patch addresses the bug detailed in gnats pr 2495 and after the 
> conversion to bugzilla: pr 9600
>
> Due to the method that GDB uses to construct a frame for an inferior 
> function call, the unwinder can terminate the inferior if it raises an 
> exception and relies on an out-of-frame handler to normally handle 
> that exception. For most systems GCC uses the exception handling 
> strategy documented here:  http://www.codesourcery.com/public/cxx-abi/ 
> . If the function that raises the exception relies on an out-of-frame 
> handler, the single dummy frame constructed by GDB for the inferior 
> function call does not allow the unwinder to walk the stack looking 
> for an appropriate handler. Briefly, if this happens the unwinder 
> terminates the inferior via a call to the default exception handler on 
> the (wrong) assumption that there is no user-created handler for the 
> exception.
>
> This patch addresses that by gating access to std::terminate in an 
> inferior function call. If it enters that function, it is popped back 
> to the calling state and "rescues" the inferior. This patch also adds 
> the functionality to turn this gating behaviour on or off. The default 
> is on.
>
> This patch applies cleanly to CVS GDB head. Tested on x8664 with no 
> regressions.

Further to Eli's and Joel's requested changes I am submitting this patch 
again for review. What do you think?

Regards

Phil

gdb/ChangeLog

2009-06-09  Phil Muldoon  <pmuldoon@redhat.com>

        * infcall.c (show_unwind_on_terminating_exception_p): New
        function.
        (call_function_by_hand): Create breakpoint and clean-up call for
        std::terminate.breakpoint. Add unwind_on_terminating_exception_p
        gate. Pop frame on breakpoint hit.
        (_initialize_infcall): Add add_setshow_boolean_cmd for
        unwind-on-terminating-exception.

gdb/testsuite/ChangeLog

2009-06-09  Phil Muldoon  <pmuldoon@redhat.com>

        * gdb.cp/gdb2495.cc: New file.
        * gdb.cp/gdb2495.exp: New file.

gdb/doc/ChangeLog

2009-06-09  Phil Muldoon  <pmuldoon@redhat.com>

        * doc/gdb.texinfo (Calling): Document
        set-unwind-on-terminating-exception usage.




[-- Attachment #2: unwind-terminating-excp.patch --]
[-- Type: text/x-patch, Size: 16499 bytes --]

Index: infcall.c
===================================================================
RCS file: /cvs/src/src/gdb/infcall.c,v
retrieving revision 1.114
diff -u -r1.114 infcall.c
--- infcall.c	28 May 2009 00:53:51 -0000	1.114
+++ infcall.c	9 Jun 2009 10:26:51 -0000
@@ -98,6 +98,30 @@
 		    value);
 }
 
+/* This boolean tells what gdb should do if a std::terminate call is
+   made while in a function called from gdb (call dummy).
+   As the confines of a single dummy stack prohibit out-of-frame
+   handlers from handling a raised exception, and as out-of-frame
+   handlers are common in C++, this can lead to no handler being found
+   by the unwinder, and a std::terminate call.  This is a false positive.
+   If set, gdb unwinds the stack and restores the context to what it
+   was before the call.
+
+   The default is to unwind the frame if a std::terminate call is
+   made.  */
+
+static int unwind_on_terminating_exception_p = 1;
+
+static void
+show_unwind_on_terminating_exception_p (struct ui_file *file, int from_tty,
+					struct cmd_list_element *c,
+					const char *value)
+
+{
+  fprintf_filtered (file, _("\
+Unwind stack if a C++ exception is unhandled while in a call dummy is %s.\n"),
+		    value);
+}
 
 /* Perform the standard coercions that are specified
    for arguments to be passed to C or Ada functions.
@@ -416,6 +440,8 @@
   struct cleanup *args_cleanup;
   struct frame_info *frame;
   struct gdbarch *gdbarch;
+  struct breakpoint *terminate_bp = NULL;
+  struct minimal_symbol *tm;
   ptid_t call_thread_ptid;
   struct gdb_exception e;
   const char *name;
@@ -716,6 +742,29 @@
     bpt->disposition = disp_del;
   }
 
+  /* Create a breakpoint in std::terminate.
+     If a C++ exception is raised in the dummy-frame, and the
+     exception handler is (normally, and expected to be) out-of-frame,
+     the default C++ handler will (wrongly) be called in an inferior
+     function call.  This is wrong, as an exception can be  normally
+     and legally handled out-of-frame.  The confines of the dummy frame
+     prevent the unwinder from finding the correct handler (or any
+     handler, unless it is in-frame).  The default handler calls
+     std::terminate.  This will kill the inferior. Assert that
+     terminate should never be called in an inferior function
+     call.  Place a momentary breakpoint in the std::terminate function
+     and if triggered in the call, rewind.  */
+  if (unwind_on_terminating_exception_p)
+     {
+       struct minimal_symbol *tm = lookup_minimal_symbol  ("std::terminate()",
+							   NULL, NULL);
+       if (tm != NULL)
+	 {
+	   terminate_bp = set_momentary_breakpoint_at_pc
+	     (SYMBOL_VALUE_ADDRESS (tm),  bp_breakpoint);
+	}
+     }
+
   /* Everything's ready, push all the info needed to restore the
      caller (and identify the dummy-frame) onto the dummy-frame
      stack.  */
@@ -726,6 +775,10 @@
      or discard it.  */
   discard_cleanups (inf_status_cleanup);
 
+  /* Register a clean-up for unwind_on_terminating_exception_breakpoint.  */
+  if (terminate_bp)
+    make_cleanup_delete_breakpoint (terminate_bp);
+
   /* - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP -
      If you're looking to implement asynchronous dummy-frames, then
      just below is the place to chop this function in two..  */
@@ -881,6 +934,39 @@
 
       if (!stop_stack_dummy)
 	{
+
+	  /* Check if unwind on terminating exception behaviour is on.  */
+	  if (unwind_on_terminating_exception_p)
+	    {
+	      /* Check that the breakpoint is our special std::terminate
+		 breakpoint.  If it is, we do not want to kill the inferior
+		 in an inferior function call. Rewind, and warn the
+		 user.  */
+
+	      if (terminate_bp != NULL
+		  && (inferior_thread()->stop_bpstat->breakpoint_at->address
+		      == terminate_bp->loc->address))
+		{
+
+		  /* We must get back to the frame we were before the
+		     dummy call.  */
+		  dummy_frame_pop (dummy_id);
+
+		  /* We also need to restore inferior status to that before the
+		     dummy call.  */
+		  restore_inferior_status (inf_status);
+
+		  error (_("\
+The program being debugged entered a std::terminate call, most likely\n\
+caused by an unhandled C++ exception.  GDB blocked this call in order\n\
+to prevent the program from being terminated, and has restored the\n\
+context to its original state before the call.\n\
+To change this behaviour use \"set unwind-on-terminating-exception off\".\n\
+Evaluation of the expression containing the function (%s)\n\
+will be abandoned."),
+			 name);
+		}
+	    }
 	  /* We hit a breakpoint inside the FUNCTION.
 	     Keep the dummy frame, the user may want to examine its state.
 	     Discard inferior status, we're not at the same point
@@ -989,4 +1075,19 @@
 			   NULL,
 			   show_unwind_on_signal_p,
 			   &setlist, &showlist);
+
+  add_setshow_boolean_cmd ("unwind-on-terminating-exception", no_class,
+			   &unwind_on_terminating_exception_p, _("\
+Set unwinding of stack if std::terminate is called while in call dummy."), _("\
+Show unwinding of stack if std::terminate() is called while in a call dummy."), _("\
+The unwind on terminating exception flag lets the user determine\n\
+what gdb should do if a std::terminate() call is made from the\n\
+default exception handler.  If set, gdb unwinds the stack and restores\n\
+the context to what it was before the call.  If unset, gdb allows the\n\
+std::terminate call to proceed.\n\
+The default is to unwind the frame."),
+			   NULL,
+			   show_unwind_on_terminating_exception_p,
+			   &setlist, &showlist);
+
 }
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.598
diff -u -r1.598 gdb.texinfo
--- doc/gdb.texinfo	28 May 2009 16:49:54 -0000	1.598
+++ doc/gdb.texinfo	9 Jun 2009 10:26:59 -0000
@@ -12865,6 +12865,16 @@
 the function, or if you passed it incorrect arguments).  What happens
 in that case is controlled by the @code{set unwindonsignal} command.
 
+Similarly, with a C@t{++} program it is possible for the function you
+call via the @code{print} or @code{call} command to generate an
+exception that is not handled due to the constraints of the dummy
+frame.  In this case, any exception that is raised in the frame, but has
+an out-of-frame exception handler will not be found.  GDB builds a
+dummy-frame for the inferior function call, and the unwinder cannot
+seek for exception handlers outside of this dummy-frame.  What happens
+in that case is controlled by the
+@code{set unwind-on-terminating-exception} command.
+
 @table @code
 @item set unwindonsignal
 @kindex set unwindonsignal
@@ -12881,6 +12891,23 @@
 @kindex show unwindonsignal
 Show the current setting of stack unwinding in the functions called by
 @value{GDBN}.
+
+@item set unwind-on-terminating-exception
+@kindex set unwind-on-terminating-exception
+@cindex unwind stack in called functions with unhandled exceptions
+@cindex call dummy stack unwinding on unhandled exception.
+Set unwinding of the stack if a C@t{++} exception is raised, but left
+unhandled while in a function that @value{GDBN} called in the program being
+debugged.  If set to on (the default), @value{GDBN} unwinds the stack
+it created for the call and restores the context to what it was before
+the call.  If set to off, @value{GDBN} the exception is delivered to
+the default C@t{++} exception handler and the inferior terminated.
+
+@item show unwind-on-terminating-exception
+@kindex show unwind-on-terminating-exception
+Show the current setting of stack unwinding in the functions called by
+@value{GDBN}.
+
 @end table
 
 @cindex weak alias functions
Index: testsuite/gdb.cp/Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/Makefile.in,v
retrieving revision 1.5
diff -u -r1.5 Makefile.in
--- testsuite/gdb.cp/Makefile.in	3 Feb 2009 01:09:01 -0000	1.5
+++ testsuite/gdb.cp/Makefile.in	9 Jun 2009 10:26:59 -0000
@@ -4,7 +4,7 @@
 EXECUTABLES = ambiguous annota2 anon-union cplusfuncs cttiadd \
 	derivation inherit local member-ptr method misc \
         overload ovldbreak ref-typ ref-typ2 templates userdef virtfunc namespace \
-	ref-types ref-params method2 pr9594
+	ref-types ref-params method2 pr9594 gdb2495
 
 all info install-info dvi install uninstall installcheck check:
 	@echo "Nothing to be done for $@..."
Index: testsuite/gdb.cp/gdb2495.cc
===================================================================
RCS file: testsuite/gdb.cp/gdb2495.cc
diff -N testsuite/gdb.cp/gdb2495.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/gdb2495.cc	9 Jun 2009 10:26:59 -0000
@@ -0,0 +1,89 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2009 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+   */
+
+#include <iostream>
+#include <signal.h>
+
+using namespace std;
+
+class SimpleException
+{
+
+public:
+
+  void raise_signal (int dummy)
+  {
+    if (dummy > 0)
+      raise(SIGABRT);
+  }
+
+  int no_throw_function ()
+  {
+    return 1;
+  }
+
+  void throw_function ()
+  {
+    throw 1;
+  }
+
+  int throw_function_with_handler ()
+  {
+    try
+      {
+	throw 1;
+      }
+    catch (...)
+      {
+	cout << "Handled" << endl;
+      }
+
+    return 2;
+  }
+
+  void call_throw_function_no_handler ()
+  {
+    throw_function ();
+  }
+
+  void call_throw_function_handler ()
+  {
+    throw_function_with_handler ();
+  }
+};
+SimpleException exceptions;
+
+int
+main()
+{
+  /* Have to call these functions so GCC does not optimize them
+     away.  */
+  exceptions.raise_signal (-1);
+  exceptions.no_throw_function ();
+  exceptions.throw_function_with_handler ();
+  exceptions.call_throw_function_handler ();
+  try
+    {
+      exceptions.throw_function ();
+      exceptions.call_throw_function_no_handler ();
+    }
+  catch (...)
+    {
+    }
+  return 0;
+}
Index: testsuite/gdb.cp/gdb2495.exp
===================================================================
RCS file: testsuite/gdb.cp/gdb2495.exp
diff -N testsuite/gdb.cp/gdb2495.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/gdb2495.exp	9 Jun 2009 10:26:59 -0000
@@ -0,0 +1,157 @@
+# Copyright 2009 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+
+# In gdb inferior function calls, if a C++ exception is raised in the
+# dummy-frame, and the exception handler is (normally, and expected to
+# be) out-of-frame, the default C++ handler will (wrongly) be called
+# in an inferior function call.
+# This is incorrect as an exception can normally and legally be handled
+# out-of-frame. The confines of the dummy frame prevent the unwinder
+# from finding the correct handler (or any handler, unless it is
+# in-frame). The default handler calls std::terminate. This will kill
+# the inferior. Assert that terminate should never be called in an
+# inferior function call. These tests test the functionality around
+# unwinding that sequence and also tests the flag behaviour gating this
+# functionality.
+
+# This test is largely based of gdb.base/callfuncs.exp.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+if { [skip_cplus_tests] } { continue }
+
+set prms_id 2495
+set bug_id 0
+
+set testfile "gdb2495"
+set srcfile ${testfile}.cc
+set binfile $objdir/$subdir/$testfile
+
+# Create and source the file that provides information about the compiler
+# used to compile the test case.
+if [get_compiler_info ${binfile} "c++"] {
+    return -1
+}
+
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug c++}] != "" } {
+     untested gdb2495.exp
+     return -1
+}
+
+# Some targets can't do function calls, so don't even bother with this
+# test.
+if [target_info exists gdb,cannot_call_functions] {
+    setup_xfail "*-*-*" 2416
+    fail "This target can not call functions"
+    continue
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if ![runto_main] then {
+    perror "couldn't run to main"
+    continue
+}
+
+# See http://sourceware.org/gdb/bugs/2495
+
+# Test normal baseline behaviour. Call a function that
+# does not raise an exception.
+gdb_test "p exceptions.no_throw_function()" " = 1"
+# And one that does but handles it in-frame.
+gdb_test "p exceptions.throw_function_with_handler()" " = 2"
+# Both should return normally.
+
+# Test basic unwind.  Call a function that raises an exception but
+# does not handle it.  It should be rewound.
+gdb_test "p exceptions.throw_function()" \
+    "The program being debugged entered a std::terminate call, .*" \
+    "Call a function that raises an exception without a handler."
+
+# Make sure that after rewinding we are back at the call parent.
+gdb_test "bt" \
+    "#0  main.*" \
+    "bt after returning from a popped frame"
+
+# Make sure the only breakpoint is the one set via the runto_main
+# call and that the std::terminate breakpoint has evaporated and
+# cleaned-up.
+gdb_test "info breakpoints" \
+    "gdb.cp/gdb2495\.cc.*"
+
+# Turn off this new behaviour.
+gdb_test_multiple "set unwind-on-terminating-exception off" \
+    "Turn unwind-on-terminating-exception off" {
+    -re "$gdb_prompt $" {pass "set unwinn-on-terminating-exception off"}
+    timeout {fail "(timeout) set unwind-on-terminating-exception off"}
+}
+
+# Check that it is turned off.
+gdb_test "show unwind-on-terminating-exception" \
+    "exception is unhandled while in a call dummy is off.*" \
+    "Turn off unwind on terminating exception flag"
+
+# Check that the old behaviour is restored.
+gdb_test "p exceptions.throw_function()" \
+    "The program being debugged was signaled while in a function called .*" \
+    "Call a function that raises an exception with unwinding off.."
+
+# Restart the inferior back at main.
+if ![runto_main] then {
+    perror "couldn't run to main"
+    continue
+}
+
+
+# Check to see if the new behaviour alters the unwind signal
+# behaviour; it should not.  Test both on and off states.
+
+# Turn on unwind on signal behaviour.
+gdb_test_multiple "set unwindonsignal on" "Turn unwindonsignal on" {
+    -re "$gdb_prompt $" {pass "set unwindonsignal on"}
+    timeout {fail "(timeout) set unwindonsignal on"}
+}
+
+# Check that it is turned on.
+gdb_test "show unwindonsignal" \
+    "signal is received while in a call dummy is on.*" \
+    "Turn on unwind on signal"
+
+# Check to see if new behaviour interferes with
+# normal signal handling in inferior function calls.
+gdb_test "p exceptions.raise_signal(1)" \
+    "To change this behavior use \"set unwindonsignal off\".*"
+
+# And reverse - turn off again.
+gdb_test_multiple "set unwindonsignal off" "Turn unwindonsignal off" {
+    -re "$gdb_prompt $" {pass "set unwindonsignal off"}
+    timeout {fail "(timeout) set unwindonsignal off"}
+}
+
+# Check that it is actually turned off.
+gdb_test "show unwindonsignal" \
+    "signal is received while in a call dummy is off.*" \
+    "Turn off unwind on signal"
+
+# Check to see if new behaviour interferes with
+# normal signal handling in inferior function calls.
+gdb_test "p exceptions.raise_signal(1)" \
+    "To change this behavior use \"set unwindonsignal on\".*"

Re: [patch] try 2 - Do not allow unwinder to terminate inferior in an inferior function call if a C++ exception handler cannot be found.

[Joel Brobecker]

> 2009-06-09  Phil Muldoon  <pmuldoon@redhat.com>
>
>        * infcall.c (show_unwind_on_terminating_exception_p): New
>        function.
>        (call_function_by_hand): Create breakpoint and clean-up call for
>        std::terminate.breakpoint. Add unwind_on_terminating_exception_p
>        gate. Pop frame on breakpoint hit.
>        (_initialize_infcall): Add add_setshow_boolean_cmd for
>        unwind-on-terminating-exception.
>
> gdb/testsuite/ChangeLog
>
> 2009-06-09  Phil Muldoon  <pmuldoon@redhat.com>
>
>        * gdb.cp/gdb2495.cc: New file.
>        * gdb.cp/gdb2495.exp: New file.

These two parts look OK to me. Just a couple of formatting nits to be
fixed before you commit.

> +     handler, unless it is in-frame).  The default handler calls
> +     std::terminate.  This will kill the inferior. Assert that
                                                     ^ Missing space

> +  if (unwind_on_terminating_exception_p)
> +     {
> +       struct minimal_symbol *tm = lookup_minimal_symbol  ("std::terminate()",
> +							   NULL, NULL);
> +       if (tm != NULL)
> +	 {
> +	   terminate_bp = set_momentary_breakpoint_at_pc
> +	     (SYMBOL_VALUE_ADDRESS (tm),  bp_breakpoint);
> +	}

I think that the general style in GDB is to omit the curly braces
when the body of an if/while is only one statement.

> +	      if (terminate_bp != NULL
> +		  && (inferior_thread()->stop_bpstat->breakpoint_at->address
> +		      == terminate_bp->loc->address))
> +		{
> +
> +		  /* We must get back to the frame we were before the

Let's remove the empty line before the comment...

> +# This is incorrect as an exception can normally and legally be handled
> +# out-of-frame. The confines of the dummy frame prevent the unwinder
> +# from finding the correct handler (or any handler, unless it is
> +# in-frame). The default handler calls std::terminate. This will kill
> +# the inferior. Assert that terminate should never be called in an
> +# inferior function call. These tests test the functionality around
> +# unwinding that sequence and also tests the flag behaviour gating this
> +# functionality.

Two spaces after a period throught the paragraph...

Thanks!
-- 
Joel

Re: [patch] try 2 - Do not allow unwinder to terminate inferior in an inferior function call if a C++ exception handler cannot be found.

[Eli Zaretskii]

> Date: Fri, 12 Jun 2009 00:32:12 +0100
> From: Phil Muldoon <pmuldoon@redhat.com>
> CC: gdb-patches ml <gdb-patches@sourceware.org>, Eli Zaretskii <eliz@gnu.org>
> 
> Thanks, I've fixed the nits. Once Eli has approved the documentation 
> updates, I'll commit.

Sorry for the delay (I'm traveling).  The documentation patch is
approved.

Thanks.

Re: [patch] try 2 - Do not allow unwinder to terminate inferior in an inferior function call if a C++ exception handler cannot be found.

[Phil Muldoon]

On 06/13/2009 03:20 AM, Eli Zaretskii wrote:
>> Date: Fri, 12 Jun 2009 00:32:12 +0100
>> From: Phil Muldoon<pmuldoon@redhat.com>
>> CC: gdb-patches ml<gdb-patches@sourceware.org>, Eli Zaretskii<eliz@gnu.org>
>>
>> Thanks, I've fixed the nits. Once Eli has approved the documentation
>> updates, I'll commit.
>>      
>
> Sorry for the delay (I'm traveling).  The documentation patch is
> approved.
>
> Thanks.
>    

FYI I've checked this in.