Re: [patch] Do not bpstat_clear_actions on throw_exception

[Pedro Alves <pedro at codesourcery dot com>]

On Friday 05 August 2011 21:14:17, Jan Kratochvil wrote:
> Hi,
> 
> as discussed here recently throw_exception does various unwised side effects.
> This patch is a pre-requisite for the entryval patchset as unavailable entry
> values is a normal caught silent exception there but it now stops any commands
> execution.
> 
> This function was there already during initial import of GDB to CVS.  I do not
> see its reason as any uncaught exception throws through bpstat_do_actions_1
> and thus terminates the bpstat evaluation anyway.
> 
> And it the exception is caught there it is wrong to stop the commands
> execution.

This isn't enough.  If you have a bpstat with only one breakpoint with
commands, and the error you tried was while running the breakpoint's
commands, you won't see a problem, because bpstat_do_actions_1
does bs->commands_left = NULL before executing the commands.

But, e.g., try something like: 

(gdb) b foo
(gdb) commands
> p 1
> p *0
> p 2
end

(gdb) b foo
(gdb) commands
> p 3
> p *0
> p 4
end

and run to foo.  After seeing the error, try any other
command, like "p 5".  You'll see the breakpoint commands
of the other breakpoint running...

Things get more complicated with any error that is thrown
from between bpstat_stop_status, and actually running the
breakpoint commands.  You'll also leave bs->commands_left
set then.

Maybe bs->commands_left shouldn't be set by
bpstat_stop_status at all.

> 
> There was also the reset of bpstat->commands but I do not find it related:
> 
> http://sourceware.org/ml/gdb-patches/2003-12/msg00350.html
> RFA: protect breakpoint commands from being freed
> http://sourceware.org/ml/gdb-cvs/2003-12/msg00136.html
> commit 79dbd81d5190cb413c7c44fdf00a858576f14e7e
> Author: Jim Blandy <jimb@codesourcery.com>
> Date:   Mon Dec 22 03:43:19 2003 +0000
> 
>     * breakpoint.c (bpstat_do_actions): To ensure that
>     clear_proceed_status doesn't free the command tree we're
>     evaluating out from under us, zero the bpstat's pointer to it, and
>     take care of freeing it ourselves.
>     * cli/cli-script.c (make_cleanup_free_command_lines): Make this
>     function externally visible.
>     * cli/cli-script.h (make_cleanup_free_command_lines): New
>     declaration.
> 
> No regressions on {x86_64,x86_64-m32,i686}-fedora16pre-linux-gnu.
> 
> 
> Thanks,
> Jan
> 
> 
> gdb/
> 2011-08-05  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* breakpoint.c (bpstat_clear_actions): Remove.
> 	* breakpoint.h (bpstat_clear_actions): Remove.
> 	* exceptions.c (throw_exception): Remove variable tp, its
> 	initialization and the call of bpstat_clear_actions.
> 
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -3186,23 +3186,6 @@ bpstat_num (bpstat *bsp, int *num)
>    return 1;
>  }
>  
> -/* Modify BS so that the actions will not be performed.  */
> -
> -void
> -bpstat_clear_actions (bpstat bs)
> -{
> -  for (; bs != NULL; bs = bs->next)
> -    {
> -      decref_counted_command_line (&bs->commands);
> -      bs->commands_left = NULL;
> -      if (bs->old_val != NULL)
> -	{
> -	  value_free (bs->old_val);
> -	  bs->old_val = NULL;
> -	}
> -    }
> -}
> -
>  /* Called when a command is about to proceed the inferior.  */
>  
>  static void
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -895,9 +895,6 @@ extern int bpstat_num (bpstat *, int *);
>     command loop).  */
>  extern void bpstat_do_actions (void);
>  
> -/* Modify BS so that the actions will not be performed.  */
> -extern void bpstat_clear_actions (bpstat);
> -
>  /* Implementation:  */
>  
>  /* Values used to tell the printing routine how to behave for this
> --- a/gdb/exceptions.c
> +++ b/gdb/exceptions.c
> @@ -207,22 +207,9 @@ exceptions_state_mc_action_iter_1 (void)
>  void
>  throw_exception (struct gdb_exception exception)
>  {
> -  struct thread_info *tp = NULL;
> -
>    quit_flag = 0;
>    immediate_quit = 0;
>  
> -  if (!ptid_equal (inferior_ptid, null_ptid))
> -    tp = find_thread_ptid (inferior_ptid);
> -
> -  /* Perhaps it would be cleaner to do this via the cleanup chain (not sure
> -     I can think of a reason why that is vital, though).  */
> -  if (tp != NULL)
> -    {
> -      /* Clear queued breakpoint commands.  */
> -      bpstat_clear_actions (tp->control.stop_bpstat);
> -    }
> -
>    do_cleanups (ALL_CLEANUPS);
>  
>    /* Jump to the containing catch_errors() call, communicating REASON
> 

-- 
Pedro Alves