This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [RFA]: Revised C++ patch
Daniel Berlin writes:
> >
> > 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.
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.
> > 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.
> > 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.
> 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.
> 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.
> > 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)
> > 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 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.
> >
> > 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.
> > 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.
>
> 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.
> 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.
> 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