Re: [RFA]: Revised C++ patch

[Daniel Berlin <dberlin at cygnus dot com>]

> 
> OK, Dan, thanks for the patches. 
> Sorry it took so long to get back to you on them.
> 
> However, I am still quite not sure about the problem that we are
> trying to fix.  Looking back at the first message you sent out,
> you mention 4 different issues this patch is supposed to address:
> 
> [http://sources.redhat.com/ml/gdb-patches/2000-08/msg00199.html]
> 
>  > 1. Removes the remaining use of linear search through symbol tables
>  > when there was a C++ symbol in the symtab. This means that rather than
>  > search through 1,000,000 symbols to find out we don't have a
>  > particular C++ symbol, we now search 20.  Trust me when i tell you
>  > this is a significant speedup.
>  > 2. Fixes a bad problem with respect to STABS debugging in C++. This
>  > was causing massive testsuite failures, as well as the inability to
>  > call operator functions.
>  > 3. Speed up symbol searching even more by makeing sure we don't check
>  > the same block twice in a given global lookup (before we would check
>  > the same block, over and over again, for the same symbol, inside one
>  > particular loop. Now i just make sure we only check a given block once
>  > inside that loop).
>  > 4. Fix completion of C++ names. Now you can properly complete without
>  > quotes, even when you have space in the name, assuming the space is in
>  > a template or function argument list.
> 
> 
> A general rule for patch submissions is that one patch should address
> one problem, so that it is easier to review, and to back out in case
> there are regressions.  Here I am seeing 2 enhancements and 2 bug
> fixes all in one single patch.
2 and 4 can be split out, 1 and 3 happened to go together.
I did it all on the same day originally, so i submitted them altogether.
> 
> I would prefer if you could split this up into 4 different patches that
> could be checked in separately.
Sure, if you like.
> [More comments below]
>  
>  > Changelog entry:
>  > 2000-09-12  Daniel Berlin  <dberlin@redhat.com>
>  >  
>  >  	* symtab.c: Add lookup_symbol_aux prototype, and make it static.
>  >  	(lookup_symbol): Made into wrapper for lookup_symbol_aux, so we
>  >  	can perform case sensitivity/demangling without leaking memory.
>  >  	Moved code to do demangling/case sensitivity from
>  >  	old_lookup_symbol to here.
>  >  	(lookup_symbol_aux): Renamed from lookup_symbol. Removed code to
>  >  	do demangling/case sensitivity. Now done in new lookup_symbol.
>  >  
>  >   2000-09-12  Alexandre Oliva  <aoliva@redhat.com>
>  >   
>  >   	* MAINTAINERS: Added myself.
>  > *************** Fri Aug 25 12:03:15 2000  David Taylor  
>  > *** 487,492 ****
>  > --- 497,522 ----
>  >   	* value.h (struct value) <lazy>: Add a comment about its use for
>  >           watchpoints.
>  >   	
>  >  2000-08-10  Daniel Berlin  <dberlin@redhat.com>
>  >  
>  >  	* valops.c (typecmp): Properly handle reference arguments.
>  >  
>  >  	* symtab.h (OPNAME_PREFIX_P): Change operator prefix to correct value.
>  >  
>  >  	* symtab.c (decode_line_1): Properly handle complex templates
>  >  	without quoting.
>  >  	(completion_list_add_name): Fix completion problems
>  >  	with C++ completions. 
>  >  	(lookup_partial_symbol): Remove linear search for C++.
>  >  	(lookup_symbol): Record blocks we've already checked. Also,
>  >  	demangle names before searching. 
>  >  	(lookup_block_symbol): Stop using linear search for C++.
>  >  	(gdb_mangle_name): Properly handle operators.
>  >  
>  >  	* symfile.c (compare_symbols): Use SYMBOL_SOURCE_NAME instead of
>  >  	SYMBOL_NAME. 
>  >  	(compare_psymbols): Same here.
>  >  	
>  > 
> 
> 
> I am extremely confused by these changelog entries. Which ones are the
> real ones? You have listed changes for valops.c that are not included
> in the diffs.
Both are.
The valops.c changes are there  because they were already approved and 
put in.  I didn't expect to have to wait so long for approval. It was all 
originally part of the same patch, way back when.
> Also, why is this split  over 2 different  ChangeLogs entries? > 
Because it ook over a month to get approval, and i made revisions.
 > Some  more comments: > 
> The ChangeLogs should not be submitted as diffs.  
> 
Originally, they weren't.
I got tired of splitting them out multiple times.
> You changed the macro OPNAME_PREFIX_P, but you didn't update its
> comment.  Actually, I think this macro is used only in 2 spots,
> and it has been simplified, so, could it be eliminated?
> 
The comment still holds, and needed no update.
No, it can't be eliminated, because it might not be g++ specific in the 
future.
In fact, i can almost guarantee it will stay. When i change everything to 
stop having different variables for differnet runtime models, i'll update 
the comment.
> Did you need to introduce the goto in decode_line_1()? I would prefer
> if there was an alternative approach.
> 
Yes, I did.
There is no alternative approach anyone could think of. It would require 
splitting up decode_line_1, and i jut don't have time to do this and 
debug it. decode_line_1 is too hairy and too important to break in random 
ways by splitting it up.
We need to retry what we just did with one slightly different 
setting. If someone can point out how to do this without the goto, 
and without splitting the function,  i'm happy to listen.
> You are eliminating support for GNU mangled names in gdb_mangle_name(),
> can you explain why? (I am probably out of the loop on this one). 
> 
No i'm not, what gave you that idea. I'm fixing a problem decoding operators.
We just don't need to decode them anymore.

> What does the setting of the language to language_auto as first thing in
> SYMBOL_INIT_DEMANGLED_NAME fix?
The fact that languages don't always get set properly before it inits the 
demangled name, cuasing  massive demangling failures.
Before, it would go to deamngle it, see the language of the symbol as 
unknown, and never try to demangle it again, ever.
This is wrong, and a bad optimization.
Whether demangling works or not should not be dependent on the order in 
which you initialize the symbol structures.

 > 
> Bottom line, can you show examples of gdb behavior that was broken
> before and fixed after these changes?
Of course.
Java demangling failures, C++ demangling problems in stack backtraces, etc.
Things weren't getting demangled when they should.
> 
> Also, I still see some formatting and indentation problems.
I went back and reindented it properly later, I just never resubmitted a 
revised patch.
 > 
> This code:
>  > +   /* If we are using C++ language, demangle the name before doing a lookup, so
>  > +      we can always binary search. */
>  > +   if (current_language->la_language == language_cplus)
>  > +     {
>  > +       modified_name2 = cplus_demangle (modified_name, DMGL_ANSI |
>  > + 				       DMGL_PARAMS);
>  > +     }
>  > +   else
>  > +     {
>  > +       needtofreename=0;
>  > +     }
>  > +   
>  > +   if (!modified_name2)
>  > +     {
>  > +       modified_name2 = modified_name;
>  > +       needtofreename=0;
>  > +     }
>  > +   returnval = lookup_symbol_aux (modified_name2, block, namespace,
>  > + 				 is_a_field_of_this, symtab);
>  > +   if (needtofreename)
>  > +     free(modified_name2);
>  > + 
>  > +   return returnval;	 
>  > + }
> 
> Could be simplified if needtofreename was initialized to 0 instead of 1.

