Re: [PATCH] PR 16253 revisited

[Doug Evans <dje@google.com>]

Keith Seitz writes:
  > This is a request for formal review of an earlier proposed workaround
  > for c++/16253.  A complete description of the proposal is below.
  >
  > Changes since proposal (with Doug's assistance -- THANKS DOUG!):
  > - Add exact/best domain matching concept to block_lookup_symbol.
  > - Add comment to block_lookup_symbol explaining why c++/16253 is not
  >   likely to affect blocks defined in functions.
  > - Update tests to coverage test block_lookup_symbol.
  >
  > ---
  >
  > 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.  A similar change has been made
  > to block_lookup_symbol for other cases in which this bug might appear.
  >
  > The tests from the previous submission have been resurrected and updated.
  > 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..."
  >
  > ChangeLog
  >
  > 	PR 16253
  > 	* block.c (block_lookup_symbol): For non-function blocks,
  > 	continue to search for a symbol with an exact domain match
  > 	Otherwise, return any previously found "best domain" symbol.
  > 	(block_lookup_symbol_primary): Likewise.
  >
  > testsuite/ChangeLog
  >
  > 	PR 16253
  > 	* gdb.cp/var-tag-2.cc: New file.
  > 	* gdb.cp/var-tag-3.cc: New file.
  > 	* gdb.cp/var-tag-4.cc: New file.
  > 	* gdb.cp/var-tag.cc: New file.
  > 	* gdb.cp/var-tag.exp: New file.

LGTM.

  > +    # These tests exercise lookup of symbols using the "quick fns" API.
  > +    # Each of them is in a separate CU as once its CU is expanded,
  > +    # we're no longer using the quick fns API.
  > +    gdb_test "print E2" "= a2"
  > +    gdb_test "ptype E2" "type = enum E2 {.*}"
  > +    gdb_test "print S2" "= {<No data fields>}"
  > +    gdb_test "ptype S2" "type = struct S2 {.*}"
  > +    gdb_test "print U2" "= {.*}"
  > +    gdb_test "ptype U2" "type = union U2 {.*}"
  > +    }

Just a note for the archives:
The ptypes here will work with expanded symtabs since they follow the print.
If we really want full coverage we'd have to create tests where the
VAR_DOMAIN variable appears ahead of the STRUCT_DOMAIN type
(or even VAR_DOMAIN type for c++), and try all four combinations
(var/type first -x- looking for var/type).
That'd require some handcrafted dwarf that had both cases
(var first or type first), and felt excessive for this particular case.