Re: [patch 2/2] Do not bpstat_clear_actions on throw_exception #3

[Pedro Alves <pedro at codesourcery dot com>]

On Friday 26 August 2011 11:13:59, Jan Kratochvil wrote:
> On Wed, 24 Aug 2011 12:19:03 +0200, Pedro Alves wrote:
> > On Tuesday 23 August 2011 21:32:58, Jan Kratochvil wrote:
> > > Testing is more difficult, going to post now patch 3/2 but in fact the real
> > > continuation is not tested there as the testcase gets caught by
> > > execute_command.  I haven't done a proper testcase for the async mode.
> > 
> > I think a hook-stop that errors would be sufficient to leave
> > breakpoint commands dangling (normal_stop is called after
> > handle_inferior_event, from fetch_inferior_event)
> 
> The problem is from hook-stop one runs execute_command which already catches
> the error and does bpstat_clear_actions on its own.
> 
> Just FYI, it would be better but not everything needs to have a testcase.

I agree.

> I started this thread to support entry-values-not-available implemented via an
> exception.  It may even have been inspired by your NOT_AVAILABLE_ERROR.
> 
> When evaluating all TRY_CATCH cases where should be done bpstat_clear_actions
> and where not I have found it is very subjective.  gdb_target_find_new_threads
> for example may catch exception due to corrupted thread list when analysing
> a core file, therefore it currently does bpstat_clear_actions.  But maybe it
> was expected the thread list is corrupted (it may have been the reason a core
> file got dumped).

Yes, the fact that that does bpstat_clear_actions is a bug, because
whatever error happened was handled, and the command the user
entered was not cancelled.  At the CLI level, the internal handling
of the error is an implementation detail of the command --- you
could instead reimplement gdb_target_find_new_threads to not
catch errors, but instead pass return error codes around, avoiding
any throw, but ending up behaving the same, that is, not consider
a corrupt thread list a fatal error.

I think that canceptually, what really matters for this patch is:

   (1) - some execution command is invoked at the cli prompt (e.g, "next").
   (2) - thread stops for breakpoint, and breakpoint has commands
   (3) - breakpoint commands are ran
   (4) - user enters some other command at the prompt

What we want is that the commands in 2-3 are either fully cancelled
or fully ran by the time we get to 4.  If some specific command should
handle errors itself gracefully or throw an error back at the interpreter
doesn't matter here --- the fact remains that some commands will
throw errors, others handle them gracefully.

Implementation wise, the fact that it is currently throw_exception that
clears the bpstat actions list is simply because #1 thread->stop_stat was
once a global, and, #2 gdb didn't use to have the exception scheme we
have nowadays, we only had a single setjmp at the top level, and
everywhere there was an error, we'd longjmp to the top level.  But nowadays
we can have errors that are caught by intermediate layers, and handled
there, so those should not cause the breakpoint command list to be
cancelled, because the error was handled before escaping out of the
command, making it non-fatal for the command's life.  Internal
NOT_AVAILABLE_ERROR handling by the printing layer is one such
example.  Another would be, say, MEMORY_ERRORs handled by e.g., 
"watch *0" (the watch command succeeds, so a command list
should move on to the next command).  Yet another would be the
general errors caught when printing values, that translate the errors
to "<error reading variable: %s>".  All these intermediate error
cases are breaking breakpoint command lists.
We're not terribly consistent in the printing stuff --- in some cases
we end up showing that "<error reading variable: %s>", while in
other cases we let the error escape, like in "p *0".

> bpstat_clear_actions is not so strong as script_from_file abort 
> but I find it similar and maybe confusing their conditions are not the same.  I find wrong
> that `gdb -nx -x ./cmd' stops on first error 

I'd find it wrong that a command sequence continues
blindly ignoring previous errors by default.

I think the neatest fix would be to add some try/catch/finaly
syntax to the cli.  There was a patch for that posted eons ago:

http://sourceware.org/ml/gdb-patches/2001-12/msg00449.html

I don't know what happened to it.  (Cagney's reply raises
some questions that would apply to a global continue-on-errors
flag equally.)

> as I have to run `gdb -nx <cmd'
> in such cases while that mode of run has other kinds of disadvantages.
> 
> Even if this patch gets fixed the way we were leading to it would still make
> changes in GDB behavior as GDB would much less call bpstat_clear_actions.

I think most if not all those changes are actually bug fixes.

> The tracepoint example below on FSF GDB HEAD does:
> 
> Found trace frame 0, tracepoint 4
> #0  globals_test_func () at ./gdb.trace/unavailable.cc:319
> Backtrace stopped: Not enough registers or memory available to unwind further
> No longer looking at any trace frame
> (gdb) _
> 
> while it does not print "still-executing" which is IMO a bug, do you agree?

I agree it's a bug.  The backtrace stopped gracefully, and completed,
it didn't throw any error back to the interpreter.

> If not printing "still-executing" is really a bug proposing to forget about
> the current patch and instead to:

But we're so close to having this fixed!  :-(  Did you find some
legitimate use case the patch breaks?

> (a) throw_exception will call bpstat_clear_actions only if exception.error is
>     not NOT_AVAILABLE_ERROR (adding later my new ENTRY_VALUE_NOT_AVAILABLE).

I don't like this at all.  I've given examples above how it's not
really just NOT_AVAILABLE_ERROR that matters.

> (b) bpstat_clear_actions should also abort script_from_file.

Hmm?

> (c) There should be a new setting "set error-stops-script" with default "on"
>     (backward compatibility) where "off" would disable bpstat_clear_actions
>     completely.  And I will happily use "set error-stops-script off" in my
>     ~/.gdbinit so that I can finally run `gdb -nx -x ./transcript.1'.

Some patch for something like that has been posted before too:

 <http://sourceware.org/ml/gdb-patches/2005-08/msg00120.html>

> 
> I will try to code it.
> 
> 
> Thanks,
> Jan
> 
> 
> cat >cmd <<EOH
> file gdb.trace/unavailable
> target remote localhost:1234
> break begin
> break end
> commands
>   tstop
>   tfind start
>   bt
>   tfind none
> end
> break end
> commands
>   echo still-executing\n
> end
> trace 319
> actions
> collect a
> end
> continue
> tstart
> continue
> EOH
> 
> ./gdbserver --once :1234 ../testsuite/gdb.trace/unavailable
> ../gdb -nx -x cmd 
> 
> 

-- 
Pedro Alves