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

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

[Doug Evans]

Jan Kratochvil writes:
  > On Tue, 23 Jun 2015 20:39:52 +0200, Keith Seitz wrote:
  > > [But then my philosophy is "if it ain't broke, don't fix it." As far as
  > > I can tell, block_lookup_symbol is not "broke."]
  >
  > I agree, just I think a proper fix would cover also  
block_lookup_symbol().
  > But then it is questionable what is a proper fix as after this fix the  
C++
  > expression support still remains poor.  The proper GDB fix is  
C++ 'compile'
  > support being worked on.

I dunno.
We still have to keep gdb working well enough without "compile",
(other languages, core files, embedded systems)
and compile-object-load.c calls block_lookup_symbol
("compile" still relies on gdb's symbol lookup machinery).

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

[Keith Seitz]

On 06/24/2015 04:02 PM, Doug Evans wrote:
> A variation of PR 18150?
> You need to put the symbols in an unexpanded symtab,
> the catch being that gdb expands the symtab with main() at startup.
> 

Doh! That does indeed do it!

>  > Nonetheless it is not quite so straight-forward in the BLOCK_FUNCTION
>  > case where we have to decide what is a "better" match:
>  >
>  >   SYMBOL_DOMAIN == domain && SYMBOL_IS_ARGUMENT
>  >
>  > or
>  >   SYMBOL_DOMAIN != domain (but symbol_matches_domain returns 1) &&
>  > !SYMBOL_IS_ARGUMENT
> 
> I'm not sure either. I'm not sure the BLOCK_FUNCTION case
> can even exercise this bug.

Forest/trees. Darn my vision! :-)

> 
>  > In that case, I cannot say which is more correct. Moreover I have been
>  > unable to figure out how to test this. I worry that I would simply be
>  > introducing a regression. IMO this is getting into "risky" territory.
>  > [But then my philosophy is "if it ain't broke, don't fix it." As far as
>  > I can tell, block_lookup_symbol is not "broke."]
>  >
>  > So what do maintainers want me to do?
> 
> How about this?

That looks good to me, and is fully covered by the test suite.

/me very happy
Keith

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

[Doug Evans]

Keith Seitz writes:
  > On 06/17/2015 08:50 AM, Keith Seitz wrote:
  > > On 06/17/2015 05:33 AM, Jan Kratochvil wrote:
  > >> On Thu, 11 Jun 2015 20:57:18 +0200, Keith Seitz wrote:
  > >>> 	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.
  > >>
  > >> Is there a reason why you haven't patched also block_lookup_symbol?
  > >
  > > Two reasons:
  > >
  > > 1) I posted this RFC to raise discussion whether this was worth
  > > pursuing. No need to expend any energy on something that has zero  
chance
  > > of being accepted by maintainers.
  > >
  > > 2) More important I have not discovered/attempted to coverage test
  > > lookup_block_symbol to trigger this bug.
  > >
  > > I'll be attempting to do that today.
  >
  > And I failed. I've not found the "magic" combination of buttons to make
  > any difference in block_lookup_symbol.

A variation of PR 18150?
You need to put the symbols in an unexpanded symtab,
the catch being that gdb expands the symtab with main() at startup.

Lookup in expanded symtabs, which is done before looking up in
unexpanded symtabs, is done with block_lookup_symbol_primary.
Lookup in unexpanded symtabs is done with block_lookup_symbol.

  > Nonetheless it is not quite so straight-forward in the BLOCK_FUNCTION
  > case where we have to decide what is a "better" match:
  >
  >   SYMBOL_DOMAIN == domain && SYMBOL_IS_ARGUMENT
  >
  > or
  >   SYMBOL_DOMAIN != domain (but symbol_matches_domain returns 1) &&
  > !SYMBOL_IS_ARGUMENT

I'm not sure either. I'm not sure the BLOCK_FUNCTION case
can even exercise this bug.

  > In that case, I cannot say which is more correct. Moreover I have been
  > unable to figure out how to test this. I worry that I would simply be
  > introducing a regression. IMO this is getting into "risky" territory.
  > [But then my philosophy is "if it ain't broke, don't fix it." As far as
  > I can tell, block_lookup_symbol is not "broke."]
  >
  > So what do maintainers want me to do?

How about this?

