Re: [RFA]: Revised C++ patch

[Daniel Berlin <dberlin at cygnus dot com>]

>  > > I would prefer if you could split this up into 4 different patches that
>  > > could be checked in separately.
>  > Sure, if you like.
> 
> Yes please. That would be great.
> 
>  > > [More comments below]
>  > >  
>  > > 
>  > > 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.
> 
> Ok, so, you agree with me, these ChangeLogs don't apply anymore to the
> current patch and need to be changed.

Yes, but that's a minor issue, IMHO.
I need to check in that part of the changelog regardless, because I 
wasn't expecting approval to take this long.
In other words, the valops.c entry isn't anywhere *else* right now.

> 
>  > > 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.
> 
> Sorry, I know it's a pain, but there are procedures that have to be
> followed, and I encourage you to do that.
Fine.

> 
>  > > 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.
> 
> I am referring to this bit:
> 
>    Currently 'o' 'p' CPLUS_MARKER is used for both the symbol in the
>    symbol-file and the names in gdb's symbol table.
> 
> The macro is now checking for 'operator', not for 'o', 'p', and cplus_marker.
Okay, i'll delete the three lines, since they no longer apply.

> 
>  > 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.
> 
> Yes, it is hairy, that's why I thought that a goto would make it even
> more so.
Not really, it specifically only affects template parameters.
What happens is that it strips too much because it wasn't quoted, so we 
need to tell it to pretend it was quoted, so it doesn't strip as much.
However, you only can do this in cases where it *does* strip too much, if 
you do it all the time, you break it because you'll try to look up a 
symbol that doesn't exist. Hence the goto.


 > >  > 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.
> 
> I'll take a look.

Please do. I tried for hours, then just gave up.
> 
>  > > 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.
>  > 
> 
> OK, I just saw that the specific code was gone. Based on the comment:
>   /* Only needed for GNU-mangled names.  ANSI-mangled names
>      work with the normal mechanisms.  */
> 
> Can you explain why we don't need to decode them anymore? (sorry, I am
> little thick sometimes)
It's actually that we don't need to *reencode* them anymore. 
gdb_mangle_name is trying to generate a mangled name, because in 
stabs, the mangled names that are in methods of classes, aren't full 
names. so to get the full mangled name, we need to do a little magic.
Since about egcs 1.1.2, operator names, and only operator names, are 
fully mangled names, and thus, gdb_mangle_name doesn't need to do 
magic to get the mangled name for operators.
It just has to return what we have for the mangled name already, which is 
what it now does in he fixed version.
Of course, this breaks C++ support for really old gcc, but C++ suport in 
really old GCC was already very broken on it's own, so i'm not really 
hurting it.
I plan on checking this part in today, as it's a C++ change, rather than 
a symtab change. I have no clue who put the thing in symtab.c, but it's 
clearly not symbol related.

>  > > 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.
> 
> Ah, Ok, got it. Thanks.

> 
>  > 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.
> 
> Ok, make sure you include these fixes when you split the patches into
> smaller ones.
> 
>  >  > 
>  > > 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 thought you could get rid of the 2 "needtofreeframe = 0;" bits and
> have just one "needtofreeframe = 1;" statement. 
> 
> Something like this:
> 
> 
>   if (current_language->la_language == language_cplus)
>     {
>       modified_name2 = cplus_demangle (modified_name, DMGL_ANSI |
> 				       DMGL_PARAMS);
>       if (modified_name2)
>        {
>          modified_name = modified_name2;
>          needtofreename = 1;
>        }
>     }
> 
>   returnval = lookup_symbol_aux (modified_name, block, namespace,
> 				 is_a_field_of_this, symtab);
>   if (needtofreename)
>     free (modified_name2);
> 
>   return returnval;	 
> 
> Would this work?

I think so, i'll purify it to make sure.

> 
>  > > 
>  > > 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, right. Then you need to change the comment as well because it
> doesn't apply anymore.
Okey dokey.

> 
>  > > 
>  > > 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.
> 
> Yes, that's the other enhancement.
> 
>  > 
>  > > 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.
>  > 
> 
> I think the order has changed? It could be worthwhile to investigate
> this a little further and update the test if necessary.
Yes, the order has changed.
I'll probably just break it up to try to match each function name seperately.

> 
>  > > 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.
> 
> 3 is fine, but I would prefer to see all of them resubmitted if you
> don't mind. I promise to be faster this time around.
Well, I don't need approval for number 2, as I said, because they are C++ 
fixs, rather than symbol fixes, but i'll resubmit it because that's the 
procedure.
 > >  > 
>  > 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.
>  > 
> 
> Just one example would be enough.
> 
i'll put in a testcase.

>  > 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)
> 
> Just one simple testcase would be good.
I'll put it in a testcase.

> 
>  > 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.
>  > 
> 
> Don't know.
> 
> Elena
>