From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18108 invoked by alias); 28 Mar 2011 21:17:07 -0000 Received: (qmail 18098 invoked by uid 22791); 28 Mar 2011 21:17:06 -0000 X-SWARE-Spam-Status: No, hits=-1.5 required=5.0 tests=AWL,BAYES_00,SPF_SOFTFAIL,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from e5.ny.us.ibm.com (HELO e5.ny.us.ibm.com) (32.97.182.145) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 28 Mar 2011 21:17:01 +0000 Received: from d01dlp02.pok.ibm.com (d01dlp02.pok.ibm.com [9.56.224.85]) by e5.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p2SKpWUK004195 for ; Mon, 28 Mar 2011 16:51:33 -0400 Received: from d01relay01.pok.ibm.com (d01relay01.pok.ibm.com [9.56.227.233]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id 0F5F46E8039 for ; Mon, 28 Mar 2011 17:17:00 -0400 (EDT) Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by d01relay01.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p2SLGxgn372136 for ; Mon, 28 Mar 2011 17:16:59 -0400 Received: from d01av04.pok.ibm.com (loopback [127.0.0.1]) by d01av04.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p2SLGxNG029877 for ; Mon, 28 Mar 2011 17:16:59 -0400 Received: from [9.12.224.197] ([9.12.224.197]) by d01av04.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id p2SLGvHS029668; Mon, 28 Mar 2011 17:16:58 -0400 Subject: Re: [RFA] Refactor breakpoint_re_set_one From: Thiago Jung Bauermann To: Joel Brobecker Cc: gdb-patches ml In-Reply-To: <20110328184647.GD3670@adacore.com> References: <1301322874.2433.0.camel@hactar> <20110328184647.GD3670@adacore.com> Content-Type: text/plain; charset="UTF-8" Date: Tue, 29 Mar 2011 01:40:00 -0000 Message-ID: <1301347008.2433.11.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/msg01151.txt.bz2 Hi Joel, Thanks for the review! On Mon, 2011-03-28 at 11:46 -0700, Joel Brobecker wrote: > > 2011-03-27 Thiago Jung Bauermann > > > > * breakpoint.c (breakpoint_re_set_one): Factor out breakpoint-resetting > > code from here ... > > (reset_breakpoint): ... to here ... > > (addr_string_to_sals): ... and here. > > I didn't really verify line by line that you just extracted out > the code without actually changing something, but I think we can > trust your copy/paste tool :-). :-) > No real issue on my end of things (just a few little nits). > But I'm no longer a specialist of this area, so I'd wait a little > to see if anyone else might have some comments (say, until the > end of the week?). Sure, I'll wait. > My comments below: > > > +/* Find the SaL locations corresponding to the given addr_string. */ > > By convention `addr_string' should be capitalized. It's one of these > things I really wonder why we do it, but anyways... Right. I forgot about that. > > + /* 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 > ^^^^ doesn't > > + 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 > ^^^^^ doesn't > > + errors. */ Fixed. > > +/* Reset a hardware or software breakpoint. */ > > + > > +static void > > +reset_breakpoint (struct breakpoint *b) > > My only comment is that `reset' is a little ambiguous, but maybe > that's just me. I often think of "reset" as set back to original > value. I like the use of `re_set' in breakpoint_re_set_one, so > what do you think of doing the same here? Also, a more descriptive > description of the function would be useful, IMO. You have a point. I renamed it to re_set_breakpoint, and expanded the description a bit. What do you think? -- []'s Thiago Jung Bauermann IBM Linux Technology Center 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. Index: gdb.git/gdb/breakpoint.c =================================================================== --- gdb.git.orig/gdb/breakpoint.c 2011-03-28 18:13:43.000000000 -0300 +++ gdb.git/gdb/breakpoint.c 2011-03-28 18:14:28.000000000 -0300 @@ -10484,6 +10484,116 @@ update_breakpoint_locations (struct brea 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); + } + + /* 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 doesn't want to see more + errors. */ + if (e.reason < 0 && (!not_found || (!b->condition_not_parsed + && !(b->loc && b->loc->shlib_disabled) + && b->enable_state != bp_disabled))) + { + /* 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. */ @@ -10493,14 +10603,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) { @@ -10524,82 +10626,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); - } - - /* 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 (e.reason < 0 && (!not_found || (!b->condition_not_parsed - && !(b->loc && b->loc->shlib_disabled) - && b->enable_state != bp_disabled))) - { - /* 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: @@ -10679,7 +10706,6 @@ breakpoint_re_set_one (void *bint) break; } - do_cleanups (cleanups); return 0; }