From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 44215 invoked by alias); 16 Jun 2015 17:54:28 -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 44094 invoked by uid 89); 16 Jun 2015 17:54:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=no version=3.3.2 X-HELO: mail-qc0-f202.google.com Received: from mail-qc0-f202.google.com (HELO mail-qc0-f202.google.com) (209.85.216.202) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 16 Jun 2015 17:54:24 +0000 Received: by qcrw7 with SMTP id w7so916571qcr.1 for ; Tue, 16 Jun 2015 10:54:22 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:message-id:date:subject:from:to:cc :content-type; bh=1cLwyRBYc/UFCHheOhv6AFYJBpj6OdPWkah8cy52qM8=; b=hDXOLRBC5Sy0ineepWYo7AP7AhtMvJGhiD27jfqgVOzLn70+ybRCF7OeH1gO1RJiew H+kkzsFSbQ0mOa6XR64P2RvC7H4W28HRgtr+wsOGwBiOnlQp1Y6uMJk6nwOfwXe36MNZ 7xna27SZ/gUpMduhxd0HJZX2DcuPhC0RNcQNegc8luGZXthnsJ/RZSfjJBSiANgyqTmn 9KDXVa98B+CxKQgQyg4x0d9HsPIwEwpI62FdRSm3hawWZGq/3Naa1yl54KuzemvOJvlY +ZBVAyeXIK35c56HCpyVtziQOX8KsetVrsT3xYfzTcRn4imPhuXplBAAiJKRRe6vTvbu 8uNw== X-Gm-Message-State: ALoCoQk8bb3T291yDHiBAlr6p3e6jQup8DlRCStZkH9AVk+2Evj9lirq0i5yXpdZiPq2ELz8y/LJ MIME-Version: 1.0 X-Received: by 10.140.236.67 with SMTP id h64mr1721002qhc.13.1434477262335; Tue, 16 Jun 2015 10:54:22 -0700 (PDT) Message-ID: <001a1135a192a92e3f0518a643d3@google.com> Date: Tue, 16 Jun 2015 17:54:00 -0000 Subject: Re: [RFC] Revisit PR 16253 ("Attempt to use a type name...") From: Doug Evans To: Keith Seitz Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=UTF-8; format=flowed; delsp=yes X-IsSubscribed: yes X-SW-Source: 2015-06/txt/msg00352.txt.bz2 Keith Seitz writes: > Last year a patch was submitted/approved/commited to eliminate > symbol_matches_domain which was causing this problem. It was later reverted > because it introduced a (severe) performance regression. > > Recap: > > (gdb) list > 1 enum e {A,B,C} e; > 2 int main (void) { return 0; } > 3 > (gdb) p e > Attempt to use a type name as an expression > > The parser attempts to find a symbol named "e" of VAR_DOMAIN. > This gets passed down through lookup_symbol and (eventually) into > block_lookup_symbol_primary, which iterates over the block's dictionary > of symbols: > > for (sym = dict_iter_name_first (block->dict, name, &dict_iter); > sym != NULL; > sym = dict_iter_name_next (name, &dict_iter)) > { > if (symbol_matches_domain (SYMBOL_LANGUAGE (sym), > SYMBOL_DOMAIN (sym), domain)) > return sym; > } > > The problem here is that we have a symbol named "e" in both STRUCT_DOMAIN > and VAR_DOMAIN, and for languages like C++, Java, and Ada, where a tag name > may be used as an implicit typedef of the type, symbol_matches_domain ignores > the difference between VAR_DOMAIN and STRUCT_DOMAIN. As it happens, the > STRUCT_DOMAIN symbol is found first, considered a match, and that symbol is > returned to the parser, eliciting the (now dreaded) error message. > > Since this bug exists specifically because we have both STRUCT and VAR_DOMAIN > symbols in a given block/CU, this patch rather simply/naively changes > block_lookup_symbol_primary so that it continues to search for an exact > domain match on the symbol if symbol_matches_domain returns a symbol > which does not exactly match the requested domain. > > This "fixes" the immediate problem, but admittedly might uncover other, > related bugs. [Paranoia?] However, it causes no regressions (functional > or performance) in the test suite. > > I have also resurrected the tests from the previous submission. However > since we can still be given a matching symbol with a different domain than > requested, we cannot say that a symbol "was not found." The error messages > today will still be the (dreaded) "Attempt to use a type name..." I've > updated the tests to reflect this. > > ChangeLog > > PR 16253 > * block.c (block_lookup_symbol_primary): If a symbol is found > which does not exactly match the requested domain, keep searching > for an exact match. Otherwise, return the previously found "best" > symbol. Hi. This approach is an improvement with no ill effects (that I can see), so I'm ok with it. Please add a reference to PR 16253 in the "hack" comment in the code. Do other callers of symbol_matches_domain need similar treatment? I was wondering about block_lookup_symbol. btw, here's some perf data using the gmonster1-pervasive-typedef.exp test from my monster testcase generator. [basically, every CU has "typedef int my_int;" and the test does "ptype my_func" where my_func takes a my_int as an argument] trunk: gmonster1:gmonster-pervasive-typedef cpu_time 10-cus 0.00091 gmonster1:gmonster-pervasive-typedef cpu_time 100-cus 0.000966 gmonster1:gmonster-pervasive-typedef cpu_time 1000-cus 0.001145 gmonster1:gmonster-pervasive-typedef cpu_time 10000-cus 0.011014 gmonster1:gmonster-pervasive-typedef wall_time 10-cus 0.000936985015869 gmonster1:gmonster-pervasive-typedef wall_time 100-cus 0.000993967056274 gmonster1:gmonster-pervasive-typedef wall_time 1000-cus 0.00116896629333 gmonster1:gmonster-pervasive-typedef wall_time 10000-cus 0.0110721588135 gmonster1:gmonster-pervasive-typedef vmsize 10-cus 31480 gmonster1:gmonster-pervasive-typedef vmsize 100-cus 67152 gmonster1:gmonster-pervasive-typedef vmsize 1000-cus 429264 gmonster1:gmonster-pervasive-typedef vmsize 10000-cus 3976272 with orig 16253 patch: gmonster1:gmonster-pervasive-typedef cpu_time 10-cus 0.060466 gmonster1:gmonster-pervasive-typedef cpu_time 100-cus 0.549413 gmonster1:gmonster-pervasive-typedef cpu_time 1000-cus 8.515956 gmonster1:gmonster-pervasive-typedef cpu_time 10000-cus 492.20361 gmonster1:gmonster-pervasive-typedef wall_time 10-cus 0.0605120658875 gmonster1:gmonster-pervasive-typedef wall_time 100-cus 0.549317836761 gmonster1:gmonster-pervasive-typedef wall_time 1000-cus 8.5144739151 gmonster1:gmonster-pervasive-typedef wall_time 10000-cus 492.134341955 gmonster1:gmonster-pervasive-typedef vmsize 10-cus 43176 gmonster1:gmonster-pervasive-typedef vmsize 100-cus 174356 gmonster1:gmonster-pervasive-typedef vmsize 1000-cus 1501776 gmonster1:gmonster-pervasive-typedef vmsize 10000-cus 14895344 Ok, it's not in the 1000s :-), but it is an additional 490+ seconds and 10+ GB. with this 16253 patch: gmonster1:gmonster-pervasive-typedef cpu_time 10-cus 0.0009 gmonster1:gmonster-pervasive-typedef cpu_time 100-cus 0.000964 gmonster1:gmonster-pervasive-typedef cpu_time 1000-cus 0.001097 gmonster1:gmonster-pervasive-typedef cpu_time 10000-cus 0.010179 gmonster1:gmonster-pervasive-typedef wall_time 10-cus 0.000927209854126 gmonster1:gmonster-pervasive-typedef wall_time 100-cus 0.000990152359009 gmonster1:gmonster-pervasive-typedef wall_time 1000-cus 0.00112009048462 gmonster1:gmonster-pervasive-typedef wall_time 10000-cus 0.0102391242981 gmonster1:gmonster-pervasive-typedef vmsize 10-cus 31484 gmonster1:gmonster-pervasive-typedef vmsize 100-cus 67148 gmonster1:gmonster-pervasive-typedef vmsize 1000-cus 429248 gmonster1:gmonster-pervasive-typedef vmsize 10000-cus 3976256 btw, I think I have a simple way to get the perf back with the original patch, but it involves (I think, TBD) "breaking" psyms/gold-generated-gdb-index the same way gdb-generated-gdb-index is "broken": PR 17387. Namely, only record one static psym, the theory being if one is not in a context where static symbol my_foo is defined, gdb is going to (essentially) pick a random one so why record them all? The catch is that, e.g., "info types foo" uses psyms/gdb-index too so if we went this route we'd either have to accept the breakage that .gdb_index introduced (PR 17387) or rewrite "info types, etc., to work differently: it'd have to scan the debug info, but how important is a fast "info types"? One could employ various kinds of caching to speed things up a bit. The other way is recording the symbol domain in the index. Neither of these is proposed for 7.10 though.