[PATCH] libiberty: Initialize d_printing in all cplus_demangle_* functions.
Pedro Alves
palves@redhat.com
Tue Mar 14 10:50:00 GMT 2017
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
More information about the Gdb-patches
mailing list