[PING][PATCH][RFC] vterminate use of malloc-less demangler

[Simon Baldwin]

Any movement or further thoughts on this patch?

Thanks.


Simon Baldwin wrote:

> Benjamin Kosnik wrote:
>
>>
>>> Testing this work is, unfortunately, rather tricky.  I do have a 
>>> simple test case, appended below, that allows me to confirm manually 
>>> that __verbose_terminate_handler is operating without heap use.
>>>
>>> This test, however, is hard to fit into any test framework.  First, 
>>> it aborts whether or not it's successful; that's the nature of 
>>> changing termination code.  Second, it relies on MALLOC_CHECK_, 
>>> making it Linux only and not portable.  And third, it relies on 
>>> carnal knowledge of libc's heap organization in order to "lightly" 
>>> corrupt the heap without overdoing it.
>>>
>>> Some of these restrictions might be avoidable.  For example, 
>>> interposing malloc, realloc, free, and calloc catches calls to heap 
>>> functions in the same way as MALLOC_CHECK_, so could make the test 
>>> slightly more portable.  This still isn't fully portable across all 
>>> platforms, though, and may require platform-specific link options 
>>> even where practical.  Likewise, the abort() can be caught with a 
>>> SIGABRT handler, but again is not portable.  I experimented with a 
>>> test based on these ideas, but it's probably too complex to be 
>>> reliable.
>>>
>>> There seems to be no identifiable current vterminate.cc test I can 
>>> extend for this patch.  If you have other ideas for how to add a 
>>> suitable test to the libstdc++ test suite, please let me know.
>>
>>
>>
>> I cannot, at the moment, sorry. Also, your test case does not 
>> reproduce the issue for me, on FC6.
>>
>> I get:
>>
>> %./a.out
>> malloc: using debugging hooks
>> *** glibc detected *** ./a.out: malloc: top chunk is corrupt: 
>> 0x08d40088 ***
>
>
>
> Did you link the test program with a libstdc++ that contains my 
> patch?  If not, then you have successfully reproduced the issue -- 
> this is it.
>
> Without the patch, __cxa_demangle(), called from 
> __verbose_terminate(), will attempt to allocate memory to hold the 
> result of demangling.  Because the test program deliberately corrupts 
> the heap and sets MALLOC_CHECK_, demangling will then fail to 
> allocate, and give an error like that above.
>
> With the patch, and linking the test program to a patched libstdc++.a, 
> you'll see
>
>  %./a.out
>  malloc: using debugging hooks
>  terminate called after throwing an instance of 'std::bad_alloc'
>    what():  St9bad_alloc
>  Aborted
>
> indicating that name demangling and other termination handling still 
> function even in the presence of a corrupted and unusable heap.
>
>>>> 2) Are you planning on making this new function available to other 
>>>> users besides the verbose terminate handler? If so, the declaration 
>>>> should go below cxa_demangle in cxxabi.h and the function should be 
>>>> exported. If not, the declaration should be removed.
>>>
>>>
>>>
>>>
>>> The __gcclibcxx_demangle_callback() function is intended as a 
>>> private contract between glibc and libstdc++.  There's no plan to 
>>> use it elsewhere.  Callback-based demangling for general use is 
>>> provided by cplus_demangle_v3_callback(), supplied by libiberty's 
>>> include/demangle.h as an extension to the existing 
>>> cplus_demangle_v3().  Moving the declaration of 
>>> __gcclibcxx_demangle_callback() to cxxabi.h is also slightly 
>>> misleading, as the function isn't actually part of the defined ABI.
>>>
>>> The current declaration can't be removed if not placed in cxxabi.h 
>>> -- this would prevent the vterminate.cc from compiling.
>>
>>
>>
>> [snip]
>>
>>> There don't appear to be any other functions that are private 
>>> contracts between glibc and libstdc++, so this name seems 
>>> reasonable.  In particular, it's not part of the C++ ABI, so naming 
>>> it __cxa_... was deemed unacceptable.  (In practice, it's actually a 
>>> variant of __cxa_demangle(), but has to have a different name as the 
>>> specification of __cxa_demangle() doesn't permit malloc-less 
>>> operation).
>>
>>
>>
>> I think you should re-evaluate your approach with this patch. I think 
>> this malloc-free demangler addition should be obvious to people using 
>> the cxxabi.h interfaces.
>>
>> In overview, libiberty and libsupc++/libstdc++ share the demangler 
>> code. However, libstdc++ has an API-defined interface to the 
>> demangler, in __cxa_demangle from cxxabi.h. You're augmenting this, 
>> much like other non-ABI extensions (__cxa_cdtor_return_type, 
>> __cxa_cdtor_type), and admittedly so from your last parenthetical bit 
>> above.
>>
>> And, I think this is a good thing: clearly, there are issues with 
>> __cxa_demangle's memory management facilities. There's even been 
>> previous attempts to deal with this in libsupc++, with this. See 
>> gcc-3.4.0's struct demangle in libstdc++-v3/ext/demangle.h etc etc.
>>
>> I'm of the opinion that something like __cxa_demangle_no_alloc, ie 
>> something much simpler and direct, without all the callback goo, 
>> would be a much cleaner route to get where you want to go. Then, you 
>> could use this no-alloc routine in __verbose_terminate_handler. Or, 
>> __cxa_demangle_callback if you can convince me that there is ever 
>> going to be more than one callback...
>
>
>
> __cxa_demangle_callback() was the original name of this demangler 
> function, but this was rejected as part of the review of libiberty 
> revision 121305, which changed it to _gcclibcxx_demangle_callback().  
> The rationale is that since this function is not part of the ia64 ABI, 
> it should not be named as though it is.
>
> Please explain how you envision a demangling interface that explicitly 
> avoids both allocations and callbacks.  In particular, how should it 
> return or otherwise handle the demangled string?
>
>> In addition, I think this addition should be mentioned here:
>> http://gcc.gnu.org/onlinedocs/libstdc++/18_support/howto.html#6
>
>
>
> This is relevant only for functions that are described by cxxabi.h.  
> As noted above, this addition isn't, since it's not part of the ABI.
>
> Conventional client code access to allocation-less demangling is 
> provided by the libiberty functions cplus_demangle_v3_callback(), 
> java_demangle_v3_callback(), and cplus_demangle_print_callback(), all 
> analogous to the existing libiberty functions that return allocated 
> strings -- cplus_demangle_v3(), java_demangle_v3(), and 
> cplus_demangle_print().
>
>