From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12102 invoked by alias); 25 Oct 2011 19:52:14 -0000 Received: (qmail 12092 invoked by uid 22791); 25 Oct 2011 19:52:13 -0000 X-SWARE-Spam-Status: No, hits=-1.4 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,TW_BJ X-Spam-Check-By: sourceware.org Received: from mail-fx0-f41.google.com (HELO mail-fx0-f41.google.com) (209.85.161.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 25 Oct 2011 19:51:52 +0000 Received: by faat2 with SMTP id t2so1171443faa.0 for ; Tue, 25 Oct 2011 12:51:51 -0700 (PDT) Received: by 10.223.7.14 with SMTP id b14mr55017460fab.10.1319572311298; Tue, 25 Oct 2011 12:51:51 -0700 (PDT) MIME-Version: 1.0 Received: by 10.152.22.6 with HTTP; Tue, 25 Oct 2011 12:51:31 -0700 (PDT) In-Reply-To: References: <4E8DCE67.80507@earthlink.net> <4E92E639.7000402@earthlink.net> From: Justin Lebar Date: Tue, 25 Oct 2011 20:07:00 -0000 Message-ID: Subject: Re: Status of 'blacklist' patch? To: Tom Tromey Cc: Doug Evans , Stan Shebs , gdb-patches@sourceware.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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-10/txt/msg00679.txt.bz2 > Justin> + if (arg !=3D 0) > Justin> + { > Justin> + entry_num =3D parse_and_eval_long (arg); > Justin> + } > > This will parse and evaluate an expression. I think something like what > "info break" does would be more in keeping with other places in gdb -- > that is, use get_number_or_range, then iterate. breakpoint_1 calls parse_and_eval_long: if (allflag && parse_and_eval_long (args) !=3D b->number) continue; if (!allflag && !number_is_in_list (args, b->number)) continue; Do we want to copy the breakpoint code here, or not? On Thu, Oct 20, 2011 at 3:33 PM, Tom Tromey wrote: >>>>>> "Justin" =3D=3D Justin Lebar writes: > > Justin> Added a NEWS entry in this patch. > > Thanks. > > I think this still needs a doc review. > > Some review below. =C2=A0I think this patch is pretty close to being read= y; > and I am eager for it to go in. > > Justin> + =C2=A0/* null if this isn't a skiplist entry for an entire file. > > "NULL" > > Justin> + =C2=A0 =C2=A0 The skiplist entry owns this pointer. */ > > In our style, sentences have 2 spaces after the period. > > Justin> +//static void try_resolve_pending_entry (struct skiplist_entry *= e); > > We can't use "//" comments in gdb, but I think there's really no reason > for a new file to have commented-out code. > > Justin> + =C2=A0 =C2=A0 =C2=A0TRY_CATCH(decode_exception, NOT_FOUND_ERROR) > > Space before the open paren. > > Justin> + =C2=A0if (arg !=3D 0) > Justin> + =C2=A0 =C2=A0{ > Justin> + =C2=A0 =C2=A0 =C2=A0entry_num =3D parse_and_eval_long (arg); > Justin> + =C2=A0 =C2=A0} > > This will parse and evaluate an expression. =C2=A0I think something like = what > "info break" does would be more in keeping with other places in gdb -- > that is, use get_number_or_range, then iterate. > > Justin> + =C2=A0 =C2=A0 =C2=A0entry_chain =3D make_cleanup_ui_out_tuple_b= egin_end (current_uiout, "blklst-entry"); > > I think this line is too long. > Maybe a few others in this function, too. > > Justin> +static void > Justin> +skip_enable_command (char *arg, int from_tty) > Justin> +{ > Justin> + =C2=A0struct skiplist_entry *e; > Justin> + =C2=A0int entry_num; > Justin> + =C2=A0if (arg =3D=3D 0) > > Newline after the declaration block, here and elsewhere. > > Justin> + =C2=A0entry_num =3D parse_and_eval_long (arg); > > This command and others should use get_number_or_range. > > Justin> + =C2=A0 =C2=A0 =C2=A0ALL_SKIPLIST_ENTRIES_SAFE(e, temp) > > Space before open paren. > > Justin> + =C2=A0ALL_SKIPLIST_ENTRIES (e) > Justin> + =C2=A0 =C2=A0{ > Justin> + =C2=A0 =C2=A0 =C2=A0int pc_match =3D e->pc !=3D 0 && pc =3D=3D = e->pc; > Justin> + =C2=A0 =C2=A0 =C2=A0int filename_match =3D e->filename !=3D 0 &= & filename !=3D 0 && > > You have to parenthesize the RHS according to GNU standards, and put the > "&&" at the start of the second line. > > Justin> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0strcmp (filename, e->filename) =3D=3D 0; > Justin> + =C2=A0 =C2=A0 =C2=A0if (e->enabled && !e->pending && (pc_match = || filename_match)) > Justin> + =C2=A0 =C2=A0 =C2=A0 return 1; > > I think you could check enabled and pending earlier and avoid the strcmp > unless truly needed. > > Also I think it would be worthwhile to lazily compute the SAL in this > function, to avoid computing it at all when there are no (or no enabled) > skip entries. > > Justin> +void > Justin> +skip_re_set () > > Should be '(void)' > > Justin> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 TRY_CATCH(decode_exception, NOT_FOU= ND_ERROR) > > Space before paren. > > Justin> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (decode_exception.reason >=3D 0 = && > > "&&" on next line. > > Justin> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0e= ->function_name =3D xstrdup(func_name); > > Space before paren. > > Justin> +/* Helper function to get a gdbarch from a symtab_and_line. */ > Justin> +static struct gdbarch* > Justin> +get_sal_arch (struct symtab_and_line *sal) > Justin> +{ > Justin> + =C2=A0if (sal->section) > Justin> + =C2=A0 =C2=A0return get_objfile_arch (sal->section->objfile); > Justin> + =C2=A0if (sal->symtab) > Justin> + =C2=A0 =C2=A0return get_objfile_arch (sal->symtab->objfile); > Justin> + =C2=A0return get_current_arch (); > Justin> +} > > I think it is better to make this public in breakpoint.c. > > Really, we should probably have a "sal.h" and "sal.c" and treat it like > a real data structure. =C2=A0Not your job though. > > Justin> + =C2=A0add_prefix_cmd ("skip", class_breakpoint, skip_function_c= ommand, _("\ > Justin> +Ignore a function while stepping.\n\ > Justin> +skip [FUNCTION NAME]\n\ > > I'd like this line to start with "Usage: " (and likewise in the other > help text). > > Justin> +void skip_re_set (); > > Should be '(void)'. > > Justin> +struct program_space* > Justin> +get_last_displayed_pspace () > > Space before "*". > "(void)", here and elsewhere. > > Most of the functions around this need introductory comments. > > Justin> +void clear_last_displayed_sal (); > Justin> +int last_displayed_sal_is_valid (); > Justin> +struct program_space* get_last_displayed_pspace (); > Justin> +CORE_ADDR get_last_displayed_addr (); > Justin> +struct symtab* get_last_displayed_symtab (); > Justin> +int get_last_displayed_line (); > > "(void)" for all of these. > > Tom >