From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2378 invoked by alias); 14 May 2014 18:32:45 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 2365 invoked by uid 89); 14 May 2014 18:32:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 14 May 2014 18:32:43 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s4EIWeSR021523 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 14 May 2014 14:32:40 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id s4EIWcdY013026; Wed, 14 May 2014 14:32:39 -0400 Message-ID: <5373B6C6.6060401@redhat.com> Date: Wed, 14 May 2014 18:32:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: Andrew Burgess , gdb-patches@sourceware.org Subject: Re: [PATCH 0/2] Demangler crash handler References: <20140509100656.GA4760@blade.nx> <201405091120.s49BKO1f010622@glazunov.sibelius.xs4all.nl> <87fvkhjqvs.fsf@mid.deneb.enyo.de> <53737737.2030901@redhat.com> <5373950D.7050903@broadcom.com> In-Reply-To: <5373950D.7050903@broadcom.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-05/txt/msg00212.txt.bz2 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