From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 117804 invoked by alias); 24 Aug 2015 08:43:00 -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 117788 invoked by uid 89); 24 Aug 2015 08:42:59 -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,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_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 (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 24 Aug 2015 08:42:58 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id EB202AA9 for ; Mon, 24 Aug 2015 08:42:56 +0000 (UTC) Received: from blade.nx (ovpn-116-87.ams2.redhat.com [10.36.116.87]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t7O8gu2K004575; Mon, 24 Aug 2015 04:42:56 -0400 Received: by blade.nx (Postfix, from userid 1000) id AF9B12622EE; Mon, 24 Aug 2015 09:42:55 +0100 (BST) Date: Mon, 24 Aug 2015 08:43:00 -0000 From: Gary Benson To: Sergio Durigan Junior Cc: GDB Patches Subject: Re: [PATCH 2/2] Catching errors on probes-based dynamic linker interface Message-ID: <20150824084255.GA16508@blade.nx> References: <1440200253-28603-1-git-send-email-sergiodj@redhat.com> <1440200253-28603-3-git-send-email-sergiodj@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1440200253-28603-3-git-send-email-sergiodj@redhat.com> X-IsSubscribed: yes X-SW-Source: 2015-08/txt/msg00662.txt.bz2 Sergio Durigan Junior wrote: > This patch is intended to make the interaction between the > probes-based dynamic linker interface and the SystemTap SDT probe > code on GDB more robust. It does that by wrapping the calls to the > probe API with TRY...CATCH'es, so that any exception thrown will be > caught and handled properly. Thanks for doing this! > diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c > index 1fb07d5..028c3d0 100644 > --- a/gdb/solib-svr4.c > +++ b/gdb/solib-svr4.c > @@ -1786,7 +1786,17 @@ solib_event_probe_action (struct probe_and_action *pa) > arg0: Lmid_t lmid (mandatory) > arg1: struct r_debug *debug_base (mandatory) > arg2: struct link_map *new (optional, for incremental updates) */ > - probe_argc = get_probe_argument_count (pa->probe, frame); > + TRY > + { > + probe_argc = get_probe_argument_count (pa->probe, frame); > + } > + CATCH (ex, RETURN_MASK_ERROR) > + { > + exception_print (gdb_stderr, ex); > + probe_argc = 0; > + } > + END_CATCH > + > if (probe_argc == 2) > action = FULL_RELOAD; > else if (probe_argc < 2) Maybe this would be clearer and more robust: TRY { unsigned probe_argc; probe_argc = get_probe_argument_count (pa->probe, frame); if (probe_argc == 2) action = FULL_RELOAD; else if (probe_argc < 2) action = PROBES_INTERFACE_FAILED; } CATCH (ex, RETURN_MASK_ERROR) { exception_print (gdb_stderr, ex); action = PROBES_INTERFACE_FAILED; } END_CATCH > @@ -1940,7 +1950,17 @@ svr4_handle_solib_event (void) > usm_chain = make_cleanup (resume_section_map_updates_cleanup, > current_program_space); > > - val = evaluate_probe_argument (pa->probe, 1, frame); > + TRY > + { > + val = evaluate_probe_argument (pa->probe, 1, frame); > + } > + CATCH (ex, RETURN_MASK_ERROR) > + { > + exception_print (gdb_stderr, ex); > + val = NULL; > + } > + END_CATCH > + > if (val == NULL) > { > do_cleanups (old_chain); This is ok. > @@ -1971,7 +1991,17 @@ svr4_handle_solib_event (void) > > if (action == UPDATE_OR_RELOAD) > { > - val = evaluate_probe_argument (pa->probe, 2, frame); > + TRY > + { > + val = evaluate_probe_argument (pa->probe, 2, frame); > + } > + CATCH (ex, RETURN_MASK_ERROR) > + { > + exception_print (gdb_stderr, ex); > + val = NULL; > + } > + END_CATCH > + > if (val != NULL) > lm = value_as_address (val); > This failure will not cause the probes interface to be disabled. FULL_RELOAD is an ok thing to do here, but it could be difficult to debug in future (if this one probe argument is broken GDB will be very much slower than it could be.) So maybe this should be: CATCH (ex, RETURN_MASK_ERROR) { exception_print (gdb_stderr, ex); do_cleanups (old_chain); return; } END_CATCH As an aside it would clarify this code greatly if "old_chain" were renamed "disable_probes_interface" or similar. It took me a while to figure out what the code was doing, and I wrote it! Cheers, Gary -- http://gbenson.net/