From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1359 invoked by alias); 31 Mar 2011 14:37:36 -0000 Received: (qmail 1152 invoked by uid 22791); 31 Mar 2011 14:37:33 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00,MAY_BE_FORGED,SPF_SOFTFAIL,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from e1.ny.us.ibm.com (HELO e1.ny.us.ibm.com) (32.97.182.141) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 31 Mar 2011 14:37:28 +0000 Received: from d01dlp01.pok.ibm.com (d01dlp01.pok.ibm.com [9.56.224.56]) by e1.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p2VERAgk025828 for ; Thu, 31 Mar 2011 10:27:11 -0400 Received: from d01relay02.pok.ibm.com (d01relay02.pok.ibm.com [9.56.227.234]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 7BA0738C803C for ; Thu, 31 Mar 2011 10:37:20 -0400 (EDT) Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by d01relay02.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p2VEbQYC468982 for ; Thu, 31 Mar 2011 10:37:27 -0400 Received: from d03av04.boulder.ibm.com (loopback [127.0.0.1]) by d03av04.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p2VEbQi5001198 for ; Thu, 31 Mar 2011 08:37:26 -0600 Received: from [9.18.235.101] (dyn531364.br.ibm.com [9.18.235.101] (may be forged)) by d03av04.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id p2VEbOdD001051; Thu, 31 Mar 2011 08:37:25 -0600 Subject: Re: [RFA] Refactor breakpoint_re_set_one From: Thiago Jung Bauermann To: Ulrich Weigand Cc: Joel Brobecker , gdb-patches ml In-Reply-To: <201103291221.p2TCLYGp020110@d06av02.portsmouth.uk.ibm.com> References: <201103291221.p2TCLYGp020110@d06av02.portsmouth.uk.ibm.com> Content-Type: text/plain; charset="UTF-8" Date: Thu, 31 Mar 2011 14:46:00 -0000 Message-ID: <1301582242.9154.4.camel@hactar> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-Content-Scanned: Fidelis XPS MAILER X-IsSubscribed: yes 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: 2011-03/txt/msg01215.txt.bz2 On Tue, 2011-03-29 at 14:21 +0200, Ulrich Weigand wrote: > Joel Brobecker wrote: > > > 2011-03-28 Thiago Jung Bauermann > > > > > > * breakpoint.c (breakpoint_re_set_one): Factor out breakpoint-resetting > > > code from here ... > > > (re_set_breakpoint): ... to here ... > > > (addr_string_to_sals): ... and here. > > > > This looks very good to me! > > FWIW this looks good to me as well ... Great, thanks! I committed the patch yesterday, but unexpectedly had to leave in a hurry and wasn't able to mention the commit. Sorry about that. This is the version I committed, which doesn't depend on the cleanup patch I mentioned earlier. That patch remains valid, though. -- []'s Thiago Jung Bauermann IBM Linux Technology Center 2011-03-30 Thiago Jung Bauermann * breakpoint.c (breakpoint_re_set_one): Factor out breakpoint-resetting code from here ... (re_set_breakpoint): ... to here ... (addr_string_to_sals): ... and here. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index c300df9..f07ee10 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -10576,6 +10576,123 @@ update_breakpoint_locations (struct breakpoint *b, update_global_location_list (1); } +/* Find the SaL locations corresponding to the given ADDR_STRING. + On return, FOUND will be 1 if any SaL was found, zero otherwise. */ + +static struct symtabs_and_lines +addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found) +{ + char *s; + int marker_spec, not_found; + struct symtabs_and_lines sals; + struct gdb_exception e; + + s = addr_string; + marker_spec = b->type == bp_static_tracepoint && is_marker_spec (s); + + TRY_CATCH (e, RETURN_MASK_ERROR) + { + if (marker_spec) + { + sals = decode_static_tracepoint_spec (&s); + if (sals.nelts > b->static_trace_marker_id_idx) + { + sals.sals[0] = sals.sals[b->static_trace_marker_id_idx]; + sals.nelts = 1; + } + else + error (_("marker %s not found"), b->static_trace_marker_id); + } + else + sals = decode_line_1 (&s, 1, (struct symtab *) NULL, 0, + NULL, ¬_found); + } + if (e.reason < 0) + { + int not_found_and_ok = 0; + /* For pending breakpoints, it's expected that parsing will + fail until the right shared library is loaded. User has + already told to create pending breakpoints and don't need + extra messages. If breakpoint is in bp_shlib_disabled + state, then user already saw the message about that + breakpoint being disabled, and don't want to see more + errors. */ + if (not_found + && (b->condition_not_parsed + || (b->loc && b->loc->shlib_disabled) + || b->enable_state == bp_disabled)) + not_found_and_ok = 1; + + if (!not_found_and_ok) + { + /* We surely don't want to warn about the same breakpoint + 10 times. One solution, implemented here, is disable + the breakpoint on error. Another solution would be to + have separate 'warning emitted' flag. Since this + happens only when a binary has changed, I don't know + which approach is better. */ + b->enable_state = bp_disabled; + throw_exception (e); + } + } + + if (!not_found) + { + gdb_assert (sals.nelts == 1); + + resolve_sal_pc (&sals.sals[0]); + if (b->condition_not_parsed && s && s[0]) + { + char *cond_string = 0; + int thread = -1; + int task = 0; + + find_condition_and_thread (s, sals.sals[0].pc, + &cond_string, &thread, &task); + if (cond_string) + b->cond_string = cond_string; + b->thread = thread; + b->task = task; + b->condition_not_parsed = 0; + } + + if (b->type == bp_static_tracepoint && !marker_spec) + sals.sals[0] = update_static_tracepoint (b, sals.sals[0]); + } + + *found = !not_found; + + return sals; +} + +/* Reevaluate a hardware or software breakpoint and recreate its locations. + This is necessary after symbols are read (e.g., an executable or DSO + was loaded, or the inferior just started). */ + +static void +re_set_breakpoint (struct breakpoint *b) +{ + int found; + struct symtabs_and_lines sals; + struct symtabs_and_lines expanded = {0}; + struct cleanup *cleanups = make_cleanup (null_cleanup, NULL); + + input_radix = b->input_radix; + save_current_space_and_thread (); + switch_to_program_space_and_thread (b->pspace); + set_language (b->language); + + sals = addr_string_to_sals (b, b->addr_string, &found); + if (found) + { + make_cleanup (xfree, sals.sals); + expanded = expand_line_sal_maybe (sals.sals[0]); + } + + update_breakpoint_locations (b, expanded); + do_cleanups (cleanups); +} + /* Reset a breakpoint given it's struct breakpoint * BINT. The value we return ends up being the return value from catch_errors. Unused in this case. */ @@ -10585,14 +10702,6 @@ breakpoint_re_set_one (void *bint) { /* Get past catch_errs. */ struct breakpoint *b = (struct breakpoint *) bint; - int not_found = 0; - int *not_found_ptr = ¬_found; - struct symtabs_and_lines sals = {0}; - struct symtabs_and_lines expanded = {0}; - char *s; - struct gdb_exception e; - struct cleanup *cleanups = make_cleanup (null_cleanup, NULL); - int marker_spec = 0; switch (b->type) { @@ -10617,89 +10726,7 @@ breakpoint_re_set_one (void *bint) return 0; } - input_radix = b->input_radix; - s = b->addr_string; - - save_current_space_and_thread (); - switch_to_program_space_and_thread (b->pspace); - - marker_spec = b->type == bp_static_tracepoint && is_marker_spec (s); - - set_language (b->language); - TRY_CATCH (e, RETURN_MASK_ERROR) - { - if (marker_spec) - { - sals = decode_static_tracepoint_spec (&s); - if (sals.nelts > b->static_trace_marker_id_idx) - { - sals.sals[0] = sals.sals[b->static_trace_marker_id_idx]; - sals.nelts = 1; - } - else - error (_("marker %s not found"), b->static_trace_marker_id); - } - else - sals = decode_line_1 (&s, 1, (struct symtab *) NULL, 0, - NULL, not_found_ptr); - } - if (e.reason < 0) - { - int not_found_and_ok = 0; - /* For pending breakpoints, it's expected that parsing will - fail until the right shared library is loaded. User has - already told to create pending breakpoints and don't need - extra messages. If breakpoint is in bp_shlib_disabled - state, then user already saw the message about that - breakpoint being disabled, and don't want to see more - errors. */ - if (not_found - && (b->condition_not_parsed - || (b->loc && b->loc->shlib_disabled) - || b->enable_state == bp_disabled)) - not_found_and_ok = 1; - - if (!not_found_and_ok) - { - /* We surely don't want to warn about the same breakpoint - 10 times. One solution, implemented here, is disable - the breakpoint on error. Another solution would be to - have separate 'warning emitted' flag. Since this - happens only when a binary has changed, I don't know - which approach is better. */ - b->enable_state = bp_disabled; - throw_exception (e); - } - } - - if (!not_found) - { - gdb_assert (sals.nelts == 1); - - resolve_sal_pc (&sals.sals[0]); - if (b->condition_not_parsed && s && s[0]) - { - char *cond_string = 0; - int thread = -1; - int task = 0; - - find_condition_and_thread (s, sals.sals[0].pc, - &cond_string, &thread, &task); - if (cond_string) - b->cond_string = cond_string; - b->thread = thread; - b->task = task; - b->condition_not_parsed = 0; - } - - if (b->type == bp_static_tracepoint && !marker_spec) - sals.sals[0] = update_static_tracepoint (b, sals.sals[0]); - - expanded = expand_line_sal_maybe (sals.sals[0]); - } - - make_cleanup (xfree, sals.sals); - update_breakpoint_locations (b, expanded); + re_set_breakpoint (b); break; case bp_watchpoint: @@ -10780,7 +10807,6 @@ breakpoint_re_set_one (void *bint) break; } - do_cleanups (cleanups); return 0; }