From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 101882 invoked by alias); 19 Feb 2020 11:26:15 -0000 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 Received: (qmail 101871 invoked by uid 89); 19 Feb 2020 11:26:13 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-12.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=thread.c, threadc, UD:thread.c, Inferior X-HELO: mail-qk1-f195.google.com Received: from mail-qk1-f195.google.com (HELO mail-qk1-f195.google.com) (209.85.222.195) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 19 Feb 2020 11:26:10 +0000 Received: by mail-qk1-f195.google.com with SMTP id a141so12756969qkg.6 for ; Wed, 19 Feb 2020 03:26:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=7nGD5/2sv16Qq0gtMde5d3/ldnYPtru1zdVRyU3Vr34=; b=bWPwMryLON8Zme5/aoc6YE7lf8nvr0yo8c2vzyoulH9yk3GhoecxUHXA1TLAaealvZ 0ZzWkHjCi4iCoObPbpVk2gSB0EseFeg7xXlv/Ev9WRyqZJLVngqiHU0GytAVJxnTAp++ QouyO9hcTjkaE6S4g6maTulpEmcO7LNWNpwtiIkLaDS1CeRhv29bOUtYYuvgoix3c5rF y61uaBCK7wScs7mgH3NUhuTEMo2AJ+InfvDANTM4te6tVuqWM0zKGctl3xO/NkzcWDmF VGHzYtYGm/Mdwnt3B6GrGjFAAOeexZptrkl+lF48/zjL4FLL3WF8Dkle13vWb8UuNaAE FDpA== Return-Path: Received: from [192.168.0.185] ([191.34.219.95]) by smtp.gmail.com with ESMTPSA id w53sm789214qtb.91.2020.02.19.03.26.05 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 19 Feb 2020 03:26:07 -0800 (PST) Subject: Re: [PATCH v2 1/4] Remove stale breakpoint step-over information To: "Maciej W. Rozycki" , Pedro Alves , gdb-patches@sourceware.org Cc: Jim Wilson References: From: Luis Machado Message-ID: <3f707cde-ec92-d011-1793-3c017018fa33@linaro.org> Date: Wed, 19 Feb 2020 11:26:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2020-02/txt/msg00767.txt.bz2 On 11/6/19 5:51 PM, Maciej W. Rozycki wrote: > 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; > + Since the thread number is only used to check if a particular thread is stepping over a breakpoint, wouldn't it be better to move the check to thread.c as opposed to embedding it in clear_step_over_info and having to change all of its callers? > 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); > + So something like... /* If this thread has a pending step-over request, clear it now. */ if (thread_is_stepping_over_breakpoint (tp->global_num)) clear_step_over_info (); > bpstat_clear (&tp->control.stop_bpstat); > > btrace_teardown (tp); > Otherwise it looks OK to me, assuming the testsuite has executed properly against the thread tests.