From patchwork Sat Jun 22 11:05:58 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 33278 Received: (qmail 111934 invoked by alias); 22 Jun 2019 11:06:04 -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 111926 invoked by uid 89); 22 Jun 2019 11:06:04 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=deny, enquiry, belief, love X-HELO: mail-wm1-f65.google.com Received: from mail-wm1-f65.google.com (HELO mail-wm1-f65.google.com) (209.85.128.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 22 Jun 2019 11:06:02 +0000 Received: by mail-wm1-f65.google.com with SMTP id c6so8918395wml.0 for ; Sat, 22 Jun 2019 04:06:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=yWH/ZmIXSBu+Xz9WRI2rH+2ChXuQgS79z/gzLBlb3Pw=; b=HF4VvH9QTgmeXkdHfvMMFOKKzpLPk/MSvmgm67GcD351jsaWnn+W1SgW07H2KnEwXK EiM1g5tLCndvt2O3xzMXgW1KwFjmlrJ3zUQrMqdYRjnPSdEUqwX+bbpMqUwKZEixiH8D giRY0NmCWMVadkj/1fyUlFfvcuqx3cbDZLPMdFqtP5GC3cRf/hXFAgGt44prJfvXwAVG bRN02cKi0FPur9DtnzRNJyFPyunYTz/dJg7DqEPYL218dKvqiUAejuxsTbp8r2XiEt99 doKoJJcLkEHz780mQ4f0Tkjf8mXlWH2jRar0NcD6PZCBgPJRu297c3tydAZnPg5uI6UN Wo5g== Return-Path: Received: from localhost (host86-180-62-212.range86-180.btcentralplus.com. [86.180.62.212]) by smtp.gmail.com with ESMTPSA id 35sm5774973wrj.87.2019.06.22.04.05.59 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 22 Jun 2019 04:05:59 -0700 (PDT) Date: Sat, 22 Jun 2019 12:05:58 +0100 From: Andrew Burgess To: Pedro Alves Cc: Kevin Buettner , gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler Message-ID: <20190622110558.GK23204@embecosm.com> References: <20190612123403.14348-1-andrew.burgess@embecosm.com> <20190619181147.69974f43@f29-4.lan> <20190620205759.GI23204@embecosm.com> <20190620232314.GJ23204@embecosm.com> <406d910b-8b63-1e93-d340-7e9ab841ad0b@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <406d910b-8b63-1e93-d340-7e9ab841ad0b@redhat.com> X-Fortune: Your own mileage may vary. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes * Pedro Alves [2019-06-21 14:43:26 +0100]: > On 6/21/19 12:23 AM, Andrew Burgess wrote: > > > I spent some more time trying to find a path that would call both > > 'decode_digits_list_mode' and then 'skip_prologue_sal', but I still > > can't find one. > > But won't that change affect any code path that ends up in > skip_prologue_sal with explicit_line set? [ Disclaimer: In the below I'll take about 'in current testing we never do X'. I understand that this doesn't mean GDB will _never_ do X as our testing doesn't guarantee to hit every possible code path, it's more an invitation for people to suggest how me might create a test that does do X. ] Indeed. My claim is that in the current testing we never get to skip_prologue_sal with explicit_line set. My patch means we do now enter skip_prologue_sal with explicit_line set, and I find that the existing check that uses explicit_line means GDB doesn't behave as I'd like. Given that in HEAD explicit_line only seems to be set when decoding a line spec in 'list_mode', my current belief is that explicit_line is never set in a condition where the flag will then be checked. In other words, I think the explicit_line is currently useless. > > E.g.: > > /* Helper function for break_command_1 and disassemble_command. */ > > void > resolve_sal_pc (struct symtab_and_line *sal) > { > CORE_ADDR pc; > > if (sal->pc == 0 && sal->symtab != NULL) > { > if (!find_line_pc (sal->symtab, sal->line, &pc)) > error (_("No line %d in file \"%s\"."), > sal->line, symtab_to_filename_for_display (sal->symtab)); > sal->pc = pc; > > /* If this SAL corresponds to a breakpoint inserted using a line > number, then skip the function prologue if necessary. */ > if (sal->explicit_line) > skip_prologue_sal (sal); > } > > Is that path unreachable today? In current testing we enter this code block once, by accident. The test 'gdb.dwarf2/dw2-objfile-overlap.exp' enters the block because we load a symbol file at address 0. The check '(sal->pc == 0 && sal->symtab != NULL)' is trying to find SALs where the 'pc' has not been set, in our case the 'pc' has been set; to zero. When we then call 'find_line_pc' and then 'sal->pc = pc', we reset the 'pc' to zero again. In this one case the explicit_line flag is false, so skip_prologue_sal is never called. As an aside how would you feel about a patch that made the 'pc' field of symtab_and_line private, and updated all users to use getter/setter methods? I already did this in order to add a 'is_pc_initialised?' type method to symtab_and_line. When I add this and change the above code to say this: void resolve_sal_pc (struct symtab_and_line *sal) { CORE_ADDR pc; if (sal->symtab != NULL && !sal->pc_p ()) { // ... etc ... then we no longer enter this block at all during the current testing. > > > > > > Looking back at how the explicit_line flag was originally used when > > it was added in commit ed0616c6b78a0966, things have changed quite a > > bit in the 10+ years since. There were some tests added along with > > the explicit_line flag (gdb.cp/mb-*.exp) and these all pass both in > > master and in my patched branch. > > > > My current thinking is that the explicit_line flag was no longer doing > > anything useful in master, but if someone disagrees I'd love to > > understand more about this. > > I seem to recall that GDB didn't use to update a breakpoint's line > number to advance to the next line number that includes some actual > compiled code. Like if you set a breakpoint at line 10 below: > > 10 // just a comment > 11 i++; > > you end up with a breakpoint at line 11. Maybe it's old code > related to that. I wonder if what you meant to say here is the breakpoint is placed at the address of line 11, but is recorded as being at line 10. This actually would line up with what the explicit line flag was doing if the explicit line flag was being set. The problem seems to be that when the explicit_line flag was first added there was just function for decoding linespec line numbers 'decode_all_digits'. At some point in time this split into decode_digits_ordinary and decode_digits_list_mode, when this happened the explicit_line flag was only ever being set in one path. I suspect that if the behaviour you discussed above ever existed, then it was before the split in how digits were decoded. I'm running out of time to investigate this today, but when I get some more time I'll dig a little more on this line of enquiry to see if I can confirm or deny the above theory. > > Maybe I'm misremembering. > > In any case, if you change this, then you also should change > the function's entry comment: > > /* Adjust SAL to the first instruction past the function prologue. > If the PC was explicitly specified, the SAL is not changed. > If the line number was explicitly specified, at most the SAL's PC > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > is updated. If SAL is already past the prologue, then do nothing. */ > ^^^^^^^^^^ Would this be OK? (I'm not pushing anything until the above questions are resolved): Thanks, Andrew > > void > skip_prologue_sal (struct symtab_and_line *sal) > { > > Thanks, > Pedro Alves diff --git a/gdb/symtab.c b/gdb/symtab.c index c10e6b3e358..6e7e32fb4d8 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -3673,8 +3673,10 @@ skip_prologue_using_lineinfo (CORE_ADDR func_addr, struct symtab *symtab) /* Adjust SAL to the first instruction past the function prologue. If the PC was explicitly specified, the SAL is not changed. - If the line number was explicitly specified, at most the SAL's PC - is updated. If SAL is already past the prologue, then do nothing. */ + If the line number was explicitly specified then the SAL can still be + updated, unless the language for SAL is assembler, in which case the SAL + will be left unchanged. + If SAL is already past the prologue, then do nothing. */ void skip_prologue_sal (struct symtab_and_line *sal)