From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32350 invoked by alias); 13 Jan 2012 11:09:57 -0000 Received: (qmail 32342 invoked by uid 22791); 13 Jan 2012 11:09:56 -0000 X-SWARE-Spam-Status: No, hits=-6.5 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; Fri, 13 Jan 2012 11:09:43 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q0DB9gwc027710 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 13 Jan 2012 06:09:42 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q0DB9fYQ027219; Fri, 13 Jan 2012 06:09:41 -0500 Message-ID: <4F1010F5.4020104@redhat.com> Date: Fri, 13 Jan 2012 11:14:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20111222 Thunderbird/9.0 MIME-Version: 1.0 To: Sergio Durigan Junior CC: gdb-patches@sourceware.org, Tom Tromey Subject: Re: [RFC] Make static tracepoint with markers more OO 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-01/txt/msg00467.txt.bz2 On 01/13/2012 04:01 AM, Sergio Durigan Junior wrote: > Hello there, > > I have been working on this patch, and I would like some comments from > you guys. It basically implements new methods inside breakpoint_ops in > order to make static tracepoint with markers (`strace -m') more OO. Thanks. > Currently, we use a check (defined in `is_marker_spec') in order to > identify those tracepoints, and act accordingly. However, it makes the > code (especially the functions `create_breakpoint' and > `addr_string_to_sals') very conditional. > > This would be OK if it were to be the only case, but as it turns out our > SystemTap integration patch will have to add more complexity to this > code, which would make things uglier and uglier. So, as a preparation > for the second round of submissions of the SystemTap patch, I am > submitting this patch first. > > I am not sure the names I picked for the new methods are good. I had > spent a good time thinking about the names, but unfortunately I couldn't > come up with something better than this. I am specially concerned about > the `create_breakpoints_sal*' set of methods/functions, because there > are too many of them IMO. I would appreciate some reviews on the > comments of the methods, as well. I believe the separation is pretty > straightforward, so I think this is just moving bits here and there > without actually implementing anything new. > > Comments are welcome, as usual. This is fine with me. > -/* Assuming we're creating a static tracepoint, does S look like a > - static tracepoint marker spec ("-m MARKER_ID")? */ > -#define is_marker_spec(s) \ > - (s != NULL && strncmp (s, "-m", 2) == 0 && ((s)[2] == ' ' || (s)[2] == '\t')) > +/* Return 1 if B refers to a static tracepoint marker, zero otherwise. */ I think /* Return 1 if B refers to a static tracepoint set by marker ("-m"), zero otherwise. */ would be clearer. > + > +static int strace_marker_p (struct breakpoint *b); > - if (b->type == bp_static_tracepoint && !marker_spec) > + if (strace_marker_p (b)) This one looks wrong. The old condition had a `!', so this was for static tracepoints _not_ set through a marker. > + > + /* Create SALs from address string, storing the result in linespec_result. > + Return 1 on success, or zero otherwise. > + > + For an explanation about the arguments, see the function > + `create_sals_from_address_default'. > + > + This function is called inside `create_breakpoint'. */ > + int (*create_sals_from_address) (char **, struct linespec_result *, > + enum bptype, int *, char *, char **); > + > + /* This method will be responsible for creating a breakpoint given its SALs. > + Usually, it just calls `create_breakpoints_sal' (for ordinary > + breakpoints). However, there may be some special cases where we might > + need to do some tweaks, e.g., see > + `strace_marker_init_or_create_breakpoint_sal'. > + > + This function is called inside `create_breakpoint'. */ > + void (*create_breakpoints_sal) (struct gdbarch *, > + struct linespec_result *, > + struct linespec_sals *, char *, > + enum bptype, enum bpdisp, int, int, > + int, const struct breakpoint_ops *, > + int, int, int); It's unfortunate to be calling the breakpoint's virtual methods before the object itself is created, which will require some redesign and refactoring if we ever switch to C++ (and is dangerous, as you may end up touching parts of the object which are not constructed yet by mistake), but, this is no worse than what we have now, so I'm fine with it. > + > + /* Given the address string (second parameter), this method decodes it > + and provides the SAL locations related to it. For ordinary breakpoints, > + it calls `decode_line_full'. > + > + This function is called inside `addr_string_to_sals'. */ > + void (*decode_linespec) (struct breakpoint *, char **, > + struct symtabs_and_lines *); > }; > > /* Helper for breakpoint_ops->print_recreate implementations. Prints