[RFA]: Revised C++ patch

Elena Zannoni ezannoni@cygnus.com
Mon Oct 2 18:48:00 GMT 2000


Daniel Berlin writes:
 > >  > > 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.

If the 2 patches were submitted independently so should have been the
corresponding ChangeLogs. Could you add the valops.c changelog entry
before we all forget?

 > 
 > > 
 > >  > > 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.
 > 

Cool.

 > > 
 > >  > 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.

Still hadn't had a chance to look at it. Hopefully tomorrow.

 > > 
 > >  > > 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.
 > 

OK, I see. OK, check this bit in, but since it is still in the
symtab.c file, would you mind reposting the patch, just for
completeness' sake?

 > >  > > 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.
 > 

I think that's reasonable.

 > > 
 > >  > > 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.

Thanks.

 >  > >  > 
 > >  > 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.
 > 

OK. Good.

 > >  > 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.
 > 

Good.

 > > 
 > >  > 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
 > > 


More information about the Gdb-patches mailing list