diff --git a/gdb/block.c b/gdb/block.c
index 79a8f19..93ed6e7 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -739,13 +739,21 @@ block_lookup_symbol (const struct block *block, const  
char *name,

    if (!BLOCK_FUNCTION (block))
      {
+      struct symbol *other = NULL;
+
        ALL_BLOCK_SYMBOLS_WITH_NAME (block, name, iter, sym)
  	{
+	  if (SYMBOL_DOMAIN (sym) == domain)
+	    return sym;
+	  /* This is a bit of a hack, but symbol_matches_domain might ignore
+	     STRUCT vs VAR domain symbols.  So if a matching symbol is found,
+	     make sure there is no "better" matching symbol, i.e., one with
+	     exactly the same domain.  PR 16253.  */
  	  if (symbol_matches_domain (SYMBOL_LANGUAGE (sym),
  				     SYMBOL_DOMAIN (sym), domain))
-	    return sym;
+	    other = sym;
  	}
-      return NULL;
+      return other;
      }
    else
      {
@@ -753,7 +761,10 @@ block_lookup_symbol (const struct block *block, const  
char *name,
  	 list; this loop makes sure to take anything else other than
  	 parameter symbols first; it only uses parameter symbols as a
  	 last resort.  Note that this only takes up extra computation
-	 time on a match.  */
+	 time on a match.
+	 It's hard to define types in the parameter list (at least in
+	 C/C++) so we don't do the same PR 16253 hack here that is done
+	 for the !BLOCK_FUNCTION case.  */

        struct symbol *sym_found = NULL;

@@ -779,23 +790,33 @@ struct symbol *
  block_lookup_symbol_primary (const struct block *block, const char *name,
  			     const domain_enum domain)
  {
-  struct symbol *sym;
+  struct symbol *sym, *other;
    struct dict_iterator dict_iter;

    /* Verify BLOCK is STATIC_BLOCK or GLOBAL_BLOCK.  */
    gdb_assert (BLOCK_SUPERBLOCK (block) == NULL
  	      || BLOCK_SUPERBLOCK (BLOCK_SUPERBLOCK (block)) == NULL);

+  other = NULL;
    for (sym = dict_iter_name_first (block->dict, name, &dict_iter);
         sym != NULL;
         sym = dict_iter_name_next (name, &dict_iter))
      {
+      if (SYMBOL_DOMAIN (sym) == domain)
+	return sym;
+
+      /* This is a bit of a hack, but symbol_matches_domain might ignore
+	 STRUCT vs VAR domain symbols.  So if a matching symbol is found,
+	 make sure there is no "better" matching symbol, i.e., one with
+	 exactly the same domain.  PR 16253.  */
        if (symbol_matches_domain (SYMBOL_LANGUAGE (sym),
  				 SYMBOL_DOMAIN (sym), domain))
-	return sym;
+	{
+	  other = sym;
+	}
      }

-  return NULL;
+  return other;
  }

  /* See block.h.  */
diff --git a/gdb/testsuite/gdb.cp/var-tag-2.cc  
b/gdb/testsuite/gdb.cp/var-tag-2.cc
new file mode 100644
index 0000000..7733473
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/var-tag-2.cc
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2015 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.   
*/
+
+/* This object is in a separate file so that its debug info is not
+   expanded at startup.  Once debug info is expanded we are no longer
+   exercising block_lookup_symbol, and instead are exercising
+   block_lookup_symbol_primary.  */
+enum E2 {a2, b2, c2} E2;
diff --git a/gdb/testsuite/gdb.cp/var-tag-3.cc  
b/gdb/testsuite/gdb.cp/var-tag-3.cc
new file mode 100644
index 0000000..7f2133f
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/var-tag-3.cc
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2015 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.   
*/
+
+/* This object is in a separate file so that its debug info is not
+   expanded at startup.  Once debug info is expanded we are no longer
+   exercising block_lookup_symbol, and instead are exercising
+   block_lookup_symbol_primary.  */
+struct S2 {} S2;
diff --git a/gdb/testsuite/gdb.cp/var-tag-4.cc  
b/gdb/testsuite/gdb.cp/var-tag-4.cc
new file mode 100644
index 0000000..162541c
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/var-tag-4.cc
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2015 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.   
*/
+
+/* This object is in a separate file so that its debug info is not
+   expanded at startup.  Once debug info is expanded we are no longer
+   exercising block_lookup_symbol, and instead are exercising
+   block_lookup_symbol_primary.  */
+union U2 {int a; char b;} U2;
diff --git a/gdb/testsuite/gdb.cp/var-tag.cc  
b/gdb/testsuite/gdb.cp/var-tag.cc
new file mode 100644
index 0000000..93b9caf
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/var-tag.cc
@@ -0,0 +1,44 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.   
*/
+
+int global = 3;
+
+class C {
+public:
+  struct C1 {} C1;
+  enum E1 {a1, b1, c1} E1;
+  union U1 {int a1; char b1;} U1;
+
+  C () : E1 (b1) {}
+  void global (void) const {}
+  int f (void) const { global (); return 0; }
+} C;
+
+struct S {} S;
+enum E {a, b, c} E;
+union U {int a; char b;} U;
+
+class CC {} cc;
+struct SS {} ss;
+enum EE {ea, eb, ec} ee;
+union UU {int aa; char bb;} uu;
+
+int
+main (void)
+{
+  return C.f ();
+}
diff --git a/gdb/testsuite/gdb.cp/var-tag.exp  
b/gdb/testsuite/gdb.cp/var-tag.exp
new file mode 100644
index 0000000..9854c31
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/var-tag.exp
@@ -0,0 +1,105 @@
+# Copyright 2014, 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the gdb testsuite
+
+# Test expressions in which variable names shadow tag names.
+
+if {[skip_cplus_tests]} { continue }
+
+standard_testfile var-tag.cc var-tag-2.cc var-tag-3.cc var-tag-4.cc
+
+if {[prepare_for_testing $testfile.exp $testfile \
+	[list $srcfile $srcfile2 $srcfile3 $srcfile4] {debug c++}]} {
+    return -1
+}
+
+proc do_global_tests {lang} {
+    set invalid_print "Attempt to use a type name as an expression"
+    set ptypefmt "type = (class|enum|union|struct) %s {.*}"
+
+    with_test_prefix $lang {
+	gdb_test_no_output "set language $lang"
+	gdb_test "ptype C" "type = class C {.*}"
+	gdb_test "print E" "= a"
+	gdb_test "ptype E" "type = enum E {.*}"
+	gdb_test "print S" "= {<No data fields>}"
+	gdb_test "ptype S" "type = struct S {.*}"
+	gdb_test "print U" "= {.*}"
+	gdb_test "ptype U" "type = union U {.*}"
+	gdb_test "print cc" "= {.*}"
+	gdb_test "ptype cc" "type = class CC {.*}"
+	gdb_test "print CC" [format $invalid_print "CC"]
+	gdb_test "ptype CC" [format $ptypefmt "CC"]
+	gdb_test "print ss" "= {<No data fields>}"
+	gdb_test "ptype ss" "type = struct SS {.*}"
+	gdb_test "print SS" [format $invalid_print "SS"]
+	gdb_test "ptype SS" [format $ptypefmt "SS"]
+	gdb_test "print ee" "= .*"
+	gdb_test "ptype ee" "type = enum EE {.*}"
+	gdb_test "print EE" [format $invalid_print "EE"]
+	gdb_test "ptype EE" [format $ptypefmt "EE"]
+	gdb_test "print uu" "= {.*}"
+	gdb_test "ptype uu" "type = union UU {.*}"
+	gdb_test "print UU" [format $invalid_print  "UU"]
+	gdb_test "ptype UU" [format $ptypefmt "UU"]
+
+	# 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 {.*}"
+    }
+}
+
+# First test expressions when there is no context.
+with_test_prefix "before start" {
+    do_global_tests c++
+    do_global_tests c
+}
+
+# Run to main and test again.
+if {![runto_main]} {
+    perror "couldn't run to main"
+    continue
+}
+
+with_test_prefix "in main" {
+    do_global_tests c++
+    do_global_tests c
+}
+
+# Run to C::f and test again.
+gdb_breakpoint "C::f"
+gdb_continue_to_breakpoint "continue to C::f"
+with_test_prefix "in C::f" {
+    do_global_tests c++
+    do_global_tests c
+}
+
+# Another hard-to-guess-the-users-intent bug...
+# It would be really nice if we could query the user!
+with_test_prefix "global collision" {
+    gdb_test_no_output "set language c++"
+    setup_kfail "c++/16463" "*-*-*"
+    gdb_test "print global" "= 3"
+
+    # ... with a simple workaround:
+    gdb_test "print ::global" "= 3"
+}

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

[Jan Kratochvil]

On Tue, 23 Jun 2015 20:39:52 +0200, Keith Seitz wrote:
> [But then my philosophy is "if it ain't broke, don't fix it." As far as
> I can tell, block_lookup_symbol is not "broke."]

I agree, just I think a proper fix would cover also block_lookup_symbol().
But then it is questionable what is a proper fix as after this fix the C++
expression support still remains poor.  The proper GDB fix is C++ 'compile'
support being worked on.



Jan

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

[Keith Seitz]

On 06/17/2015 08:50 AM, Keith Seitz wrote:
> On 06/17/2015 05:33 AM, Jan Kratochvil wrote:
>> On Thu, 11 Jun 2015 20:57:18 +0200, Keith Seitz wrote:
>>> 	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.
>>
>> Is there a reason why you haven't patched also block_lookup_symbol?
> 
> Two reasons:
> 
> 1) I posted this RFC to raise discussion whether this was worth
> pursuing. No need to expend any energy on something that has zero chance
> of being accepted by maintainers.
> 
> 2) More important I have not discovered/attempted to coverage test
> lookup_block_symbol to trigger this bug.
> 
> I'll be attempting to do that today.

