[PATCH][RFC] Malloc-less demangler interface

[Ian Lance Taylor]

simonb@google.com (Simon Baldwin) writes:

> diff -rcp3 orig/include/demangle.h new/include/demangle.h
> *** orig/include/demangle.h	Wed Dec 27 12:03:11 2006
> --- new/include/demangle.h	Tue Jan  9 10:01:25 2007
> *************** cplus_demangle_print (int options,
> *** 529,534 ****
> --- 529,555 ----
>                         int estimated_length,
>                         size_t *p_allocated_size);
>   
> + /* This function takes a struct demangle_component tree and passes back
> +    a demangled string in one or more calls to a callback function.
> +    The first argument is DMGL_* options.  The second is the tree to
> +    demangle.  The third is an opaque value used as a callback argument.
> +    The fourth is a pointer to a callback function, called with opaque
> +    as its first argument, and its second and third arguments an element
> +    of the demangled string and its length.  The callback is called once
> +    or more to return the full demangled string; the demangled element
> +    string is always nul-terminated, although its length is also provided
> +    for convenience.  In contrast to cplus_demangle_print(), this function
> +    does not allocate heap memory to grow output strings (except perhaps
> +    where alloca() is implemented by malloc()), and so is normally safe
> +    for use where the heap has been corrupted.  On success, this function
> +    returns 1; on failure, 0.  */
> + 
> + typedef void (*demangle_callbackref) (void*, const char*, size_t);
> + extern int
> + cplus_demangle_callback (int options,
> +                          const struct demangle_component *tree,
> +                          void *opaque, demangle_callbackref);
> + 

I think the function 'cplus_demangle_callback' should take a char*
parameter rather than a struct demangle_component*.  That will be
parallel to 'cplus_demangle'.  I think the function here named
'cplus_demangle_callback' should instead be named
'cplus_demangle_print_callback', since it is parallel to
'cplus_demangle_print'.

Then in cp-demangle.c, when IN_LIBGCC2 or IN_GLIBCPP_V3 is defined,
'cplus_demangle_callback' should be renamed to
'__gcclibcxx_demangle_callback' (following Andrew's suggestion).

Also: the gcc convention (e.g., walk_tree) is to pass the callback
function first, and then the opaque argument.  And in the callback
function, the opaque argument is last.  So, although I know this is
tedious, I think this should be written as

typedef void (*demangle_callbackref) (const char *, size_t, void *);
extern int cplus_demangle_print_callback (int options,
                                          const struct demangle_component *tree,
                                          demangle_callbackref, void *);

And, this is C code, not C++: write pointers as "type *", not "type*".

> + #ifdef HAVE_ALLOCA_H
> + # include <alloca.h>
> + #else
> + # ifndef alloca
> + #  ifdef __GNUC__
> + #   define alloca __builtin_alloca
> + #  else
> + extern char *alloca ();
> + #  endif /* __GNUC__ */
> + # endif /* alloca */
> + #endif /* HAVE_ALLOCA_H */
> + 

For horrible reasons which are simply too awful to get into, code
which may call alloca must put this before any non-comment code
(including any #include) in the file:

#if defined (_AIX) && !defined (__GNUC__)
 #pragma alloca
#endif

See, e.g., libiberty/putenv.c.


> *************** d_identifier (struct d_info *di, int len
> *** 1333,1339 ****
>   #define NL(s) s, (sizeof s) - 1
>   
>   CP_STATIC_IF_GLIBCPP_V3
> ! const struct demangle_operator_info cplus_demangle_operators[] =
>   {
>     { "aN", NL ("&="),        2 },
>     { "aS", NL ("="),         2 },
> --- 1362,1369 ----
>   #define NL(s) s, (sizeof s) - 1
>   
>   CP_STATIC_IF_GLIBCPP_V3
> ! const struct demangle_operator_info
> ! cplus_demangle_operators[D_OPERATORS_COUNT] =
>   {
>     { "aN", NL ("&="),        2 },
>     { "aS", NL ("="),         2 },

Why introduce D_OPERATORS_COUNT?  I'd prefer to leave it out if we
don't need it.


> !   newbuf = (char *) realloc (dgs->buf, newalc);

This shows a good faith in a standards-conformant realloc: that it
will handle a NULL first argument correctly.  I think I'm OK with that
at this point, but I wanted to call it out in case anybody reading
this finds that problematic.


> !   if (dgs->allocation_failure == 1)
> !     return;

dgs->allocation_failure is logically speaking a boolean variable, and
it only takes on the values 0 and 1.  I think it would be clearer to
write
  if (dgs->allocation_failure)
here and in a couple of other places.


Patch is OK with this niggling changes.  Thanks and sorry for the
delay.

Ian