FYI: fix spelling of "delete" in c-exp.y

FYI: fix spelling of "delete" in c-exp.y

[Tom Tromey]

I'm checking this in.

Running the test suite with a modified compiler (jason/comdat-debug in
gcc git) shows a few regressions.

Debugging one of these showed that there were some extra spaces in
c-exp.y, that caused problems in this test case.  This patch removes the
spaces.

psymtab_search_name will not remove a trailing space; but it will remove
a trailing " ()".  That is the core of the problem.  That plus the fact
that, with an unmodified compiler, all the methods end up in the same
CU, so we bypass the psymtab bits for the particular lookup.

I was also surprised to find the call to value_maybe_namespace_elt in
from value_struct_elt_for_reference doing a lot of work here.
I suspect this is basically a consequence of not doing type interning;
we end up with different copies of the enclosing type in different CUs,
and each one has a different set of methods attached depending on
which CU contains a method's body.

IOW, symbol tables are a crazy mess, see
http://sourceware.org/gdb/wiki/SymbolHandling

Built and regtested on x86-64 Fedora 16, with the comdat-debug gcc and
the system gcc.

Tom

2012-06-18  Tom Tromey  <tromey@redhat.com>

	* c-exp.y (operator): Remove trailing space after "delete" and
	"delete[]".

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 1e14337..bf53fbc 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -1224,11 +1224,11 @@ const_or_volatile_noopt:  	const_and_volatile
 operator:	OPERATOR NEW
 			{ $$ = operator_stoken (" new"); }
 	|	OPERATOR DELETE
-			{ $$ = operator_stoken (" delete "); }
+			{ $$ = operator_stoken (" delete"); }
 	|	OPERATOR NEW '[' ']'
 			{ $$ = operator_stoken (" new[]"); }
 	|	OPERATOR DELETE '[' ']'
-			{ $$ = operator_stoken (" delete[] "); }
+			{ $$ = operator_stoken (" delete[]"); }
 	|	OPERATOR '+'
 			{ $$ = operator_stoken ("+"); }
 	|	OPERATOR '-'

Regression for -gstabs+ [Re: FYI: fix spelling of "delete" in c-exp.y]

[Jan Kratochvil]

Hi,

On Mon, 18 Jun 2012 22:22:11 +0200, Tom Tromey wrote:
> I'm checking this in.

this exactly reverts my change:
	[WIP patch] Fix crash regressions after libiberty/ update
	http://sourceware.org/ml/gdb-patches/2012-01/msg00261.html

I made there this change to match libiberty/cp-demangle.c
cplus_demangle_operators[].


> Running the test suite with a modified compiler (jason/comdat-debug in
> gcc git) shows a few regressions.

I do not say which version is right but there should be some more settlement
on why it is done.


> Debugging one of these showed that there were some extra spaces in
> c-exp.y, that caused problems in this test case.  This patch removes the
> spaces.

"Extra" why?  There seem to be missing spaces vs. the demanger now.


It has caused regression for -gstabs+ (tested only on Fedora Rawhide):
-PASS: gdb.cp/cplusfuncs.exp: print &foo::operator delete(void*)
-PASS: gdb.cp/cplusfuncs.exp: print &foo::operator delete(void*)
+FAIL: gdb.cp/cplusfuncs.exp: print &foo::operator delete(void*)
+FAIL: gdb.cp/cplusfuncs.exp: print &foo::operator delete(void*)

I do not say -gstabs+ is required but it may suggest something is wrong.

69a179f1518fae5168238f8775ff46013d0d155e is the first bad commit
commit 69a179f1518fae5168238f8775ff46013d0d155e
Author: Tom Tromey <tromey@redhat.com>
Date:   Mon Jun 18 20:23:30 2012 +0000

    	* c-exp.y (operator): Remove trailing space after "delete" and
    	"delete[]".

Maybe the right fix is elsewhere in GDB?


Thanks,
Jan

Re: Regression for -gstabs+ [Re: FYI: fix spelling of "delete" in c-exp.y]

[Tom Tromey]

Jan> this exactly reverts my change:
Jan> 	[WIP patch] Fix crash regressions after libiberty/ update
Jan> 	http://sourceware.org/ml/gdb-patches/2012-01/msg00261.html

Jan> I made there this change to match libiberty/cp-demangle.c
Jan> cplus_demangle_operators[].

Sorry.
I'll revert it and figure out a different fix.

Why are we spending any time at all on stabs and C++?  IIRC that never
really worked that well, and stabs are past obsolete.

>> Debugging one of these showed that there were some extra spaces in
>> c-exp.y, that caused problems in this test case.  This patch removes the
>> spaces.

Jan> "Extra" why?  There seem to be missing spaces vs. the demanger now.

I will debug it again, but right now I don't understand this.
The demangler will add argument types as well.
When can you end up with a symbol name with a trailing space?

Tom

Re: Regression for -gstabs+ [Re: FYI: fix spelling of "delete" in c-exp.y]

[Jan Kratochvil]

On Tue, 19 Jun 2012 16:09:51 +0200, Tom Tromey wrote:
> Why are we spending any time at all on stabs and C++?  IIRC that never
> really worked that well, and stabs are past obsolete.

If we understand why is it as it is (currently I do not) I have no problem
with anything about stabs.  -gstabs+ already had/has many other C++ failures.

I will also look at it more.


Thanks,
Jan

Re: Regression for -gstabs+ [Re: FYI: fix spelling of "delete" in c-exp.y]

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> I'll revert it and figure out a different fix.

Ok, I take that back.

I did some more debugging and I think my fix is correct.

First, abstractly, it does not make sense to try to look up a name with
a trailing space.  No such symbol ever exists -- partial symbols have
the argument (and any possible space) stripped, and full symbols have
the arguments.

Second, as noted earlier, psymtab_search_name will not strip this
trailing space.  (And why should it?  It is invalid.)

Third, the actual failure occurs in value_struct_elt_for_reference,
in the loop over function fields:

      if (t_field_name && strcmp (t_field_name, name) == 0)

Here the function field is:

(top-gdb) p t_field_name
$27 = 0x2158539 "operator delete"

But we are searching for:

(top-gdb) p name
$23 = 0x2767c10 "operator delete "

Now, arguably perhaps this spot should use strcmp_iw.  However,
strcmp_iw is evil; and secondly, there ought to be no reason to do so,
because the other lookup paths ensure that the search name doesn't have
a trailing space.

I think your earlier patch was correct, but the c-exp.y part was just
not needed.

I couldn't reproduce any crashing failure using -gstabs+ with the
current tree (I tried the two .exp files you mentioned in your original
post).

So in sum, I'm leaving things as they are.

Tom