[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