From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6359 invoked by alias); 18 Aug 2011 15:34:45 -0000 Received: (qmail 5723 invoked by uid 22791); 18 Aug 2011 15:34:44 -0000 X-SWARE-Spam-Status: No, hits=-7.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 18 Aug 2011 15:34:26 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p7IFYIbZ014593 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 18 Aug 2011 11:34:18 -0400 Received: from host1.jankratochvil.net (ovpn-116-17.ams2.redhat.com [10.36.116.17]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p7IFYFfL010739 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 18 Aug 2011 11:34:17 -0400 Received: from host1.jankratochvil.net (localhost [127.0.0.1]) by host1.jankratochvil.net (8.14.4/8.14.4) with ESMTP id p7IFYEVR024100; Thu, 18 Aug 2011 17:34:14 +0200 Received: (from jkratoch@localhost) by host1.jankratochvil.net (8.14.4/8.14.4/Submit) id p7IFYDPW024099; Thu, 18 Aug 2011 17:34:13 +0200 Date: Thu, 18 Aug 2011 15:34:00 -0000 From: Jan Kratochvil To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [patch] Do not bpstat_clear_actions on throw_exception Message-ID: <20110818153413.GA23189@host1.jankratochvil.net> References: <20110805201417.GA23405@host1.jankratochvil.net> <201108081644.08465.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201108081644.08465.pedro@codesourcery.com> User-Agent: Mutt/1.5.21 (2010-09-15) 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/msg00355.txt.bz2 On Mon, 08 Aug 2011 17:44:08 +0200, Pedro Alves wrote: > 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... Thank you very much, I agree now, created a testcase for it. > 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. The code path execute_command...bpstat_do_actions is multiple times in GDB (CLI, MI, bpstat_do_actions from continuations etc.). The path execute_command...bpstat_do_actions is currently unprotected by any TRY_CATCH, therefore I see the only possibility to clear commands_left _before_ execute_command. One could protect execute_command...bpstat_do_actions but that is present at multiple places and I find the patch below also OK. I rather cleared commands/commands_left before starting a new command as it seems easily unified in GDB. There is also normal_stop but that is before the commands get executed at all. > Maybe bs->commands_left shouldn't be set by > bpstat_stop_status at all. It already needs to check for the "silent" statement. Besides it if "commands" are fetched in bpstat_do_actions_1 the unwanted execution you have shown to me can also happen. Or could you describe more the idea you had? No regressions on {x86_64,x86_64-m32,i686}-fedora16pre-linux-gnu. Thanks, Jan gdb/ 2011-08-18 Jan Kratochvil * exceptions.c (throw_exception): Move variable tp, its initialization and the call of bpstat_clear_actions to ... * top.c (prepare_execute_command): ... here. gdb/testsuite/ 2011-08-18 Jan Kratochvil * gdb.base/commands.exp (error_clears_commands_left): New function. (): Call it. --- 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 --- a/gdb/top.c +++ b/gdb/top.c @@ -348,6 +348,7 @@ prepare_execute_command (void) { struct value *mark; struct cleanup *cleanup; + struct thread_info *tp = NULL; mark = value_mark (); cleanup = make_cleanup_value_free_to_mark (mark); @@ -359,6 +360,17 @@ prepare_execute_command (void) if (non_stop) target_dcache_invalidate (); + 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); + } + return cleanup; } --- a/gdb/testsuite/gdb.base/commands.exp +++ b/gdb/testsuite/gdb.base/commands.exp @@ -678,6 +678,61 @@ proc if_commands_test {} { } } +# Verify an error during "commands" commands execution will prevent any other +# "commands" from other breakpoints at the same location to be executed. + +proc error_clears_commands_left {} { + delete_breakpoints + + gdb_breakpoint "main" + + set test "main commands 1" + gdb_test_multiple {commands $bpnum} $test { + -re "End with a line saying just \"end\"\\.\r\n>$" { + pass $test + } + } + set test "main commands 1a" + gdb_test_multiple {echo cmd1\n} $test { + -re "\r\n>$" { + pass $test + } + } + set test "main commands 1b" + gdb_test_multiple {errorcommandxy\n} $test { + -re "\r\n>$" { + pass $test + } + } + gdb_test_no_output "end" "main commands 1c" + + gdb_breakpoint "main" + set test "main commands 2" + gdb_test_multiple {commands $bpnum} $test { + -re "End with a line saying just \"end\"\\.\r\n>$" { + pass $test + } + } + set test "main commands 2a" + gdb_test_multiple {echo cmd2\n} $test { + -re "\r\n>$" { + pass $test + } + } + set test "main commands 2b" + gdb_test_multiple {errorcommandyz\n} $test { + -re "\r\n>$" { + pass $test + } + } + gdb_test_no_output "end" "main commands 2c" + + gdb_run_cmd + gdb_test "" "cmd1\r\nUndefined command: \"errorcommandxy\"\\. Try \"help\"\\." "cmd1 error" + + gdb_test {echo idle\n} "\r\nidle" "no cmd2" +} + proc redefine_hook_test {} { global gdb_prompt @@ -758,6 +813,7 @@ stray_arg0_test source_file_with_indented_comment recursive_source_test if_commands_test +error_clears_commands_left redefine_hook_test # This one should come last, as it redefines "backtrace". redefine_backtrace_test