This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] libiberty: Initialize d_printing in all cplus_demangle_* functions.
- From: Pedro Alves <palves at redhat dot com>
- To: Mark Wielaard <mark at klomp dot org>
- Cc: Markus Trippelsdorf <markus at trippelsdorf dot de>, gdb-patches at sourceware dot org, Nathan Sidwell <nathan at acm dot org>, Ian Lance Taylor <iant at google dot com>, Nick Clifton <nickc at redhat dot com>
- Date: Tue, 14 Mar 2017 10:50:16 +0000
- Subject: Re: [PATCH] libiberty: Initialize d_printing in all cplus_demangle_* functions.
- Authentication-results: sourceware.org; auth=none
- References: <1489356354-27648-1-git-send-email-mark@klomp.org> <20170313181150.GA287@x4> <20170313182959.GC2167@stream> <a3993cdc-dbb0-e377-3e46-a84d8cf971a0@redhat.com> <1489437334.21350.72.camel@klomp.org> <f341a66c-8a47-b38c-6355-818400e9f7c7@redhat.com> <1489482296.21350.77.camel@klomp.org>
On 03/14/2017 09:04 AM, Mark Wielaard wrote:
> That looks good. But note that there is one behavioral change.
> cplus_demangle_fill_component is defined as:
>
> /* Fill in most component types with a left subtree and a right
> subtree. Returns non-zero on success, zero on failure, such as an
> unrecognized or inappropriate component type. */
>
> And gdb/cp-name-parser.y fill_comp does:
>
> i = cplus_demangle_fill_component (ret, d_type, lhs, rhs);
> gdb_assert (i);
>
> So you have an extra sanity check. Where before you might have silently
> created a bogus demangle_component. Which I assume is what you want.
Indeed, and I think it is.
> But it might depend on what gdb_assert precisely does
gdb_assert triggers the infamous:
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)
> and if the parser input is normally "sane" or not.
The only thing that is validated is that we don't create
a component with a left/right subtree when that doesn't make sense
for the component type. I think trying to create such a component
would be a parser/grammar/production bug, even with invalid input.
I had run into that assert in fill_comp yesterday actually, with a slightly
different/larger patch. At first I saw the cplus_demangle_fill_component
declaration in demangle.h, but didn't see the implementation in cp-demangle.c, and
thought that was just some oversight/left over. So I though I'd add one. I factored
out a cplus_demangle_fill_component out of cp-demangle.c:d_make_comp, following the
same pattern used by the other cplus_demangle_fill* / d_make* function pairs.
Only after did I notice that actually, there's an implementation of
cplus_demangle_fill_component in cplus-demint.c... AFAICS, that's only
used by GDB. No other tool in the binutils-gdb repo, nor GCC's repo uses it, AFAICS.
So I figured that I'd remove the cplus-demint.c implementation, in favor of
the new implementation in cp-demangle.c based on d_make_comp. And _that_ ran into the
assertion, because the implementations are exactly the same. GDB fills in some types with
NULL left components and fills them in later. For example, for DEMANGLE_COMPONENT_POINTER:
ptr_operator : '*' qualifiers_opt
- { $$.comp = make_empty (DEMANGLE_COMPONENT_POINTER);
- $$.comp->u.s_binary.left = $$.comp->u.s_binary.right = NULL;
+ { $$.comp = fill_comp (DEMANGLE_COMPONENT_POINTER, NULL, NULL);
$$.last = &d_left ($$.comp);
$$.comp = d_qualify ($$.comp, $2, 0); }
Note how cp-demangle.c:d_make_comp's validations are stricter than
cp-demint.c:cplus_demangle_fill_component's. The former only allows
lazy-filling for a few cases:
[...]
/* These are allowed to have no parameters--in some cases they
will be filled in later. */
case DEMANGLE_COMPONENT_FUNCTION_TYPE:
[...]
While cp-demint.c:cplus_demangle_fill_component, the version that
GDB uses, allows that in all cases. IOW, passing in a NULL left/right subtree
to cplus_demangle_fill_component when the component type expects one is OK, assuming
that the caller will fill them in afterwards. I crossed checked the types in
the new fill_comp calls with the checks inside cplus_demangle_fill_component now,
and I believe they're all OK.
Maybe it'd be possible to avoid this lazy filling in, but I didn't
bother trying.
Thanks,
Pedro Alves