From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23036 invoked by alias); 4 Jun 2012 04:06:34 -0000 Received: (qmail 22774 invoked by uid 22791); 4 Jun 2012 04:06:32 -0000 X-SWARE-Spam-Status: No, hits=-4.7 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,KAM_STOCKGEN,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,TW_BJ,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-vc0-f169.google.com (HELO mail-vc0-f169.google.com) (209.85.220.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 04 Jun 2012 04:06:19 +0000 Received: by vcbfl10 with SMTP id fl10so2708337vcb.0 for ; Sun, 03 Jun 2012 21:06:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding:x-system-of-record :x-gm-message-state; bh=Z8KmP18BL5P3JpXHmwKsyiNWQsanDZ5uAeU2CgPX9fE=; b=IzHGEIA89nx/iKXzKNrPmLT34BucJRhUPVFqYiZmSPN8FJYCWyuCS+UqT8l/CiJt3S fApDsM2/XEjLSWc8ujQ04npslRVwTb09Y2KDXI9wPZefAHWXHuXC0+Nm+alDZ29dsWAO kbsPS4Kn42gZSWO8KzAYjtNnSNGQqQWybmz1D+5+4+vY/aDROSWUOwRAJ0+Fz9d7YAez j2U74Q4AhzLpGwfw+hNGhkC5yOsNzHKohZBcoKRswY1oi1/OIsvm2WS3vulmEIFSzZUO XluDFzfsc6VaUjpOX+vliBF5WGxR9awp+OTwzBRAJb0m0piNFLXI0v8RY+BOPB6uyyay uoCA== Received: by 10.220.214.8 with SMTP id gy8mr10958542vcb.45.1338782778608; Sun, 03 Jun 2012 21:06:18 -0700 (PDT) MIME-Version: 1.0 Received: by 10.220.214.8 with SMTP id gy8mr10958530vcb.45.1338782778444; Sun, 03 Jun 2012 21:06:18 -0700 (PDT) Received: by 10.52.161.199 with HTTP; Sun, 3 Jun 2012 21:06:18 -0700 (PDT) In-Reply-To: <4FC91A33.5040900@redhat.com> References: <20120524175852.D38381E139C@ruffy2.mtv.corp.google.com> <4FBF47DD.4030100@redhat.com> <4FC91A33.5040900@redhat.com> Date: Mon, 04 Jun 2012 04:06:00 -0000 Message-ID: Subject: Re: [RFA] massively speed up "info var foo" on large programs From: Doug Evans To: Pedro Alves Cc: gdb-patches@sourceware.org, ratmice@gmail.com Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-System-Of-Record: true X-Gm-Message-State: ALoCoQmBVGB5L/q1kC4fGXnVKEN+Hf7RqteDUXcFuWUWqIRzhCKjtQXAf4ej3CK91ERM/61kJaGpzj05R02TMHxrXZ45gkiIxMwQIbA1QEQhPX8XqMbzfhsLFvuDb7fDZZEvWXzILh26eCry/MxezrTrz7Lb4nhJlA== X-IsSubscribed: yes 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 X-SW-Source: 2012-06/txt/msg00070.txt.bz2 On Fri, Jun 1, 2012 at 12:38 PM, Pedro Alves wrote: > On 05/28/2012 05:49 AM, Doug Evans wrote: > >> On Fri, May 25, 2012 at 1:50 AM, Pedro Alves wrote: >>> On 05/25/2012 09:21 AM, Doug Evans wrote: >>> >>>> >>>> The output is different from the previous code, I didn't take into >>>> account the symbols that gdb creates for @plt entries. =A0I think if we >>>> want to continue to provide the current output, we should add an >>>> option to "info var|fun|type" to produce it: the normal case shouldn't >>>> be that slow. >>> >>> >>> Different how? =A0The patch has no testsuite updates, so the email read= er is >>> left wondering. =A0;-) >> >> In the "Non-debugging symbols" section of the output, when a symbol >> would have been found in another objfile, the code would have not >> printed the non-@plt form of the function name. >> With this patch we have a decision to make. =A0Searching all the other >> objfiles is not reasonable (IMO) so what to do? =A0I can think of two >> possibilities: always print it or never print it. =A0Since the symbol in >> question is an artificial symbol created by gdb I have opted for never >> printing it. >> >> Thus instead of seeing this in the "Non-debugging symbols" section of >> the output: >> >> 0x1234 foo@plt >> 0x1234 foo >> >> the output will contain: >> >> 0x1234 foo@plt >> >> Here is v3 of the patch. =A0I added a testcase. >> Regression tested on amd64-linux. >> > > > Took me a while, and a gdb.log diff of the new testcase between > patched and unpatched gdb to grok this. =A0I think this output change > is okay. > >> @@ -3463,21 +3505,13 @@ search_symbols (char *regexp, enum searc >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 || regexec (&datum.preg, SYMBOL_NATURAL_NAME= (msymbol), 0, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 NULL, 0) =3D=3D 0) >> =A0 =A0 =A0 =A0 =A0 =A0 { >> - =A0 =A0 =A0 =A0 =A0 =A0 if (0 =3D=3D find_pc_symtab (SYMBOL_VALUE_ADDR= ESS (msymbol))) >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 { >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* FIXME: carlton/2003-02-04: Given th= at the >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0semantics of lookup_symbol keep= s on changing >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0slightly, it would be a nice id= ea if we had a >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0function lookup_symbol_minsym t= hat found the >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0symbol associated to a given mi= nimal symbol (if >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0any). =A0*/ >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (kind =3D=3D FUNCTIONS_DOMAIN >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 || lookup_symbol (SYMBOL_LINKA= GE_NAME (msymbol), >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 (struct block *) NULL, >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 VAR_DOMAIN, 0) > > There's a comment above all this that talks about "lookup_symbol" that ne= eds > updating. Yeah, already updated in my sandbox. >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D=3D NULL) >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 found_misc =3D 1; >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 } >> + =A0 =A0 =A0 =A0 =A0 =A0 /* Note: An important side-effect of these loo= kup functions >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0is to expand the symbol table if msymbo= l is found. =A0*/ > > Okay, it's an important side-effect, but why do we want the side effect > should IMO be mentioned. It's not obvious to me (a casual reader). Most of gdb's symbol handling is arguably non-obvious. 1/2 :-) I've expanded the comment. >> + =A0 =A0 =A0 =A0 =A0 =A0 if (kind =3D=3D FUNCTIONS_DOMAIN >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ? find_pc_symtab (SYMBOL_VALUE_ADDRESS= (msymbol)) =3D=3D NULL >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 : lookup_msymbol_in_objfile (objfile, = msymbol, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0VAR_DOMAIN) =3D=3D NULL) > > The "lookup_msymbol_in_objfile" function name threw me off for a few > minutes, as in, "if we already know the msymbol exists in objfile, wth > are we looking it up again? =A0For those important side-effects perhaps?" > More below. The comment for the function is pretty unambiguous. > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 found_misc =3D 1; >> =A0 =A0 =A0 =A0 =A0 =A0 } >> >> +/* Wrapper around lookup_symbol_aux_objfile for search_symbols. >> + Look up MSYMBOL in DOMAIN in the global and static blocks of OBJFILE >> + and all related objfiles. */ >> + >> +static struct symbol * >> +lookup_msymbol_in_objfile (struct objfile *objfile, >> + struct minimal_symbol *msymbol, >> + domain_enum domain) >> +{ >> + const char *name =3D SYMBOL_LINKAGE_NAME (msymbol); > > Ah, MSYMBOL is the input, not the output! =A0The lookup_symbol_xxx > functions return a symbol, so I was naively expecting this to return > an msymbol. =A0IMO it'd be clearer if it'd be called something like > lookup_symbol_of_msymbol_in_objfile then. =A0But, the only usage of MSYMB= OL > in the entire function is extracting its name. This would then be much > simpler, clearer and reusable IMO if the prototype and name of > the function was: > > static struct symbol * > lookup_symbol_in_objfile (struct objfile *objfile, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const char *name, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0domain_enum domain); > > And the caller did the SYMBOL_LINKAGE_NAME (msymbol) thing. Actually, I had a similarly named function and prototype early on, but I didn't like it. GDB's symbol support is a mess (IMO), and I wanted a name and usage to restrict it to the task at hand. Anything more general and I was pretty sure this patch would get bogged down. Later on I want to revamp the symbol API, but I don't want this patch tied down by that. (For example, I don't want to bubble up the semantics of demangle_for_lookup to the caller of this function. Here we have an msymbol, we know we have a mangled name. If you want, I can go with lookup_symbol_in_objfile but rename it to lookup_symbol_in_objfile_from_linkage_name or some such.)