Re: [RFC] Revisit PR 16253 ("Attempt to use a type name...")

[Doug Evans <dje at google dot com>]

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.