From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x42f.google.com (mail-pf1-x42f.google.com [IPv6:2607:f8b0:4864:20::42f]) by sourceware.org (Postfix) with ESMTPS id 7D9B7385AC19 for ; Tue, 20 Jul 2021 15:06:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7D9B7385AC19 Received: by mail-pf1-x42f.google.com with SMTP id p36so19791489pfw.11 for ; Tue, 20 Jul 2021 08:06:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=oRhFN24JVhUv4G/glgsD/Q/9+GZfN+JWKkmv7oB66zo=; b=Nkwv8YPqyR/isovxabox1jRcg0J6CC/3e4XG8MbahQhkg9d3DK0lxlT5dhy0ofDwyX iyulPnA9i84Eyks7Ybss2lhnHgp3cTdwsnayh5dIfmtE0BcGc12NCNKqZXpCIzx14eTo B3UkNUgNQuUxZUWCc5i8AvCC0hHTMHBt1wVm5Y2JlsFAW/SFvfHRJnr7uFXMJh0Ow0w/ 6s7v2EXDTUtrI/aWvKkU8O3ZBscIJBBycYPax/F/k5zFzR8hmrYj4PpbSwi/Q5vCrnAs KHgwWbHDVT6wnFur2t2HW34XYKQ6831TcB6eg6TsYc4t/NttMtScp3od4jb0KK1yMgKp q77w== X-Gm-Message-State: AOAM5312OKXuLem1EynpNSx5roOgpg5O4D9yWOBhLqPPi2Jy9vWucYj4 LU81B6rCFqNFiFB9cB+xnF2v8g== X-Google-Smtp-Source: ABdhPJzFw23WbL/Skf7Sn6XL26VzafIW897X4ILmbsKomoheaf1IVURwZ3mqUy7OP4dYwvuyG7X8gQ== X-Received: by 2002:a63:4765:: with SMTP id w37mr31685847pgk.400.1626793565552; Tue, 20 Jul 2021 08:06:05 -0700 (PDT) Received: from ?IPv6:2804:7f0:4841:2b39:357f:5f9a:1dce:6fcc? ([2804:7f0:4841:2b39:357f:5f9a:1dce:6fcc]) by smtp.gmail.com with ESMTPSA id q9sm14138630pgt.65.2021.07.20.08.06.03 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 20 Jul 2021 08:06:05 -0700 (PDT) Subject: Re: [PATCH 3/3] Fix reverse stepping multiple contiguous PC ranges To: Simon Marchi , gdb-patches@sourceware.org, Carl Love References: <20210201191930.2696351-1-luis.machado@linaro.org> <5791c5b6-face-f7ad-2a41-5de570619895@polymtl.ca> From: Luis Machado Message-ID: Date: Tue, 20 Jul 2021 12:06:01 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <5791c5b6-face-f7ad-2a41-5de570619895@polymtl.ca> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Jul 2021 15:06:08 -0000 cc-ing Carl. On 7/12/21 11:53 PM, Simon Marchi wrote: > On 2021-02-01 2:19 p.m., Luis Machado via Gdb-patches wrote: >> When running GDB's testsuite on aarch64-linux/Ubuntu 20.04, I noticed some >> failures in gdb.reverse/solib-precsave.exp and gdb.reverse/solib-reverse.exp. >> >> The failure happens around the following code: >> >> 38 b[1] = shr2(17); /* middle part two */ >> 40 b[0] = 6; b[1] = 9; /* generic statement, end part two */ >> 42 shr1 ("message 1\n"); /* shr1 one */ >> >> Normal execution: >> >> - step from line 1 will land on line 2. >> - step from line 2 will land on line 3. >> >> Reverse execution: >> >> - step from line 3 will land on line 2. >> - step from line 2 will land on line 2. >> - step from line 2 will land on line 1. >> >> The problem here is that line 40 contains two contiguous but distinct >> PC ranges, like so: >> >> Line 40 - [0x7ec ~ 0x7f4] >> Line 40 - [0x7f4 ~ 0x7fc] >> >> When stepping forward from line 2, we skip both of these ranges and land on >> line 42. When stepping backward from line 3, we stop at the start PC of the >> second (or first, going backwards) range of line 40. >> >> This happens because we have this check in infrun.c:process_event_stop_test: >> >> /* When stepping backward, stop at beginning of line range >> (unless it's the function entry point, in which case >> keep going back to the call point). */ >> CORE_ADDR stop_pc = ecs->event_thread->suspend.stop_pc; >> if (stop_pc == ecs->event_thread->control.step_range_start >> && stop_pc != ecs->stop_func_start >> && execution_direction == EXEC_REVERSE) >> end_stepping_range (ecs); >> else >> keep_going (ecs); >> >> Since we've reached ecs->event_thread->control.step_range_start, we stop >> stepping backwards. >> >> The right thing to do is to look for adjacent PC ranges for the same line, >> until we notice a line change. Then we take that as the start PC of the >> range. >> >> Another solution I thought about is to merge the contiguous ranges when >> we are reading the line tables. Though I'm not sure if we really want to process >> that data as opposed to keeping it as the compiler created, and then working >> around that. >> >> In any case, the following patch addresses this problem. >> >> I'm not particularly happy with how we go back in the ranges (using "pc - 1"). >> Feedback would be welcome. > > I don't have a deep knowledge of this area, so I'd like to know, how > does it work for forward stepping over the example you gave above? What > is the sequence of decisions that lead to GDB deciding to continue > stepping more? Because I am thinking that what we do for reverse > stepping should be pretty much the same as what we do for forward > stepping, just the other way around. I'll have to take a look at it again, sorry. It's been a while since I touched this and I only have a vague recollection of what the exact problem was. But the short answer is "no", we don't handle forward execution in the exact same way as forward execution. There are corner cases. I think it had something to do with how the line ranges are represented. The start address of a PC range is always contained in the range, but the ending address is not. When you move forwards/backwards, that makes a difference. > >> Validated on aarch64-linux/Ubuntu 20.04/18.04. >> >> Ubuntu 18.04 doesn't actually run into these failures because the compiler >> doesn't generate distinct PC ranges for the same line. > > Could it be that the new compiler adds column information? That would > explain why it's now two ranges. Probably. > >> gdb/ChangeLog: >> >> YYYY-MM-DD Luis Machado >> >> * infrun.c (process_event_stop_test): Handle backward stepping >> across multiple ranges for the same line. >> * symtab.c (find_line_range_start): New function. >> * symtab.h (find_line_range_start): New prototype. >> --- >> gdb/infrun.c | 22 +++++++++++++++++++++- >> gdb/symtab.c | 35 +++++++++++++++++++++++++++++++++++ >> gdb/symtab.h | 16 ++++++++++++++++ >> 3 files changed, 72 insertions(+), 1 deletion(-) >> >> diff --git a/gdb/infrun.c b/gdb/infrun.c >> index e070eff33d..5bb268529d 100644 >> --- a/gdb/infrun.c >> +++ b/gdb/infrun.c >> @@ -6534,11 +6534,31 @@ process_event_stop_test (struct execution_control_state *ecs) >> have software watchpoints). */ >> ecs->event_thread->control.may_range_step = 1; >> >> + /* When we are stepping inside a particular line range, in reverse, >> + and we are sitting at the first address of that range, we need to >> + check if this address also shows up in another line range as the >> + end address. >> + >> + If so, we need to check what line such a step range points to. >> + If it points to the same line as the current step range, that >> + means we need to keep going in order to reach the first address >> + of the line range. We repeat this until we eventually get to the >> + first address of a particular line we're stepping through. */ >> + CORE_ADDR range_start = ecs->event_thread->control.step_range_start; >> + if (execution_direction == EXEC_REVERSE) >> + { >> + gdb::optional real_range_start >> + = find_line_range_start (ecs->event_thread->suspend.stop_pc); > > With what I just merged, you'll find that you need to replace > ->suspend.stop_pc with `->stop_pc ()`. Nothing major. > >> + >> + if (real_range_start.has_value ()) >> + range_start = *real_range_start; > > The question I have while reading this is: what did set the original > range_start, ecs->event_thread->control.step_range_start? Why didn't we > get the "real" range start at the time and have to fix it up now? > That's the bit I don't remember exactly. But GDB has no notion of lines with multiple PC ranges. It works by (from what I vaguely recall) skipping ranges based on the line number, start address and end address of a range. And in the case of backwards execution, it doesn't know how to figure out the line range is not over yet. >> + } >> + >> /* When stepping backward, stop at beginning of line range >> (unless it's the function entry point, in which case >> keep going back to the call point). */ >> CORE_ADDR stop_pc = ecs->event_thread->suspend.stop_pc; >> - if (stop_pc == ecs->event_thread->control.step_range_start >> + if (stop_pc == range_start >> && stop_pc != ecs->stop_func_start >> && execution_direction == EXEC_REVERSE) >> end_stepping_range (ecs); >> diff --git a/gdb/symtab.c b/gdb/symtab.c >> index 7ffb52a943..e8f5301951 100644 >> --- a/gdb/symtab.c >> +++ b/gdb/symtab.c >> @@ -3382,6 +3382,41 @@ find_pc_line (CORE_ADDR pc, int notcurrent) >> return sal; >> } >> >> +/* See symtah.h. */ >> + >> +gdb::optional >> +find_line_range_start (CORE_ADDR pc) >> +{ >> + struct symtab_and_line current_sal = find_pc_line (pc, 0); >> + >> + if (current_sal.line == 0) >> + return {}; >> + >> + struct symtab_and_line prev_sal = find_pc_line (current_sal.pc - 1, 0); >> + >> + /* If the previous entry is for a different line, that means we are already >> + at the entry with the start PC for this line. */ >> + if (prev_sal.line != current_sal.line) >> + return current_sal.pc; >> + >> + /* Otherwise, keep looking for entries for the same line but with >> + smaller PC's. */ >> + bool done = false; >> + CORE_ADDR prev_pc; >> + while (!done) >> + { >> + prev_pc = prev_sal.pc; >> + >> + prev_sal = find_pc_line (prev_pc - 1, 0); >> + >> + /* Did we notice a line change? If so, we are done with the search. */ >> + if (prev_sal.line != current_sal.line) >> + done = true; >> + } >> + >> + return prev_pc; > > Above, should we compare more than just the line? > > What I am thinking is the edge case where looking up PC would return an > SAL, say `a.c:40`, and looking up "PC - 1" would return `b.c:40`. The > algorithm would think that we are still on the same line and keep > searching, when in fact it's a different line 40, so we should have > stopped. Yeah, that sounds worth covering in the code. > > > Simon >