Are you positive? I tried this once, and I ended up with a memory leak, 
according to purify.

 > 
> 
> I don't understand the change to use SYMBOL_MATCHES_NAME in
> lookup_block_symbol. It doesn't check for 
> SYMBOL_NAMESPACE (sym) == namespace
> anymore.
> 
It doesn't need to in that case, the comment is right. It's impossible 
for that to occur.
So using SYMBOL_MATCHES_NAME alone will take care of it.
I didn't remove any of the other namespace checks.

> 
> OK, now that I have stared at your patches for some time, the main idea
> is to demangle the c++ name before you do the lookup, and then do a
> binary search, instead of a linear search. The lookup functions have
> been modified to make sure that we compare on the demangled name if we
> have it (by using SYMBOL_SOURCE_NAME instead of SYMBOL_NAME). Yes?
> 
Right.

And i made the block lookup changes while i was debugging to make sure it 
all worked, and noticed we lookup in the same block 100 times for no 
apparent reason.

> I just tried you patch on solaris 2.5.1 and I got one extra testsuite
> failure:  FAIL: gdb.c++/namespace.exp: info func xyzq
> 
Yup.
This is a problem in the test, actually.
If you look at the log, it clearly found the function, the regexp just 
isn't matching the output anymore for some reason.

> So, OK, I think I need to see these changes split into 4 different
> patches. And for the bug fixes I would like examples, ideally
> testsuite additions.
> 
I'd rather split it into 3.
The symbol changes, the mangle changes, and the name completion changes.
Is that okay?
I can commit number 2 without approval, actually.

It's very simple to reproduce the bugs it's  fixing, if you really want 
to see them.
For the operator names, please try creating a map or vector, and calling 
operator [] by doing
print yourmap[5]

It won't work, because it'll never find the operator, because it gets 
mangled wrong.

I have quite a few bug reports about this in my inbox somewhere if you 
really want me to be more specific.

For the demangling problem, it's a general bug that shows up in various 
places (like things not being demangled right when you do info 
functions, etc)
Do all of our C++ systems supoprt the STL yet?
If so, i'll happily add a bunch of more tets i've been working on, which 
use the STL.


 > Thanks again for your hard work. > > Elena
> 
>  >