And I failed. I've not found the "magic" combination of buttons to make
any difference in block_lookup_symbol.

Nonetheless it is not quite so straight-forward in the BLOCK_FUNCTION
case where we have to decide what is a "better" match:

  SYMBOL_DOMAIN == domain && SYMBOL_IS_ARGUMENT

or
  SYMBOL_DOMAIN != domain (but symbol_matches_domain returns 1) &&
!SYMBOL_IS_ARGUMENT

In that case, I cannot say which is more correct. Moreover I have been
unable to figure out how to test this. I worry that I would simply be
introducing a regression. IMO this is getting into "risky" territory.
[But then my philosophy is "if it ain't broke, don't fix it." As far as
I can tell, block_lookup_symbol is not "broke."]

So what do maintainers want me to do?

Keith

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

[Keith Seitz]

On 06/17/2015 05:33 AM, Jan Kratochvil wrote:
> On Thu, 11 Jun 2015 20:57:18 +0200, Keith Seitz wrote:
>> 	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.
> 
> Is there a reason why you haven't patched also block_lookup_symbol?

Two reasons:

1) I posted this RFC to raise discussion whether this was worth
pursuing. No need to expend any energy on something that has zero chance
of being accepted by maintainers.

2) More important I have not discovered/attempted to coverage test
lookup_block_symbol to trigger this bug.

