Re: ping: [patch 1/2] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #5

[Joel Brobecker <brobecker@adacore.com>]

Hello,

Does anyone have any comment on this patch from Jan? It has been
identified as necessary before the release process gets started.

> gdb/
> 2012-03-27  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Remove momentary breakpoints for completed inferior calls.
> 	* breakpoint.h (bp_call_dummy): Update the comment.
> 	* dummy-frame.c: Include gdbthread.h.
> 	(pop_dummy_frame_bpt): New function.
> 	(pop_dummy_frame): Initialie DUMMY earlier.  Call pop_dummy_frame_bpt.
> 
> gdb/testsuite/
> 2012-03-09  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Remove momentary breakpoints for completed inferior calls.
> 	* gdb.base/call-signal-resume.exp (maintenance print dummy-frames)
> 	(maintenance info breakpoints): New tests.

I took a look, and nothing really obvious jumped at me. I'd wait a few
more days, and go ahead and commit if you hear no objection.

Just a quick question from me: Why do we need to remove the breakpoint?
Is it just for GDB's benefit (purge the breakpoint from our breakpoint
list), or also for the inferior's benefit.  Given that we are inserting
the breakpoint in scratch memory, it shouldn't make any different to
the inferior, right?

A couple of suggestions on the phrasing of the comment.

Thanks!

> 
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -95,10 +95,10 @@ enum bptype
>  
>      /* The breakpoint at the end of a call dummy.  */
>      /* FIXME: What if the function we are calling longjmp()s out of
> -       the call, or the user gets out with the "return" command?  We
> -       currently have no way of cleaning up the breakpoint in these
> -       (obscure) situations.  (Probably can solve this by noticing
> -       longjmp, "return", etc., it's similar to noticing when a
> +       the call?  "return" command is handled by pop_dummy_frame_bpt.

                  ^^^ The "return" command


> +       We currently have no way of cleaning up the breakpoint in such
> +       (obscure) situation.  (Probably can solve this by noticing
                               ^^^ We can probably solve
                               (also lose the extra parens, I don't think
                               they bring anything, and it will avoid
                               the parens nesting)
> +       longjmp, etc., it's similar to noticing when a
>         watchpoint on a local variable goes out of scope (with hardware
>         support for watchpoints)).  */
>      bp_call_dummy,
> --- a/gdb/dummy-frame.c
> +++ b/gdb/dummy-frame.c
> @@ -29,6 +29,7 @@
>  #include "gdbcmd.h"
>  #include "gdb_string.h"
>  #include "observer.h"
> +#include "gdbthread.h"
>  
>  /* Dummy frame.  This saves the processor state just prior to setting
>     up the inferior function call.  Older targets save the registers
> @@ -108,19 +109,36 @@ remove_dummy_frame (struct dummy_frame **dummy_ptr)
>    xfree (dummy);
>  }
>  
> +/* Delete any breakpoint B which is a momentary breakpoint for return from
> +   inferior call matching DUMMY_VOIDP.  */
> +
> +static int
> +pop_dummy_frame_bpt (struct breakpoint *b, void *dummy_voidp)
> +{
> +  struct dummy_frame *dummy = dummy_voidp;
> +
> +  if (b->disposition == disp_del && frame_id_eq (b->frame_id, dummy->id)
> +      && b->thread == pid_to_thread_id (inferior_ptid))
> +    delete_breakpoint (b);
> +
> +  /* Continue the traversal.  */
> +  return 0;
> +}
> +
>  /* Pop *DUMMY_PTR, restoring program state to that before the
>     frame was created.  */
>  
>  static void
>  pop_dummy_frame (struct dummy_frame **dummy_ptr)
>  {
> -  struct dummy_frame *dummy;
> +  struct dummy_frame *dummy = *dummy_ptr;
> +
> +  restore_infcall_suspend_state (dummy->caller_state);
>  
> -  restore_infcall_suspend_state ((*dummy_ptr)->caller_state);
> +  iterate_over_breakpoints (pop_dummy_frame_bpt, dummy);
>  
>    /* restore_infcall_control_state frees inf_state,
>       all that remains is to pop *dummy_ptr.  */
> -  dummy = *dummy_ptr;
>    *dummy_ptr = dummy->next;
>    xfree (dummy);
>  
> --- a/gdb/testsuite/gdb.base/call-signal-resume.exp
> +++ b/gdb/testsuite/gdb.base/call-signal-resume.exp
> @@ -101,6 +101,18 @@ gdb_test "frame $frame_number" ".*"
>  gdb_test_no_output "set confirm off"
>  gdb_test "return" ""
>  
> +# Verify there are no remains of the dummy frame.
> +gdb_test_no_output "maintenance print dummy-frames"
> +set test "maintenance info breakpoints"
> +gdb_test_multiple $test $test {
> +    -re "call dummy.*\r\n$gdb_prompt $" {
> +	fail $test
> +    }
> +    -re "\r\n$gdb_prompt $" {
> +	pass $test
> +    }
> +}
> +
>  # Resume execution, the program should continue without any signal.
>  
>  gdb_test "break stop_two" "Breakpoint \[0-9\]* at .*"

-- 
Joel