Re: [PATCH 0/2] Demangler crash handler

[Pedro Alves <palves@redhat.com>]

On 05/14/2014 05:08 PM, Andrew Burgess wrote:
> On 14/05/2014 3:01 PM, Pedro Alves wrote:
>> On 05/10/2014 09:55 PM, Florian Weimer wrote:
>>> * Mark Kettenis:
>>>
>>>> No.  It's this skind of duct-tape that will make sure that bugs in the
>>>> demangler won't get fixed.  Apart from removing the incentive to fix
>>>> the bugs, these SIGSEGV signal handlers make actually fixing the bugs
>>>> harder as you won't have core dumps.
>>>
>>> I find this approach extremely odd as well.
>>
>> I have to admit I'm not super keen on using signals for this either.
>> For one, not all bugs trigger segmentation faults.  Then stealing
>> a signal handler always has multi-threading considerations.  E.g.,
>> gdb Python code could well spawn a thread that happens to call
>> something that wants its own SIGSEGV handler...  Signal handlers
>> are per-process, not per-thread.
>>
>> How about we instead add a new hook to the demangler interface,
>> that allows registering a callback that has the prototype of
>> gdb's internal_error?
> 
> I thought that if the demangler couldn't demangle a symbol you
> just got back NULL indicating no demangle was possible.

Well, that's fine, and I think that it's a matter that can
be changed independently of the scheme used to detect bad state
in the demangled.  For instance, we can have GDB's
demangler_internal_error callback throw a normal error,
and then catch it from within gdb_demangle, and have that return
NULL.

> 
> Given that, it's not clear to me where you'd want to use the error
> handler, if you know something can't be demangled then you'd return
> NULL, but if some feature wasn't implemented yet then surely you're
> still better returning NULL than using the error handler, at least
> that way the user of the demangler will continue using the mangled
> version of the symbol.
> 
> I'm not arguing _for_ catching SEGV, I just think that an error handler
> only helps with known bad states, the problem is that I think in all
> known bad states the demangler should just return NULL, it's the
> unknown bad states that are an issue here.

Well, the idea is about protecting against really bad state,
not unimplemented features.  Such a mechanism would be used
just like gdb's assertions.  E.g.,

#define d_assert(expr)                                                      \
  ((void) ((expr) ? 0 :                                                       \
           (d_assert_fail (#expr, __FILE__, __LINE__, FUNCTION_NAME), 0)))

and then:

 d_assert (...->index >= 0);
 d_assert (...->count >= 0);
 d_assert (len >= 0);

 d_assert (ptr != NULL)

 d_assert (!bad_recursion);

etc.  That seems much easier and natural to write then a bunch
of error-return style handling, which may require changing
function's prototypes.

Having the libgcc/libstdc++ versions abort on broken state
(but not on bad symbols!) is I think just fine.  We should
really prevent that with better testing, e.g., the
demangle-the-world testing, and/or fuzzy testing.

So I could see even the hook disappearing and the demangler
sigsetjmp/siglongjmp itself internally in the entry point
GDB uses (but not on libstdc++'s) and then returning NULL on
broken state.  That'd avoid adding a hook that effectively won't
ever go away, even if in reality it might be or become unnecessary.

I do wonder whether the demangler quality issue isn't being
blown out of proportion though.  I think further investments
in better testing/coverage would be much better and important
than all this bug swallowing...  I think the pay off of e.g.,
running through all symbols in a distro is higher, as we're
likely to catch earlier.  Yes, it's not mutually exclusive,
but in my mind, having something like that done routinely
effectively ups the quality assurance by a large margin.

-- 
Pedro Alves