Re: [PATCH] PR 16253 revisited

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