From patchwork Wed Nov 6 20:51:41 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Maciej W. Rozycki" X-Patchwork-Id: 35702 Received: (qmail 131055 invoked by alias); 6 Nov 2019 20:51:52 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 130997 invoked by uid 89); 6 Nov 2019 20:51:48 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-8.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_2, GIT_PATCH_3, KAM_ASCII_DIVIDERS, KAM_SHORT autolearn=ham version=3.3.1 spammy=internal-error, threadc, unreliable, thread.c X-HELO: esa6.hgst.iphmx.com Received: from esa6.hgst.iphmx.com (HELO esa6.hgst.iphmx.com) (216.71.154.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 06 Nov 2019 20:51:46 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=wdc.com; i=@wdc.com; q=dns/txt; s=dkim.wdc.com; t=1573073506; x=1604609506; h=date:from:to:cc:subject:in-reply-to:message-id: references:mime-version; bh=vaBIGnvjvnNxB77g8tGixy/6KguYkM7u9Tr9coMghrs=; b=povrB70NFRG2UHu8RbGbW8Z5ge2R5kuEqudIhJW0GtNXCsl3ANuC3lNs KyKzCDqc3HVwcKEE95FenTSzbxaF58PaNyWbTchPliKV+jcSyPcsPhI4V j2oIB4jVd0xw9zAs+ezSLQvgzeu23rfyonDnFCtyG/S/COTNXb3HXo1XD LtsMO6f0cQcYfZVC1YTGvyzQXZHxX35+59Usiqa/vDfSXmZGRpY14+/+T QrfOkAq0qD70ozn5+zZKUKEvj+tgzRQTbbA2VAu/tvRd89cvoy8HLH+qH bEKN+vBXpxTMKmovc9USCPNY86chWJBPBL83duhrvH22EpTcLA7qAbPB4 A==; IronPort-SDR: B7UFr9A/CZ0EfS1HVqih8DUMx2Jhf3c4ho6y9HWVid45bJAi7XEg8OvnmWejz+9XLS0lC2E4gO 1F7IDuvtHMmsP65mY5ynxs+J61WNiptOpWdgjBUKwc3524GByAGe23ZPdNoQ0+rjZyvlkFuOxp fOC1wBJzegt3P4yrhdgRy/6l7Pw9I/q+CCrxFLt2MHgu238dEZnwpb2cf95CsY2FjidWhweAdJ 4ehKLrH1tZsfzgZVelMbUS1YEcU/0xBWzQEK2dshiJ2DfcxqTIHix6nz1Ddvi6dYkEpW6cBsIF fds= Received: from uls-op-cesaip02.wdc.com (HELO uls-op-cesaep02.wdc.com) ([199.255.45.15]) by ob1.hgst.iphmx.com with ESMTP; 07 Nov 2019 04:51:45 +0800 IronPort-SDR: 2KyPepD65J9nf3gHSA3Yovk1Vt0oxGWLDVBRFqsLSlNxr0SOK6rqjH7xgollM/wOsjMqJvMqEQ XkNDNALlmKP4xqdywwNqn89V55QO2wpyuW7JFwDvuT77YLTc81YRk9FN5fQ5Pm/y4Wia0ZGbTz oQSJ5pxB2HTm4GioHmMui3fjT/JeSMUADFZzBd9PL/NHPbpqU9ymmUA3vM9ASy1JyhO2mefE9D hZxQ39Acr4MQNrS5Q4IluMCpfZTfcWfczGwmAATb6Kxa2NioRKpSZQB7EW0+4KXdgcZwOYz2Qe YrP+lPm6VOof8QoS0IJhKyR0 Received: from uls-op-cesaip01.wdc.com ([10.248.3.36]) by uls-op-cesaep02.wdc.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Nov 2019 12:46:56 -0800 IronPort-SDR: 5+b92vD3q9KROf6Xd312AWdHSQFUYyzNQHp5qrZPIgcUKxyzu1elP9gJHMrFol7LYHorFI7cX/ aJL9gR1LKnAASteXyPAf5DBy/U0/CZ5JFDXWLZBYgRxOqtR25F35217uErKwTqPH8P6VZJcrtM qHbuv9Ks7Lo2Fihfc+ftTStCNSfIlGml93qQQjn1mnoqk7M9pjYrMxTiZoKHasUfyCn1jxwGjs eTHyVBULLcyuDiWk+Mgt1Uc2XstG6V3AJiDG7aIINosC4Nk1b6CNYrgoOSywBkl5XNwf58Bbhn ie4= WDCIronportException: Internal Received: from unknown (HELO redsun52) ([10.149.66.28]) by uls-op-cesaip01.wdc.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Nov 2019 12:51:43 -0800 Date: Wed, 6 Nov 2019 20:51:41 +0000 (GMT) From: "Maciej W. Rozycki" To: Pedro Alves , gdb-patches@sourceware.org cc: Jim Wilson Subject: [PATCH v2 1/4] Remove stale breakpoint step-over information In-Reply-To: Message-ID: References: User-Agent: Alpine 2.21 (LFD 202 2017-01-01) MIME-Version: 1.0 Fix an issue with the `step_over_info' structure introduced with commit 31e77af205cf ("PR breakpoints/7143 - Watchpoint does not trigger when first set"), where information stored in there is not invalidated in a remote debug scenario where software stepping in the all-stop mode is forcibly interrupted and the target connection then closed. As a result a subsequent target connection triggers an assertion failure on `ecs->event_thread->control.trap_expected' and cannot be successfully established, making the particular instance of GDB unusable. This was observed with a `riscv64-linux-gnu' cross-GDB and RISC-V/Linux `gdbserver' being developed and an attempt to step over the `ret' aka `c.jr ra' instruction where the value held in the `ra' aka `x1' register and therefore the address of a software-stepping breakpoint to insert is 0, as follows: (gdb) target remote 1.2.3.4:56789 Remote debugging using 1.2.3.4:56789 warning: while parsing target description (at line 4): Target description specified unknown architecture "riscv:rv64id" warning: Could not load XML target description; ignoring Reading symbols from .../sysroot/lib/ld-linux-riscv64-lp64d.so.1... 0x0000001555556ef0 in _start () from .../sysroot/lib/ld-linux-riscv64-lp64d.so.1 (gdb) break main Breakpoint 1 at 0x1643c (gdb) continue Continuing. Cannot access memory at address 0x0 (gdb) x /i $pc => 0x15555607b8 <__GI__dl_debug_state>: ret (gdb) print /x $ra $1 = 0x0 (gdb) stepi ^C^CInterrupted while waiting for the program. Give up waiting? (y or n) y Quit (gdb) kill Kill the program being debugged? (y or n) y [Inferior 1 (process 8964) killed] (gdb) target remote 1.2.3.4:56789 Remote debugging using 1.2.3.4:56789 warning: while parsing target description (at line 4): Target description specified unknown architecture "riscv:rv64id" warning: Could not load XML target description; ignoring .../gdb/infrun.c:5299: internal-error: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) y This is a bug, please report it. For instructions, see: . .../gdb/infrun.c:5299: internal-error: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Create a core file of GDB? (y or n) n Command aborted. (gdb) Correct the issue by making a call to clear global breakpoint step-over information from `clear_thread_inferior_resources'. To do so add an argument to `clear_step_over_info' giving the global thread number to check against, complementing one given to `set_step_over_info' when the information concerned is recorded, so that information is not removed for a thread that has stayed alive in a multi-target scenario. Adjust all the existing `clear_step_over_info' call sites accordingly where a step over has completed successfully and the thread that has hit the breakpoint is expected to be one for which the breakpoint has been inserted. gdb/ * infrun.h (clear_step_over_info): New prototype. * infrun.c (thread_is_stepping_over_breakpoint): Move earlier on. (clear_step_over_info): Use it. Make external. (resume_1, finish_step_over, handle_signal_stop) (keep_going_stepped_thread, keep_going_pass_signal): Adjust accordingly. * thread.c (clear_thread_inferior_resources): Call `clear_step_over_info'. --- Hi Pedro, On Thu, 31 Oct 2019, Pedro Alves wrote: > > Correct the issue by making a call to clear global breakpoint step-over > > information from `exit_inferior_1', which is where we already do all > > kinds of similar clean-ups, e.g. `delete_thread' called from there > > clears per-thread step-over information. > > This looks like a fragile place to clear this, considering > multiprocess. I.e., what if we're calling exit_inferior for some > inferior other than the one that was stepping. Good point. > The step over information is associated with a thread. > I think it'd be better to clear the step over information > when the corresponding thread is deleted. > > So something like adding a thread_info parameter to > clear_step_over_info, and then calling clear_step_over_info > from clear_thread_inferior_resources. clear_step_over_info > would only clear the info if the thread matched, or if NULL > is passed. Would that work? Thanks for your suggestion. That indeed works except that I have figured out we actually always have a thread to match available when making a call to `clear_step_over_info', so I have not implemented a special NULL case, and I'm not convinced matching thread numbers ahead of the call is worth an assertion either. OK to apply? Maciej Changes from v1: - Add and check against thread number argument to `clear_step_over_info'. - Call the function from `clear_thread_inferior_resources' rather than `exit_inferior_1'. - Use the thread number argument for `clear_step_over_info' throughout. --- gdb/infrun.c | 40 +++++++++++++++++++++------------------- gdb/infrun.h | 4 ++++ gdb/thread.c | 3 +++ 3 files changed, 28 insertions(+), 19 deletions(-) gdb-clear-step-over-info.diff Index: binutils-gdb/gdb/infrun.c =================================================================== --- binutils-gdb.orig/gdb/infrun.c +++ binutils-gdb/gdb/infrun.c @@ -1330,12 +1330,23 @@ set_step_over_info (const address_space step_over_info.thread = thread; } -/* Called when we're not longer stepping over a breakpoint / an - instruction, so all breakpoints are free to be (re)inserted. */ +/* See infrun.h. */ -static void -clear_step_over_info (void) +int +thread_is_stepping_over_breakpoint (int thread) +{ + return (step_over_info.thread != -1 + && thread == step_over_info.thread); +} + +/* See infrun.h. */ + +void +clear_step_over_info (int thread) { + if (!thread_is_stepping_over_breakpoint (thread)) + return; + if (debug_infrun) fprintf_unfiltered (gdb_stdlog, "infrun: clear_step_over_info\n"); @@ -1360,15 +1371,6 @@ stepping_past_instruction_at (struct add /* See infrun.h. */ int -thread_is_stepping_over_breakpoint (int thread) -{ - return (step_over_info.thread != -1 - && thread == step_over_info.thread); -} - -/* See infrun.h. */ - -int stepping_past_nonsteppable_watchpoint (void) { return step_over_info.nonsteppable_watchpoint_p; @@ -2339,7 +2341,7 @@ resume_1 (enum gdb_signal sig) "infrun: resume: skipping permanent breakpoint, " "deliver signal first\n"); - clear_step_over_info (); + clear_step_over_info (tp->global_num); tp->control.trap_expected = 0; if (tp->control.step_resume_breakpoint == NULL) @@ -2496,7 +2498,7 @@ resume_1 (enum gdb_signal sig) delete_single_step_breakpoints (tp); - clear_step_over_info (); + clear_step_over_info (tp->global_num); tp->control.trap_expected = 0; insert_breakpoints (); @@ -5298,7 +5300,7 @@ finish_step_over (struct execution_contr back an event. */ gdb_assert (ecs->event_thread->control.trap_expected); - clear_step_over_info (); + clear_step_over_info (ecs->event_thread->global_num); } if (!target_is_non_stop_p ()) @@ -5913,7 +5915,7 @@ handle_signal_stop (struct execution_con "infrun: signal may take us out of " "single-step range\n"); - clear_step_over_info (); + clear_step_over_info (ecs->event_thread->global_num); insert_hp_step_resume_breakpoint_at_frame (frame); ecs->event_thread->step_after_step_resume_breakpoint = 1; /* Reset trap_expected to ensure breakpoints are re-inserted. */ @@ -7004,7 +7006,7 @@ keep_going_stepped_thread (struct thread breakpoint, otherwise if we were previously trying to step over this exact address in another thread, the breakpoint is skipped. */ - clear_step_over_info (); + clear_step_over_info (tp->global_num); tp->control.trap_expected = 0; insert_single_step_breakpoint (get_frame_arch (frame), @@ -7547,7 +7549,7 @@ keep_going_pass_signal (struct execution { exception_print (gdb_stderr, e); stop_waiting (ecs); - clear_step_over_info (); + clear_step_over_info (ecs->event_thread->global_num); return; } Index: binutils-gdb/gdb/infrun.h =================================================================== --- binutils-gdb.orig/gdb/infrun.h +++ binutils-gdb/gdb/infrun.h @@ -120,6 +120,10 @@ extern void insert_step_resume_breakpoin struct symtab_and_line , struct frame_id); +/* Called when we're not longer stepping over a breakpoint / an + instruction, so all breakpoints are free to be (re)inserted. */ +void clear_step_over_info (int thread); + /* Returns true if we're trying to step past the instruction at ADDRESS in ASPACE. */ extern int stepping_past_instruction_at (struct address_space *aspace, Index: binutils-gdb/gdb/thread.c =================================================================== --- binutils-gdb.orig/gdb/thread.c +++ binutils-gdb/gdb/thread.c @@ -191,6 +191,9 @@ clear_thread_inferior_resources (struct delete_longjmp_breakpoint_at_next_stop (tp->global_num); + /* Remove any stale breakpoint step-over information. */ + clear_step_over_info (tp->global_num); + bpstat_clear (&tp->control.stop_bpstat); btrace_teardown (tp);