I'll be attempting to do that today.

Keith

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

[Keith Seitz]

On 06/16/2015 10:54 AM, Doug Evans wrote:
>
> 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.
> 

Will do.

> Do other callers of symbol_matches_domain need similar treatment?
> I was wondering about block_lookup_symbol.

Yeah, I wonder about that myself, but I am cautious to commit to that
unless I can coverage test it. So far, I've not been able to trigger a
failure in any of these.

The proposed change is (intentionally) very conservative. While I could
be convinced to add it (nearly?) wherever symbol_matches_domain is used,
coverage testing will be difficult, if not impossible. [I spent many,
many hours doing coverage testing of the completer API change. It was
gruesome!]

> btw, here's some perf data using the gmonster1-pervasive-typedef.exp
> test from my monster testcase generator.

Thanks for that! I haven't yet had a go at it this.

Keith

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

[Jan Kratochvil]

On Thu, 11 Jun 2015 20:57:18 +0200, Keith Seitz wrote:
> 	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.

Is there a reason why you haven't patched also block_lookup_symbol?


Jan

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

[Doug Evans]

On Tue, Jun 16, 2015 at 12:54 PM, Doug Evans <dje@google.com> wrote:
>...
> 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.

Sorry for the followup.

Another way would be to have two different kinds of lookup.
IOW, record all the static versions of a symbol in the index,
but (1) only use one of them (the first?) for some lookups and
(2) use all of them for "info types"-like lookups.

