From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17490 invoked by alias); 15 Mar 2012 15:36:36 -0000 Received: (qmail 17220 invoked by uid 22791); 15 Mar 2012 15:36:35 -0000 X-SWARE-Spam-Status: No, hits=-6.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD 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, 15 Mar 2012 15:36:19 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q2FFaIeG027378 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 15 Mar 2012 11:36:19 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q2FFaHgC032107; Thu, 15 Mar 2012 11:36:18 -0400 Message-ID: <4F620C71.8060501@redhat.com> Date: Thu, 15 Mar 2012 15:36:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.1) Gecko/20120216 Thunderbird/10.0.1 MIME-Version: 1.0 To: Sergio Durigan Junior CC: gdb-patches@sourceware.org, Tom Tromey Subject: Re: [PATCH 2/3] Implement new features needed for handling SystemTap probes References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: 2012-03/txt/msg00541.txt.bz2 I've looked this one over. See my by comments below. I've tried to skip issues already pointed out. On 03/09/2012 08:33 PM, Sergio Durigan Junior wrote: > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > +/* See the comment in breakpoint.h. */ > + > +void > +modify_semaphore (struct bp_location *loc, int set) > +{ Starting with a nit... This function name is a bit too generic for an extern function. Can we make it a bit more specific, like breakpoint_modify_stap_semaphore, or modify_stap_semaphore, or perhaps even clearer, (bp_)stap_semaphore_up/(bp_)stap_semaphore_down? I'd even suggest changing the function's prototype to take the semaphore address as argument instead, and move it elsewhere most stap things are (stap-probe.c?). On 03/09/2012 08:33 PM, Sergio Durigan Junior wrote: > +{ > + struct gdbarch *arch = loc->gdbarch; > + gdb_byte bytes[sizeof (LONGEST)]; > + /* The ABI specifies "unsigned short". */ > + struct type *type = builtin_type (arch)->builtin_unsigned_short; > + CORE_ADDR address = loc->semaphore; > + ULONGEST value; > + > + if (address == 0) > + return; > + > + /* Swallow errors. */ > + if (target_read_memory (address, bytes, TYPE_LENGTH (type)) != 0) > + return; > + > + value = extract_unsigned_integer (bytes, TYPE_LENGTH (type), > + gdbarch_byte_order (arch)); > + /* Note that we explicitly don't worry about overflow or > + underflow. */ > + if (set) > + ++value; > + else > + --value; > + > + store_unsigned_integer (bytes, TYPE_LENGTH (type), > + gdbarch_byte_order (arch), value); > + > + target_write_memory (address, bytes, TYPE_LENGTH (type)); > +} Is this racing with threads in the inferior, in non-stop mode? If so, we should find a way to fix, but a comment about it here meanwhile would be quite worth it. > val = bl->owner->ops->insert_location (bl); > + > + modify_semaphore (bl, 1); This is modifying the semaphore even if the insertion failed. Please make sure we can't reach this with the breakpoint already inserted, due to the recent target side breakpoint conditions work, in which case you should not increment the semaphore. And I think we do. > } > else > { > @@ -3153,6 +3194,8 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is) > { > /* No overlay handling: just remove the breakpoint. */ > val = bl->owner->ops->remove_location (bl); > + > + modify_semaphore (bl, 0); This is modifying the semaphore even if removal failed. Not so clear what to do in this case; things are bonkers already anyway. But, in any case, at insertion time you have: - insert breakpoint - increment semaphore so at removal, the obvious code that doesn't raise suspicions is the reverse sequence: - decrement semaphore - remove breakpoint but you didn't do that. You should, to avoid raising eyebrows :-). And this makes me wonder about non-stop. If the inferior is running, does is make a different if you increment the semaphore before, or after you insert the breakpoint? On 03/09/2012 08:33 PM, Sergio Durigan Junior wrote: > + /* Arguments. We can only extract the argument format if there is a valid > + name for this probe. */ > + if (ret->name) Am I the only one who's eyes sore with implicit pointer->bool conversions? :-) > + { > + ret->args = memchr (ret->name, '\0', > + (char *) el->data + el->size - ret->name); > + > + if (ret->args != NULL) > + ++ret->args; > + if (ret->args == NULL Mixbag of styles in the same function is what hurts the most. :-) > + if (! elf_tdata (obfd)->sdt_note_head) No space after ! please. Several instances of this. On 03/09/2012 08:33 PM, Sergio Durigan Junior wrote: > + /* Parsing each probe's information. */ > + for (iter = elf_tdata (obfd)->sdt_note_head, i = 0; > + iter; > + iter = iter->next, i++) > + /* We first have to handle all the information about the > + probe which is present in the section. */ > + handle_probe (objfile, iter, &ret[i], base); GDB's coding convention now requires {}'s around the multi-line comment + statement, even though there's only a single-line statement. More instances of this throughout. > -/* Helper function that frees any unsed space in the expout array. > - It is generally used when the parser has just been parsed and > - created. */ > +/* See definition in parser-defs.h. */ > > -static void > +void > reallocate_expout (void) > { > /* Record the actual number of expression elements, and then > @@ -811,7 +803,7 @@ copy_name (struct stoken token) > return the index of the subexpression which is the left-hand-side > of the struct operation at EXPOUT_LAST_STRUCT. */ > > -static int > +int > prefixify_expression (struct expression *expr) > { Seems like you move some comments to the headers, and others not. Other examples elsewhere. On 03/09/2012 08:33 PM, Sergio Durigan Junior wrote: > --- a/gdb/ppc-linux-tdep.c > +++ b/gdb/ppc-linux-tdep.c > @@ -66,6 +66,14 @@ > #include "features/rs6000/powerpc-isa205-vsx64l.c" > #include "features/rs6000/powerpc-e500l.c" > > +#include "stap-probe.h" > +#include "ax.h" > +#include "ax-gdb.h" > +#include "cli/cli-utils.h" > +#include "parser-defs.h" > +#include "user-regs.h" > +#include Please put these above the .c includes, along with the other .h includes. More cases of this elsewhere. > + STAP_PROBE (teste, two); Aha, a little PT sneaked in. :-) > +proc stap_test {{arg ""}} { > + global testfile hex > + > + if {$arg != ""} { > + set arg "additional_flags=$arg" > + set addendum ", with semaphore, not optimized" > + } else { > + set addendum ", no semaphore, not optimized" > + } > + Instead of appending $addendum to all test messages (and missing those that are emitted from within gdb.exp and friends), you should now use with_test_prefix instead. > --- a/gdb/testsuite/gdb.cp/nextoverthrow.exp > +++ b/gdb/testsuite/gdb.cp/nextoverthrow.exp > @@ -54,6 +54,17 @@ gdb_test_multiple "print _Unwind_DebugHook" "check for unwinder hook" { > } > } > if {!$ok} { > + gdb_test_multiple "info probe" "check for stap probe in unwinder" { > + -re ".*libgcc.*unwind.*\r\n$gdb_prompt $" { > + pass "check for stap probe in unwinder" > + set ok 1 > + } > + -re "\r\n$gdb_prompt $" { > + } > + } > +} > + > +if {!$ok} { > unsupported "nextoverthrow.exp could not find _Unwind_DebugHook" > return -1 > } I don't understand why we'd now skip the test when we don't have the unwinder stap probe, but in any case, this should not be in this patch, GDB only learns to use the unwind probe in the next patch. > + > +if $tracelevel then { > + strace $tracelevel > +} Please remove all traces of this. All existing instances have already been removed from the testsuite throughout. On 03/09/2012 08:33 PM, Sergio Durigan Junior wrote: > --- a/gdb/tracepoint.c > +++ b/gdb/tracepoint.c > @@ -1717,6 +1717,7 @@ start_tracing (char *notes) > for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++) > { > struct tracepoint *t = (struct tracepoint *) b; > + struct bp_location *loc; > > if (b->enable_state == bp_enabled) > any_enabled = 1; > @@ -1779,6 +1780,9 @@ start_tracing (char *notes) > } > > t->number_on_target = b->number; > + > + for (loc = b->loc; loc; loc = loc->next) > + modify_semaphore (loc, 1); > } > VEC_free (breakpoint_p, tp_vec); > > @@ -1851,9 +1855,28 @@ void > stop_tracing (char *note) > { > int ret; > + VEC(breakpoint_p) *tp_vec = NULL; > + int ix; > + struct breakpoint *t; > > target_trace_stop (); > > + tp_vec = all_tracepoints (); > + for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, t); ix++) > + { > + struct bp_location *loc; > + > + if ((t->type == bp_fast_tracepoint > + ? !may_insert_fast_tracepoints > + : !may_insert_tracepoints)) > + continue; > + > + for (loc = t->loc; loc; loc = loc->next) > + modify_semaphore (loc, 0); > + } > + > + VEC_free (breakpoint_p, tp_vec); This made me wonder about something else with this semaphore handling: the target can itself stop tracing, without GDB requesting it. E.g., if the trace buffer is full. If so, then you'll miss decrementing the semaphore count... Even worse with disconnected tracing; GDB might not even be connected when the tracing stops, and when you reconnect, you have no clue whether to decrement the counts or not... -- Pedro Alves