Re: [PATCH] libiberty: Initialize d_printing in all cplus_demangle_* functions.

[Pedro Alves <palves@redhat.com>]

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