It's still substandard, I think, as the symbol we use
for (1) may have the wrong domain, and we should really just
fix gdb-generated-gdb-index for 17387,
and record the domain in the index to fix the perf issue.

OTOH, as with your v2 patch for 16253, it's an incremental
approach.

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

[Doug Evans]

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.

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

[Jan Kratochvil]

Hi Keith,

gcc-4.9.2-6.fc21.x86_64
PASS

gcc-5.1.1-1.fc22.x86_64
gcc-5.1.1-1.fc23.x86_64
FAIL: gdb.cp/var-tag.exp: before start: c++: ptype E
FAIL: gdb.cp/var-tag.exp: before start: c++: ptype ee
FAIL: gdb.cp/var-tag.exp: before start: c++: ptype EE
FAIL: gdb.cp/var-tag.exp: in main: c++: ptype E
FAIL: gdb.cp/var-tag.exp: in main: c++: ptype ee
FAIL: gdb.cp/var-tag.exp: in main: c++: ptype EE
FAIL: gdb.cp/var-tag.exp: in C::f: c++: ptype E
FAIL: gdb.cp/var-tag.exp: in C::f: c++: ptype ee
FAIL: gdb.cp/var-tag.exp: in C::f: c++: ptype EE

All the FAILs are like:
 ptype E^M
-type = enum E {a, b, c}^M
-(gdb) PASS: gdb.cp/var-tag.exp: before start: c++: ptype E
+type = enum E : unsigned int {a, b, c}^M
+(gdb) FAIL: gdb.cp/var-tag.exp: before start: c++: ptype E

So it seems to me as an unrelated GDB<->GCC compatibility problem.


Jan

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

[Keith Seitz]

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.

testsuite/ChangeLog

	PR 16253
	* gdb.cp/var-tag.cc: New file.
	* gdb.cp/var-tag.exp: New file.
---
 gdb/ChangeLog                    |  8 ++++
 gdb/block.c                      | 16 +++++--
 gdb/testsuite/ChangeLog          |  6 +++
 gdb/testsuite/gdb.cp/var-tag.cc  | 44 +++++++++++++++++++
 gdb/testsuite/gdb.cp/var-tag.exp | 94 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 165 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/var-tag.cc
 create mode 100644 gdb/testsuite/gdb.cp/var-tag.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 9075fcc..5a7db75 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2015-06-11  Keith Seitz  <keiths@redhat.com>
+
+	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.
+
 2015-06-11  Gary Benson <gbenson@redhat.com>
 
 	* nat/linux-namespaces.c (mnsh_send_message): Use pulongest.
diff --git a/gdb/block.c b/gdb/block.c
index 79a8f19..e043d61 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -779,23 +779,33 @@ struct symbol *
 block_lookup_symbol_primary (const struct block *block, const char *name,
 			     const domain_enum domain)
 {
-  struct symbol *sym;
+  struct symbol *sym, *other;
   struct dict_iterator dict_iter;
 
   /* Verify BLOCK is STATIC_BLOCK or GLOBAL_BLOCK.  */
   gdb_assert (BLOCK_SUPERBLOCK (block) == NULL
 	      || BLOCK_SUPERBLOCK (BLOCK_SUPERBLOCK (block)) == NULL);
 
+  other = NULL;
   for (sym = dict_iter_name_first (block->dict, name, &dict_iter);
        sym != NULL;
        sym = dict_iter_name_next (name, &dict_iter))
     {
+      if (SYMBOL_DOMAIN (sym) == domain)
+	return sym;
+
+      /* This is a bit of a hack, but symbol_matches_domain might ignore
+	 STRUCT vs VAR domain symbols.  So if a matching symbol is found, make
+	 sure there is no "better" matching symbol, i.e., one with
+	 exactly the same domain.  */
       if (symbol_matches_domain (SYMBOL_LANGUAGE (sym),
 				 SYMBOL_DOMAIN (sym), domain))
-	return sym;
+	{
+	  other = sym;
+	}
     }
 
-  return NULL;
+  return other;
 }
 
 /* See block.h.  */
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 4dbbd92..b5d9873 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2015-06-11  Keith Seitz  <keiths@redhat.com>
+
+	PR 16253
+	* gdb.cp/var-tag.cc: New file.
+	* gdb.cp/var-tag.exp: New file.
+
 2015-06-10  Walfred Tedeschi  <walfred.tedeschi@intel.com>
             Mircea Gherzan  <mircea.gherzan@intel.com>
 
