From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 70646 invoked by alias); 1 Jul 2019 17:12:19 -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 70637 invoked by uid 89); 1 Jul 2019 17:12:19 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.2 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=disagrees, enquiry, invitation, laying X-HELO: mail-wr1-f66.google.com Received: from mail-wr1-f66.google.com (HELO mail-wr1-f66.google.com) (209.85.221.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 01 Jul 2019 17:12:17 +0000 Received: by mail-wr1-f66.google.com with SMTP id p13so14655090wru.10 for ; Mon, 01 Jul 2019 10:12:16 -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=CAFxHIBydOHsRBgRnIwM+7MVpaT91n2fN2v2PhTiutE=; b=dFo0E/xpFLeVj14hlig7TCsYpgHWRK4HL+YUWlp16PoB8rA8q+QoU0rNO0U9PtoxuM IM2VJhx5EGa2iHNAXKgJeta+3Eh6Sj9F91ho3fpJIhGW+ny1twZMn6znZ6JVWK0SA55z ejp3pt+7wVH2OmhgQYuKnxxmSxdHNiIYnO1gWl5UJAvNmFFFBzokIFgJAx6GcH/jNf0E Ask4TpWtbNMlpurebKyGloV6Tkxrp+VA9UWjr0YhgHMwk0Ul+uyBzYK/Y6NkhytjWoOz T0wqJeVtmK1+nrGrjFa+b5HiWjIOm4MiKpY2UcX+raEPGd6ucaOhvhg3IQz9OJ0EJ6uk ebbg== Return-Path: Received: from localhost (cust64-dsl91-135-5.idnet.net. [91.135.5.64]) by smtp.gmail.com with ESMTPSA id v18sm12963589wrd.51.2019.07.01.10.12.14 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 01 Jul 2019 10:12:14 -0700 (PDT) Date: Mon, 01 Jul 2019 17:12:00 -0000 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: <20190701171213.GU23204@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> <20190622110558.GK23204@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Fortune: Nobody ever died from oven crude poisoning. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2019-07/txt/msg00016.txt.bz2 * Pedro Alves [2019-06-24 20:16:23 +0100]: > On 6/22/19 12:05 PM, Andrew Burgess wrote: > > * 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. > > Since the flag is checked in breakpoint.c, it likely had a use > for breakpoint mode too, though as you say, it's probably not used today. > > Greping for explicit_line finds this case: > > > set_breakpoint_location_function (loc, > sal->explicit_pc || sal->explicit_line); > > in > > add_location_to_breakpoint > > I wonder whether that explicit_line use makes sense here. > > set_breakpoint_location_function says: > > /* Initialize loc->function_name. EXPLICIT_LOC says no indirect function > resolutions should be made as the user specified the location explicitly > enough. */ > > static void > set_breakpoint_location_function (struct bp_location *loc, int explicit_loc) > { > > > I'm not immediately seeing how to set a breakpoint at an ifunc symbol > by line number such that you end up in set_breakpoint_location_function > with loc->msymbol != NULL. We certainly never get into set_breakpoint_location_function with explicit_line set in current HEAD. The explicit_loc parameter was added in commit 0e30163f127122 (which was really about adding or improving ifunc support) and the tests were in commit e462023046d892. Out of interest I ran the testsuite checking to see if: loc->msymbol != NULL && explicit_loc was ever true (in current HEAD) and it never is. I suspect this is because loc->msymbol is initialised from sal->msymbol, which is only set in one place linespec.c:minsym_found, which I don't think allows for the address and/or line number to be explicitly stated. After this investigation I propose that the explicit_loc parameter be removed completely from set_breakpoint_location_function - I've included this in an updated series that I'll post shortly > > We should probably delete that sal->explicit_line reference. Agreed. > > > > >> > >> 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? > > Sounds fine to me in principle. I'll do this as a separate patch. > > > 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. > > So does this mean that linespec nowadays always fills in the PC > then? This is my claim. > > If that's the case, when do we need that is_pc_initialized method? I imagine it would only be used within an assertion. > > I wonder what else is not reachable around here, laying around > waiting to be garbage collected... Indeed... > > >>> 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. > > Yes, exactly. > > > > > 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. > > Did you check whether we're already setting explicit_line when > parsing "b -line N", i.e., when using the explicit locations syntax? In current HEAD explicit_line will only get set for the clear, edit, list, and 'info line' commands. Any variation of setting breakpoints will never set explicit_line. I'll post an updated series very shortly. Thanks, Andrew > > > > >> > >> 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): > > Sure. > > > > > 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) > Thanks, > Pedro Alves