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

[Doug Evans <dje@google.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.