diff --git a/gdb/testsuite/gdb.cp/var-tag.cc b/gdb/testsuite/gdb.cp/var-tag.cc
new file mode 100644
index 0000000..93b9caf
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/var-tag.cc
@@ -0,0 +1,44 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int global = 3;
+
+class C {
+public:
+  struct C1 {} C1;
+  enum E1 {a1, b1, c1} E1;
+  union U1 {int a1; char b1;} U1;
+
+  C () : E1 (b1) {}
+  void global (void) const {}
+  int f (void) const { global (); return 0; }
+} C;
+
+struct S {} S;
+enum E {a, b, c} E;
+union U {int a; char b;} U;
+
+class CC {} cc;
+struct SS {} ss;
+enum EE {ea, eb, ec} ee;
+union UU {int aa; char bb;} uu;
+
+int
+main (void)
+{
+  return C.f ();
+}
diff --git a/gdb/testsuite/gdb.cp/var-tag.exp b/gdb/testsuite/gdb.cp/var-tag.exp
new file mode 100644
index 0000000..3a8831f
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/var-tag.exp
@@ -0,0 +1,94 @@
+# Copyright 2014, 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the gdb testsuite
+
+# Test expressions in which variable names shadow tag names.
+
+if {[skip_cplus_tests]} { continue }
+
+standard_testfile .cc
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug c++}]} {
+    return -1
+}
+
+proc do_global_tests {lang} {
+    set invalid_print "Attempt to use a type name as an expression"
+    set ptypefmt "type = (class|enum|union|struct) %s {.*}"
+
+    with_test_prefix $lang {
+	gdb_test_no_output "set language $lang"
+	gdb_test "ptype C" "type = class C {.*}"
+	gdb_test "print E" "= a"
+	gdb_test "ptype E" "type = enum E {.*}"
+	gdb_test "print S" "= {<No data fields>}"
+	gdb_test "ptype S" "type = struct S {.*}"
+	gdb_test "print U" "= {.*}"
+	gdb_test "ptype U" "type = union U {.*}"
+	gdb_test "print cc" "= {.*}"
+	gdb_test "ptype cc" "type = class CC {.*}"
+	gdb_test "print CC" [format $invalid_print "CC"]
+	gdb_test "ptype CC" [format $ptypefmt "CC"]
+	gdb_test "print ss" "= {<No data fields>}"
+	gdb_test "ptype ss" "type = struct SS {.*}"
+	gdb_test "print SS" [format $invalid_print "SS"]
+	gdb_test "ptype SS" [format $ptypefmt "SS"]
+	gdb_test "print ee" "= .*"
+	gdb_test "ptype ee" "type = enum EE {.*}"
+	gdb_test "print EE" [format $invalid_print "EE"]
+	gdb_test "ptype EE" [format $ptypefmt "EE"]
+	gdb_test "print uu" "= {.*}"
+	gdb_test "ptype uu" "type = union UU {.*}"
+	gdb_test "print UU" [format $invalid_print  "UU"]
+	gdb_test "ptype UU" [format $ptypefmt "UU"]
+    }
+}
+
+# First test expressions when there is no context.
+with_test_prefix "before start" {
+    do_global_tests c++
+    do_global_tests c
+}
+
+# Run to main and test again.
+if {![runto_main]} {
+    perror "couldn't run to main"
+    continue
+}
+
+with_test_prefix "in main" {
+    do_global_tests c++
+    do_global_tests c
+}
+
+# Finally run to C::f and test again
+gdb_breakpoint "C::f"
+gdb_continue_to_breakpoint "continue to C::f"
+with_test_prefix "in C::f" {
+    do_global_tests c++
+    do_global_tests c
+}
+
+# Another hard-to-guess-the-users-intent bug...
+# It would be really nice if we could query the user!
+with_test_prefix "global collision" {
+    gdb_test_no_output "set language c++"
+    setup_kfail "c++/16463" "*-*-*"
+    gdb_test "print global" "= 3"
+
+    # ... with a simple workaround:
+    gdb_test "print ::global" "= 3"
+}
-- 
2.1.0