Re: RFC: fix bug with std::terminate handler

[Pedro Alves <pedro at codesourcery dot com>]

Hi Tom, sorry for the delay.

On Tuesday 16 March 2010 20:49:03, Tom Tromey wrote:
> Pedro> A `gdb_assert (type != bp_breakpoint)' in
> Pedro> set_momentary_breakpoint would be a Nice To Have.
> 
> I didn't do this, as I'm not familiar enough with the process record
> code.

That's fine.

> 
> Pedro> There's also the option of making the breakpoint at
> Pedro> std::terminate be a real internal breakpoint, enabled/disabled
> Pedro> on need, a-la-E.g., enable_overlay_breakpoints or
> Pedro> set_longjmp_breakpoint.
> 
> I took this route.

Thanks.

> 
> Pedro> Also see a couple of PRs I just opened related to this.
> Pedro> <http://sourceware.org/bugzilla/show_bug.cgi?id=11328>
> Pedro>  <http://sourceware.org/bugzilla/show_bug.cgi?id=11327>
> 
> This patch fixes these problems as well.  Thanks for pointing them out.

Great!

> First, I changed the type of stop_stack_dummy to be an enum.  Then I use
> this to record whether we hit a stack dummy or a std::terminate
> breakpoint.  This seemed pretty direct and ok to me, but if you want
> something else, just say what and I will implement it.  I think the
> names of the enumerators are not super.

Yeah.  I'm fine with leaving as is.  Dunno what others think.
I guess we could get rid of this variable altogether by looking
over the stop_bpstat of the thread instead.

> 2010-03-16  Tom Tromey  <tromey@redhat.com>
> 
>         PR gdb/11327, PR gdb/11328:

PR11368 as well?

>         * infrun.c (handle_inferior_event): Change initialization of
>         stop_stack_dummy.
>         (handle_inferior_event): Change assignment to stop_stack_dummy.
>         (normal_stop): Update use of stop_stack_dummy.
>         (struct inferior_status) <stop_stack_dummy>: Change type.
>         * inferior.h (stop_stack_dummy): Update.
>         * infcmd.c (stop_stack_dummy): Change type.
>         * infcall.c (cleanup_delete_std_terminate_breakpoint): New
>         function.
>         (call_function_by_hand): Call set_std_terminate_breakpoint.
>         Rewrite std::terminate handling.
>         * breakpoint.h (enum bptype) <bp_std_terminate,
>         bp_std_terminate_master>: New.
>         (enum stop_stack_kind): New.
>         (struct bpstat_what) <call_dummy>: Change type.
>         (set_std_terminate_breakpoint, delete_std_terminate_breakpoint):
>         Declare.
>         * breakpoint.c (create_std_terminate_master_breakpoint): New
>         function.
>         (update_breakpoints_after_exec): Handle bp_std_terminate_master.
>         Call create_std_terminate_master_breakpoint.
>         (breakpoint_init_inferior): Handle bp_std_terminate.
>         (print_it_typical): Handle new breakpoint kinds.
>         (bpstat_stop_status): Handle bp_std_terminate_master.
>         (bpstat_what): Correctly set call_dummy field.  Handle
>         bp_std_terminate_master and bp_std_terminate.
>         (print_one_breakpoint_location): Update.
>         (allocate_bp_location): Update.
>         (set_std_terminate_breakpoint): New function.
>         (delete_std_terminate_breakpoint): Likewise.
>         (create_thread_event_breakpoint): Update.
>         (delete_command): Update.
>         (breakpoint_re_set_one): Update.
>         (breakpoint_re_set): Call create_std_terminate_master_breakpoint.

> @@ -2295,6 +2331,7 @@ breakpoint_init_inferior (enum inf_context context)
>      switch (b->type)
>        {
>        case bp_call_dummy:
> +      case bp_std_terminate:


Hmm, why's this needed?  Doesn't the cleanup handling in infcall
always delete these?  longjmp breakpoints also aren't deleted here.
In any case, if this stays for some reason, the comment below
doesn't apply to this breakpoint, so move the case further below,
just before delete_breakpoint, for instance --- notice how each
case has a describing comment below it.

> -      if (!stop_stack_dummy)
> +      if (stop_stack_dummy == STOP_STD_TERMINATE)
>         {
> +         /* We must get back to the frame we were before the dummy
> +            call.  */
> +         dummy_frame_pop (dummy_id);

One thing I wonder is what should happen if there's
a user breakpoint on "std::terminate".  My intuition says that
we should not auto-unwind in that case (user breakpoints should
have higher precedence than internal handlings), but your change
matches the current behavior, so it's fine.

Otherwise, this looks good to me.  Thanks!

-- 
Pedro Alves