[RFA] GDB/624 - tbreak commands not executed when breakpoint hit

[RFA] GDB/624 - tbreak commands not executed when breakpoint hit

[Joel Brobecker]

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

As described in PR GDB/624:

Using the small program from gdb.base/commands.exp, the following copy
of a GDB sessions shows that commands associated to temporary
breakpoints are not executed when the breakpoint is hit:

        (gdb) tbreak factorial
        Breakpoint 1 at 0x8048582: file ./gdb.base/run.c, line 77.
        (gdb) commands
        Type commands for when breakpoint 1 is hit, one per line.
        End with a line saying just "end".
        >silent
        >printf "factorial command-list executed\n"
        >cont
        >end
        (gdb) run 1
        Starting program: [...]/gdb.base/commands 1
        (gdb)

The backtrace confirms that the program executed up to the factorial
breakpoint, and then stopped.

        (gdb) bt
        #0  factorial (value=1) at ./gdb.base/run.c:77
        #1  0x0804855e in main (argc=2, argv=0xbffffa34, envp=0xbffffa40)
            at ./gdb.base/run.c:57
        #2  0x4005514f in __libc_start_main () from /lib/libc.so.6
        (gdb) p /x $pc
        $1 = 0x8048582        <<<--- $pc is equal to bp #1 address

The expected output after the run command was:

        (gdb) run 1
        Starting program: [...]/gdb.base/commands 1
        factorial command-list executed
        1

        Program exited normally.
        (gdb)

The problem is that the breakpoint is deleted very early in the inferior
stop handling, so the rest of the code does not have a chance to see
that there was a temporary breakpoint there with a commands list to
execute. To fix this, it is necessary to delay a bit the deletion of
this breakpoint.

This is what the attached patch does. There is a small side-effect,
which I think should be ok, is that the after the temporary breakpoint
is hit, there is a small period during which the breakpoint is still
visible when doing ``info break''. The status report is disabled, and to
be deleted at the next stop.

I also attached an extra test in gdb.base/commands.exp.

No new regression introduced on x86-linux.

Ok to apply?

2002-07-31  Joel Brobecker  <brobecker@gnat.com>

        * breakpoint.c (breakpoint_auto_delete): Do not delete temporary
        breakpoints to which a list of commands is attached. Mark them
        for deletion at the next stop instead. Also disable them to avoid
        hitting these breakpoints again.
        Move the part that deletes the to-be-deleted breakpoints before
        the part that deletes the temporary breakpoints, to avoid immediately
        deleting the temporary breakpoints that we just marked for deletion.

2002-07-31  Joel Brobecker  <brobecker@gnat.com>

        * gdb.base/commands.exp (temporary_breakpoint_commands): New test.

Thanks,
-- 
Joel

