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

[Joel Brobecker <brobecker at adacore dot com>]

Phil,

>    2009-04-23  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.

This looks generally OK.  Watch out, however, for lots and lots of
trailing spaces both in the code and the documentation. I'll try to
point them out to you below. Do check the patches for the doc and
the testsuite, as I haven't checked these for this type of issue.

>      gdb/testsuite/ChangeLog
>    2009-04-23  Phil Muldoon  <pmuldoon@redhat.com>
>          * gdb.cp/gdb2495.cc: New file.
>        * gdb.cp/gdb2495.exp: New file.
>      gdb/doc/ChangeLog
>    2009-04-23  Phil Muldoon  <pmuldoon@redhat.com>
>          * doc/gdb.texinfo (Calling): Document
>        set-unwind-on-terminating-exception usage.
>

> +/* This boolean tells what gdb should do if a std::terminate call is
> +   made while in a function called from gdb (call dummy).  

Trailing spaces

> +   If set, gdb unwinds the stack and restores the context to what it 

Trailing space.

> +static int unwind_on_terminating_exception_p = 1;

Would you mind adding an empty after this declaration?

> +  struct breakpoint *terminate_bp = 0;

That's nit-picking, and I apologize, but since we're going to make
adjustments to your patch, would you mind using NULL, here? I don't
think one is better than the other, but NULL is more explicit, IMO.

> +  struct minimal_symbol *tm;

Can you move that declaration inside the small block where it is
actually used?

>      bpt->disposition = disp_del;
>    }
> +  

Trailing spaces - just before:

> +  /* 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, 

Trailing space in the 3 lines above.

> + 	  terminate_bp = set_momentary_breakpoint_at_pc
> + 	    (SYMBOL_VALUE_ADDRESS (tm),  bp_breakpoint);
> + 	  make_cleanup_delete_breakpoint (terminate_bp);
> + 	}

The 4 lines above start with a space followed by a tab.

> +  if (! target_has_execution)
> +    {
> +      /* If we try to restore the inferior status (via the cleanup),
> +	 we'll crash as the inferior is no longer running.  */
> +      discard_cleanups (inf_status_cleanup);
> +      discard_inferior_status (inf_status);
> +      error (_("\
> +The program being debugged exited while in a function called from GDB."));
> +    }
> +

There is already handling of !target_has_execution a few lines above.
Perhaps it's been introduced after you sent your patch? Anyway, it
does almost exactly the same except that it's missing the
"discard_cleanups" call. As far as I can tell, this is not necessary
either, because it's already been discarded earlier:

  /* Everything's ready, push all the info needed to restore the
     caller (and identify the dummy-frame) onto the dummy-frame
     stack.  */
  dummy_frame_push (caller_state, &dummy_id);

  /* Discard both inf_status and caller_state cleanups.
     From this point on we explicitly restore the associated state
     or discard it.  */
  discard_cleanups (inf_status_cleanup);

So I think you can discard this hunk entirely.


> @@ -880,6 +938,39 @@
>  
>        if (!stop_stack_dummy)
>  	{
> +	  

Trailing space.

> + 	  /* Check if unwind on terminating exception behaviour is on.  */
> + 	  if (unwind_on_terminating_exception_p)
> + 	    {
> + 	      /* Check that the breakpoint is our special std::terminate

The 4 lines above start with a space followed by a tab.

> +		 breakpoint.  If it is, we do not want to kill the inferior
> +		 in an inferior function call. Rewind, and warn the
> +		 user.  */
> +	      

Trailing space

> + 	      if ((terminate_bp != NULL) &&
> + 		  (inferior_thread()->stop_bpstat->breakpoint_at->address
> + 		   == terminate_bp->loc->address)) 
> + 		
> + 		
> + 		{

The lines above start with a space followed by a tab. Some of them also
have trailing spaces. Can you delete the 2 empty lines before the curly
brace? Lastly, binary operators should be at the beginning of the line:

               if (terminate_bp != NULL
                   && (inferior_thread()->stop_bpstat->breakpoint_at->address
                       == terminate_bp->loc->address))

> +		  
Trailing spaces
> +		  /* We must get back to the frame we were before the
> +		     dummy call.  */
> +		  dummy_frame_pop (dummy_id);
> +		  
Trailing spaces
> +		  /* We also need to restore inferior status to that before the
> +		     dummy call.  */
> +		  restore_inferior_status (inf_status);
> +		  
Trailing spaces
> +		  error (_("\
> +The program being debugged entered a std::terminate call which would\n\
> +have terminated the program being debugged.  GDB has restored the\n\
> +context to what it was before the call\n\
> +To change this behaviour use \"set unwind-on-terminating-exception off\"\n\
> +Evaluation of the expression containing the function (%s) will be abandoned."), 

I think it would be nice to mention the fact that this was likely caused
by an unhandled exception. Perhaps:

The program being debugged entered a std::terminate call, most likely
caused by an unhandled C++ exception.  GDB blocked this call in order
to prevent the program from being terminated, and has restored the
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."), 

(notice the period at the end the "To change ..." sentence, and I also
split the "Evaluation of ..." because the function name is going to take
a little bit of room in our line.

> +			 name);
> + 		}
> + 	    } 

The two lines above start with a space followed by a tab. Last one has
a trailing space.

> +frame.  What happens in that case is controlled by the  
Trailing spaces.

> +Set unwinding of the stack if a C++ exception is raised but unhandled  
Trailing spaces.

> +  int  no_throw_function () 

Nit-picking warning: Extra space after "int". Looks like there's also
a trailing space at the end.

> +# This test is largley based off gdb.base/callfuncs.exp.
                  ^^^^^^^       ^^^
                  spelling      on (based on)

> +# See http://sources.redhat.com/gdb/bugs/2495

Can you use sourceware.org?

> +# Turn off this new behaviour.
> +send_gdb "set unwind-on-terminating-exception off\n" 
> +gdb_expect {
> +    -re "$gdb_prompt $" {pass "set unwind-on-terminating-exception"}
> +    timeout {fail "(timeout) set  unwind-on-terminating-exception"}

Can you used gdb_test_multiple, here? We should be using send_gdb/gdb_expect
only if we can't do otherwise.

> +# Check that the old behaviour is restored.
> +gdb_test "p exceptions.throw_function()" \
> +    "The program being debugged stopped while in a function called .*" \
> +    "Call a function that raises an exception with unwinding off.."

This one surprised me. Shouldn't GDB say "the program exited"?

> +# Turn on unwind on signal behaviour.
> +send_gdb "set unwindonsignal on\n" 
> +gdb_expect {
> +    -re "$gdb_prompt $" {pass "set unwindonsignal on"}
> +    timeout {fail "(timeout) set  unwindonsignal on"}

Same as above.

> +# And reverse - turn off again.
> +send_gdb "set unwindonsignal off\n" 
> +gdb_expect {
> +    -re "$gdb_prompt $" {pass "set unwindonsignal off"}
> +    timeout {fail "(timeout) set  unwindonsignal off"}

Ditto.

-- 
Joel