From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 671 invoked by alias); 25 Feb 2010 18:11:53 -0000 Received: (qmail 643 invoked by uid 22791); 25 Feb 2010 18:11:52 -0000 X-SWARE-Spam-Status: No, hits=-6.1 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,RCVD_IN_DNSWL_HI,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 25 Feb 2010 18:11:47 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o1PIBkmP026885 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 25 Feb 2010 13:11:46 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o1PIBjSP029337; Thu, 25 Feb 2010 13:11:46 -0500 Received: from opsy.redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id o1PIBiol032474; Thu, 25 Feb 2010 13:11:45 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id 901E437998C; Thu, 25 Feb 2010 11:11:44 -0700 (MST) From: Tom Tromey To: gdb-patches@sourceware.org Subject: RFC: fix bug with std::terminate handler Reply-To: tromey@redhat.com Date: Thu, 25 Feb 2010 18:11:00 -0000 Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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 X-SW-Source: 2010-02/txt/msg00625.txt.bz2 I would appreciate comments on this patch. This comes from an automatically-reported bug in the Red Hat bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=562975 call_function_by_hand installs a momentary breakpoint on std::terminate, and then deletes it later. However, this can cause a double deletion of the breakpoint. In the bug, the called function is dlopen, which causes gdb to enter solib_add, which calls breakpoint_re_set, deleting the momentary breakpoint. This fix works by creating the momentary breakpoint with an internal breakpoint number, and then trying to delete the breakpoint by number. This bug does not always manifest in a crash. In fact, I couldn't make it crash here, but I could observe the problem under valgrind. Built and regtested on x86-64 (compile farm). I also manually verified it using valgrind. I think this patch is mildly ugly, due to the introduction of set_momentary_breakpoint_at_pc_with_number. However, in the absence of comments, I plan to check it in after a reasonable waiting period. Tom 2010-02-25 Tom Tromey * infcall.c (do_delete_breakpoint_by_number): New function. (call_function_by_hand): Refer to momentary breakpoint by number. * breakpoint.h (set_momentary_breakpoint_at_pc_with_number): Declare. * breakpoint.c (set_momentary_breakpoint_at_pc_with_number): New function. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 8c97949..58c590d 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -6010,6 +6010,20 @@ set_momentary_breakpoint_at_pc (struct gdbarch *gdbarch, CORE_ADDR pc, return set_momentary_breakpoint (gdbarch, sal, null_frame_id, type); } + +/* Like set_momentary_breakpoint_at_pc, but ensure that the new + breakpoint has a number. */ + +struct breakpoint * +set_momentary_breakpoint_at_pc_with_number (struct gdbarch *gdbarch, + CORE_ADDR pc, + enum bptype type) +{ + struct breakpoint *result = set_momentary_breakpoint_at_pc (gdbarch, pc, + type); + result->number = internal_breakpoint_number--; + return result; +} /* Tell the user we have just set a breakpoint B. */ diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h index 6b373a3..cf04e2e 100644 --- a/gdb/breakpoint.h +++ b/gdb/breakpoint.h @@ -760,6 +760,9 @@ extern struct breakpoint *set_momentary_breakpoint extern struct breakpoint *set_momentary_breakpoint_at_pc (struct gdbarch *, CORE_ADDR pc, enum bptype type); +extern struct breakpoint *set_momentary_breakpoint_at_pc_with_number + (struct gdbarch *, CORE_ADDR pc, enum bptype type); + extern struct breakpoint *clone_momentary_breakpoint (struct breakpoint *bpkt); extern void set_ignore_count (int, int, int); diff --git a/gdb/infcall.c b/gdb/infcall.c index e642894..e020233 100644 --- a/gdb/infcall.c +++ b/gdb/infcall.c @@ -402,6 +402,18 @@ run_inferior_call (struct thread_info *call_thread, CORE_ADDR real_pc) return e; } +/* A cleanup function that deletes a breakpoint, if it still exists, + given the breakpoint's number. */ + +static void +do_delete_breakpoint_by_number (void *arg) +{ + int *num = arg; + struct breakpoint *bp = get_breakpoint (*num); + if (bp) + delete_breakpoint (bp); +} + /* All this stuff with a dummy frame may seem unnecessarily complicated (why not just save registers in GDB?). The purpose of pushing a dummy frame which looks just like a real frame is so that if you call a @@ -439,7 +451,8 @@ call_function_by_hand (struct value *function, int nargs, struct value **args) struct cleanup *args_cleanup; struct frame_info *frame; struct gdbarch *gdbarch; - struct breakpoint *terminate_bp = NULL; + int terminate_bp_num = 0; + CORE_ADDR terminate_bp_addr = 0; struct minimal_symbol *tm; struct cleanup *terminate_bp_cleanup = NULL; ptid_t call_thread_ptid; @@ -758,8 +771,13 @@ call_function_by_hand (struct value *function, int nargs, struct value **args) struct minimal_symbol *tm = lookup_minimal_symbol ("std::terminate()", NULL, NULL); if (tm != NULL) - terminate_bp = set_momentary_breakpoint_at_pc + { + struct breakpoint *bp; + bp = set_momentary_breakpoint_at_pc_with_number (gdbarch, SYMBOL_VALUE_ADDRESS (tm), bp_breakpoint); + terminate_bp_num = bp->number; + terminate_bp_addr = bp->loc->address; + } } /* Everything's ready, push all the info needed to restore the @@ -773,8 +791,9 @@ call_function_by_hand (struct value *function, int nargs, struct value **args) discard_cleanups (inf_status_cleanup); /* Register a clean-up for unwind_on_terminating_exception_breakpoint. */ - if (terminate_bp) - terminate_bp_cleanup = make_cleanup_delete_breakpoint (terminate_bp); + if (terminate_bp_num != 0) + terminate_bp_cleanup = make_cleanup (do_delete_breakpoint_by_number, + &terminate_bp_num); /* - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - If you're looking to implement asynchronous dummy-frames, then @@ -940,9 +959,9 @@ When the function is done executing, GDB will silently stop."), in an inferior function call. Rewind, and warn the user. */ - if (terminate_bp != NULL + if (terminate_bp_num != 0 && (inferior_thread ()->stop_bpstat->breakpoint_at->address - == terminate_bp->loc->address)) + == terminate_bp_addr)) { /* We must get back to the frame we were before the dummy call. */ @@ -991,7 +1010,7 @@ When the function is done executing, GDB will silently stop."), /* If we get here and the std::terminate() breakpoint has been set, it has to be cleaned manually. */ - if (terminate_bp) + if (terminate_bp_num != 0) do_cleanups (terminate_bp_cleanup); /* If we get here the called FUNCTION ran to completion,