From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7463 invoked by alias); 26 Aug 2011 11:45:52 -0000 Received: (qmail 7453 invoked by uid 22791); 26 Aug 2011 11:45:50 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,TW_GJ,TW_TJ X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 26 Aug 2011 11:45:36 +0000 Received: (qmail 18874 invoked from network); 26 Aug 2011 11:45:35 -0000 Received: from unknown (HELO scottsdale.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 26 Aug 2011 11:45:35 -0000 From: Pedro Alves To: Jan Kratochvil Subject: Re: [patch 2/2] Do not bpstat_clear_actions on throw_exception #3 Date: Fri, 26 Aug 2011 11:45:00 -0000 User-Agent: KMail/1.13.6 (Linux/2.6.38-11-generic; KDE/4.7.0; x86_64; ; ) Cc: gdb-patches@sourceware.org References: <20110822145150.GB11817@host1.jankratochvil.net> <201108241119.03993.pedro@codesourcery.com> <20110826101359.GA27121@host1.jankratochvil.net> In-Reply-To: <20110826101359.GA27121@host1.jankratochvil.net> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201108261245.32822.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-08/txt/msg00484.txt.bz2 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 "". 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 "", 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 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: > > I will try to code it. > > > Thanks, > Jan > > > cat >cmd < 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