[-- Attachment #2: tbreak_commands.diff --]
[-- Type: text/plain, Size: 5320 bytes --]

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.78
diff -c -3 -p -r1.78 breakpoint.c
*** breakpoint.c	26 Jun 2002 05:20:04 -0000	1.78
--- breakpoint.c	31 Jul 2002 20:08:35 -0000
*************** top:
*** 1882,1888 ****
    breakpoint_proceeded = 0;
    for (; bs != NULL; bs = bs->next)
      {
!       cmd = bs->commands;
        while (cmd != NULL)
  	{
  	  execute_control_command (cmd);
--- 1882,1892 ----
    breakpoint_proceeded = 0;
    for (; bs != NULL; bs = bs->next)
      {
!       /* Use a temporary copy of the commands, as one command in the list
!          may cause this breakpoint and its commands to be deleted.  */
!       cmd = copy_command_lines (bs->commands);
!       make_cleanup_free_command_lines (&cmd);
! 
        while (cmd != NULL)
  	{
  	  execute_control_command (cmd);
*************** breakpoint_auto_delete (bpstat bs)
*** 6575,6590 ****
  {
    struct breakpoint *b, *temp;
  
-   for (; bs; bs = bs->next)
-     if (bs->breakpoint_at && bs->breakpoint_at->disposition == disp_del
- 	&& bs->stop)
-       delete_breakpoint (bs->breakpoint_at);
- 
    ALL_BREAKPOINTS_SAFE (b, temp)
    {
      if (b->disposition == disp_del_at_next_stop)
        delete_breakpoint (b);
    }
  }
  
  /* Delete a breakpoint and clean up all traces of it in the data
--- 6579,6607 ----
  {
    struct breakpoint *b, *temp;
  
    ALL_BREAKPOINTS_SAFE (b, temp)
    {
      if (b->disposition == disp_del_at_next_stop)
        delete_breakpoint (b);
    }
+ 
+   for (; bs; bs = bs->next)
+     if (bs->breakpoint_at && bs->breakpoint_at->disposition == disp_del
+ 	&& bs->stop)
+       {
+         if (bs->commands)
+           {
+             /* Don't delete this breakpoint yet, as there are some
+                commands associated to this temporary breakpoint that
+                need to be executed before.  Mark this breakpoint for
+                deletion at the next stop, and also disable this breakpoint
+                to avoid hitting it later on.  */
+             bs->breakpoint_at->disposition = disp_del_at_next_stop;
+             disable_breakpoint (bs->breakpoint_at);
+           }
+         else
+           delete_breakpoint (bs->breakpoint_at);
+       }
  }
  
  /* Delete a breakpoint and clean up all traces of it in the data
Index: testsuite/gdb.base/commands.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/commands.exp,v
retrieving revision 1.10
diff -c -3 -p -r1.10 commands.exp
*** testsuite/gdb.base/commands.exp	13 Dec 2001 22:42:23 -0000	1.10
--- testsuite/gdb.base/commands.exp	31 Jul 2002 20:08:36 -0000
*************** proc deprecated_command_test {} {
*** 440,445 ****
--- 440,498 ----
  	    "deprecate with no arguments"
  }
  
+ proc temporary_breakpoint_commands {} {
+     global gdb_prompt
+     
+     gdb_test "set args 1" "" "set args in temporary_breakpoint_commands"
+     delete_breakpoints
+ 
+     # Create a temporary breakpoint, and associate a commands list to it.
+     # This test will verify that this commands list is executed when the
+     # breakpoint is hit.
+     gdb_test "tbreak factorial" \
+              "Breakpoint \[0-9\]+ at .*: file .*/run.c, line \[0-9\]+\." \
+              "breakpoint in temporary_breakpoint_commands"
+     
+     send_gdb "commands\n"
+     gdb_expect {
+       -re "Type commands for when breakpoint .* is hit, one per line.*>" {
+           pass "begin commands in bp_deleted_in_command_test"
+       }
+       -re "$gdb_prompt $" {fail "begin commands in bp_deleted_in_command_test"}
+       timeout             {fail "(timeout) begin commands bp_deleted_in_command_test"}
+     }
+     send_gdb "silent\n"
+     gdb_expect {
+         -re ">"               {pass "add silent tbreak command"}
+         -re "$gdb_prompt $"   {fail "add silent tbreak command"}
+         timeout               {fail "(timeout) add silent tbreak command"}
+     }
+     send_gdb "printf \"factorial tbreak commands executed\\n\"\n"
+     gdb_expect {
+         -re ">"               {pass "add printf tbreak command"}
+         -re "$gdb_prompt $"   {fail "add printf tbreak command"}
+         timeout               {fail "(timeout) add printf tbreak command"}
+     }
+     send_gdb "cont\n"
+     gdb_expect {
+         -re ">"               {pass "add cont tbreak command"}
+         -re "$gdb_prompt $"   {fail "add cont tbreak command"}
+         timeout               {fail "(timeout) add cont tbreak command"} }
+     send_gdb "end\n"
+     gdb_expect {
+         -re "$gdb_prompt $"   {pass "end tbreak commands"}
+         timeout               {fail "(timeout) end tbreak commands"}
+     }
+ 
+     gdb_run_cmd
+     gdb_expect {
+         -re ".*factorial tbreak commands executed.*1.*Program exited normally.*" {
+             pass "run factorial until temporary breakpoint"
+         }
+         timeout { fail "(timeout) run factorial until temporary breakpoint" }
+     }
+ 
+ }
  
  gdbvar_simple_if_test
  gdbvar_simple_while_test
*************** user_defined_command_test
*** 454,456 ****
--- 507,510 ----
  watchpoint_command_test
  test_command_prompt_position
  deprecated_command_test
+ temporary_breakpoint_commands

Re: [RFA] GDB/624 - tbreak commands not executed when breakpoint hit

[Joel Brobecker]

Grrr,

This chunck is of course not part of this RFA (see another RFA for PR
GDB/622). Sorry.

> diff -c -3 -p -r1.78 breakpoint.c
> *** breakpoint.c	26 Jun 2002 05:20:04 -0000	1.78
> --- breakpoint.c	31 Jul 2002 20:08:35 -0000
> *************** top:
> *** 1882,1888 ****
>     breakpoint_proceeded = 0;
>     for (; bs != NULL; bs = bs->next)
>       {
> !       cmd = bs->commands;
>         while (cmd != NULL)
>   	{
>   	  execute_control_command (cmd);
> --- 1882,1892 ----
>     breakpoint_proceeded = 0;
>     for (; bs != NULL; bs = bs->next)
>       {
> !       /* Use a temporary copy of the commands, as one command in the list
> !          may cause this breakpoint and its commands to be deleted.  */
> !       cmd = copy_command_lines (bs->commands);
> !       make_cleanup_free_command_lines (&cmd);
> ! 
>         while (cmd != NULL)
>   	{
>   	  execute_control_command (cmd);

-- 
Joel

Re: [RFA] GDB/624 - tbreak commands not executed when breakpoint hit

[Joel Brobecker]

ping?

> 2002-07-31  Joel Brobecker  <brobecker@gnat.com>
> 
>         * breakpoint.c (breakpoint_auto_delete): Do not delete temporary
>         breakpoints to which a list of commands is attached. Mark them
>         for deletion at the next stop instead. Also disable them to avoid
>         hitting these breakpoints again.
>         Move the part that deletes the to-be-deleted breakpoints before
>         the part that deletes the temporary breakpoints, to avoid immediately
>         deleting the temporary breakpoints that we just marked for deletion.
> 
> 2002-07-31  Joel Brobecker  <brobecker@gnat.com>
> 
>         * gdb.base/commands.exp (temporary_breakpoint_commands): New test.


-- 
Joel

Re: [RFA] GDB/624 - tbreak commands not executed when breakpoint hit

[Michael Snyder]

Joel Brobecker wrote:
> 
> As described in PR GDB/624:
> 
> Using the small program from gdb.base/commands.exp, the following copy
> of a GDB sessions shows that commands associated to temporary
> breakpoints are not executed when the breakpoint is hit:
> 
>         (gdb) tbreak factorial
>         Breakpoint 1 at 0x8048582: file ./gdb.base/run.c, line 77.
>         (gdb) commands
>         Type commands for when breakpoint 1 is hit, one per line.
>         End with a line saying just "end".
>         >silent
>         >printf "factorial command-list executed\n"
>         >cont
>         >end
>         (gdb) run 1
>         Starting program: [...]/gdb.base/commands 1
>         (gdb)
> 
> The backtrace confirms that the program executed up to the factorial
> breakpoint, and then stopped.
> 
>         (gdb) bt
>         #0  factorial (value=1) at ./gdb.base/run.c:77
>         #1  0x0804855e in main (argc=2, argv=0xbffffa34, envp=0xbffffa40)
>             at ./gdb.base/run.c:57
>         #2  0x4005514f in __libc_start_main () from /lib/libc.so.6
>         (gdb) p /x $pc
>         $1 = 0x8048582        <<<--- $pc is equal to bp #1 address
> 
> The expected output after the run command was:
> 
>         (gdb) run 1
>         Starting program: [...]/gdb.base/commands 1
>         factorial command-list executed
>         1
> 
>         Program exited normally.
>         (gdb)
> 
> The problem is that the breakpoint is deleted very early in the inferior
> stop handling, so the rest of the code does not have a chance to see
> that there was a temporary breakpoint there with a commands list to
> execute. To fix this, it is necessary to delay a bit the deletion of
> this breakpoint.
> 
> This is what the attached patch does. There is a small side-effect,
> which I think should be ok, is that the after the temporary breakpoint
> is hit, there is a small period during which the breakpoint is still
> visible when doing ``info break''. The status report is disabled, and to
> be deleted at the next stop.
> 
> I also attached an extra test in gdb.base/commands.exp.
> 
> No new regression introduced on x86-linux.
> 
> Ok to apply?

Joel, this implementation, with its use of delete_at_next_stop, 
seems clumsy and, to be frank, scary.  Instead, what if you just
made a temporary copy of the tbreak command-list (like you do for
GDB/622), then delete the breakpoint, and later execute the commands?



> 2002-07-31  Joel Brobecker  <brobecker@gnat.com>
> 
>         * breakpoint.c (breakpoint_auto_delete): Do not delete temporary
>         breakpoints to which a list of commands is attached. Mark them
>         for deletion at the next stop instead. Also disable them to avoid
>         hitting these breakpoints again.
>         Move the part that deletes the to-be-deleted breakpoints before
>         the part that deletes the temporary breakpoints, to avoid immediately
>         deleting the temporary breakpoints that we just marked for deletion.
> 
> 2002-07-31  Joel Brobecker  <brobecker@gnat.com>
> 
>         * gdb.base/commands.exp (temporary_breakpoint_commands): New test.
> 
> Thanks,
> --
> Joel
> 
>   ------------------------------------------------------------------------
> 
>    tbreak_commands.diffName: tbreak_commands.diff
>                        Type: Plain Text (text/plain)

Re: [RFA] GDB/624 - tbreak commands not executed when breakpoint hit

[Joel Brobecker]

Michael,

> Joel, this implementation, with its use of delete_at_next_stop, 
> seems clumsy and, to be frank, scary.  Instead, what if you just
> made a temporary copy of the tbreak command-list (like you do for
> GDB/622), then delete the breakpoint, and later execute the commands?

Thanks for your review. I need a bit of time to delve into this again,
but I'll be quite busy in the next 2 or 3 weeks. You can consider this
patch withdrawn in the meantime.

-- 
Joel

Re: [RFA] GDB/624 - tbreak commands not executed when breakpoint hit

[Michael Snyder]

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

Joel Brobecker wrote:
> 
> Michael,
> 
> > Joel, this implementation, with its use of delete_at_next_stop,
> > seems clumsy and, to be frank, scary.  Instead, what if you just
> > made a temporary copy of the tbreak command-list (like you do for
> > GDB/622), then delete the breakpoint, and later execute the commands?
> 
> Thanks for your review. I need a bit of time to delve into this again,
> but I'll be quite busy in the next 2 or 3 weeks. You can consider this
> patch withdrawn in the meantime.

Aww, come on.  ;-)

OK, maybe it's fresher in my mind now than it is in yours.
How about this?

[-- Attachment #2: joel2.diff --]
[-- Type: text/plain, Size: 1060 bytes --]

*** breakpoint.joel1.c	Thu Aug 22 17:01:02 2002
--- breakpoint.c	Thu Aug 22 17:56:03 2002
*************** void
*** 6594,6604 ****
  breakpoint_auto_delete (bpstat bs)
  {
    struct breakpoint *b, *temp;
  
    for (; bs; bs = bs->next)
      if (bs->breakpoint_at && bs->breakpoint_at->disposition == disp_del
  	&& bs->stop)
!       delete_breakpoint (bs->breakpoint_at);
  
    ALL_BREAKPOINTS_SAFE (b, temp)
    {
--- 6594,6614 ----
  breakpoint_auto_delete (bpstat bs)
  {
    struct breakpoint *b, *temp;
+   struct command_line *cmd;
  
    for (; bs; bs = bs->next)
      if (bs->breakpoint_at && bs->breakpoint_at->disposition == disp_del
  	&& bs->stop)
!       {
! 	/* Deleting a temp breakpoint.  If it has commands associated
! 	   with it, preserve them in the in the bpstat list, so they
! 	   can be executed by bpstat_do_actions.  */
! 
! 	cmd = copy_command_lines (bs->commands);
! 	make_cleanup_free_command_lines (&cmd);
! 	delete_breakpoint (bs->breakpoint_at);
! 	bs->commands = cmd;
!       }
  
    ALL_BREAKPOINTS_SAFE (b, temp)
    {

Re: [RFA] GDB/624 - tbreak commands not executed when breakpoint hit

[Andrew Cagney]

> Joel Brobecker wrote:
> 
>> 
>> Michael,
>> 
> 
>> > Joel, this implementation, with its use of delete_at_next_stop,
>> > seems clumsy and, to be frank, scary.  Instead, what if you just
>> > made a temporary copy of the tbreak command-list (like you do for
>> > GDB/622), then delete the breakpoint, and later execute the commands?
> 
>> 
>> Thanks for your review. I need a bit of time to delve into this again,
>> but I'll be quite busy in the next 2 or 3 weeks. You can consider this
>> patch withdrawn in the meantime.
> 
> 
> Aww, come on.  ;-)
> 
> OK, maybe it's fresher in my mind now than it is in yours.
> How about this?
> 
> 
> 
> *** breakpoint.joel1.c	Thu Aug 22 17:01:02 2002
> --- breakpoint.c	Thu Aug 22 17:56:03 2002
> *************** void
> *** 6594,6604 ****
>   breakpoint_auto_delete (bpstat bs)
>   {
>     struct breakpoint *b, *temp;
>   
>     for (; bs; bs = bs->next)
>       if (bs->breakpoint_at && bs->breakpoint_at->disposition == disp_del
>   	&& bs->stop)
> !       delete_breakpoint (bs->breakpoint_at);
>   
>     ALL_BREAKPOINTS_SAFE (b, temp)
>     {
> --- 6594,6614 ----
>   breakpoint_auto_delete (bpstat bs)
>   {
>     struct breakpoint *b, *temp;
> +   struct command_line *cmd;
>   
>     for (; bs; bs = bs->next)
>       if (bs->breakpoint_at && bs->breakpoint_at->disposition == disp_del
>   	&& bs->stop)
> !       {
> ! 	/* Deleting a temp breakpoint.  If it has commands associated
> ! 	   with it, preserve them in the in the bpstat list, so they
> ! 	   can be executed by bpstat_do_actions.  */
> ! 
> ! 	cmd = copy_command_lines (bs->commands);
> ! 	make_cleanup_free_command_lines (&cmd);
> ! 	delete_breakpoint (bs->breakpoint_at);

Won't a discard cleanups be needed here?

> ! 	bs->commands = cmd;
> !       }
>   
>     ALL_BREAKPOINTS_SAFE (b, temp)
>     {