From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18901 invoked by alias); 23 May 2008 19:30:41 -0000 Received: (qmail 18893 invoked by uid 22791); 23 May 2008 19:30:40 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 23 May 2008 19:30:23 +0000 Received: (qmail 20208 invoked from network); 23 May 2008 19:30:21 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 23 May 2008 19:30:21 -0000 From: Pedro Alves To: gdb-patches@sourceware.org, Tom Tromey Subject: Re: RFA: Patch: implement missing macro functions Date: Sat, 24 May 2008 15:42:00 -0000 User-Agent: KMail/1.9.9 References: In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline Message-Id: <200805232030.20677.pedro@codesourcery.com> 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: 2008-05/txt/msg00690.txt.bz2 Hi Tom, You'll need someone else to comment on the big picture -- I'll comment on a few strokes, below, > =A0static void > =A0macro_define_command (char *exp, int from_tty) > =A0{ > - =A0error (_("Command not implemented yet.")); > + =A0struct macro_definition *new_macro =3D NULL; > + =A0char *name =3D NULL; > + =A0struct cleanup *cleanup_chain =3D make_cleanup (free_macro_definitio= n_ptr, > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0&new_m= acro); > + =A0cleanup_chain =3D make_my_cleanup (&cleanup_chain, free_current_cont= ents, > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0 =A0 &name); That is a big nop to write + with 2 leaks attached. The new cleanup is being discarded and leaked, so NAME isn't being free'd at all. Don't call make_my_cleanup directly. Instead, call make_cleanup and discard its result: You want to hold the cleanup chain pointer representing the old cleanups on function entry, and that is returned on the first make_cleanup call. struct cleanup *cleanup_chain =3D make_cleanup (free_macro_definition_ptr, &new_macro); make_cleanup (free_current_contents, &name); (pedantically, if you don't need the NAME variable after cleaning up or an exception is thrown (local vars usually apply), an xfree cleanup is also fine, like in `make_cleanup (xfree, name)') > +static void > +print_one_macro (const char *name, const struct macro_definition *macro) > +{ > + =A0fprintf_filtered (gdb_stdout, "macro define %s", name); > + =A0if (macro->kind =3D=3D macro_function_like) > + =A0 =A0{ > + =A0 =A0 =A0int i; > + =A0 =A0 =A0fprintf_filtered (gdb_stdout, "("); > + =A0 =A0 =A0for (i =3D 0; i < macro->argc; ++i) > +=A0=A0=A0=A0=A0=A0=A0fprintf_filtered (gdb_stdout, "%s%s", (i > 0) ? ", = " : "", > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 = =A0macro->argv[i]); > + =A0 =A0 =A0fprintf_filtered (gdb_stdout, ")"); > + =A0 =A0} > + =A0/* Note that we don't need a leading space here -- "macro define" > + =A0 =A0 provided it. =A0*/ > + =A0fprintf_filtered (gdb_stdout, "%s\n", macro->replacement); > + =A0gdb_flush (gdb_stdout); > =A0} Did you really need a gdb_flush here? > =A0 > +/* Helper function for macro_for_each. =A0*/ > +static int > +foreach_macro (splay_tree_node node, void *fnp) > +{ > + =A0macro_callback_fn fn =3D (macro_callback_fn) fnp; (Hmmm, can we assume casting void* <-> func pointers is safe on all supported hosts/compilers? Posix and Windows do require it to be safe. I actually have no idea how much GDB common code relies on it today.) > + =A0struct macro_key *key =3D (struct macro_key *) node->key; > + =A0struct macro_definition *def =3D (struct macro_definition *) node->v= alue; > + =A0(*fn) (key->name, def); > + =A0return 0; > +} > + > +void > +macro_for_each (struct macro_table *table, macro_callback_fn fn) > +{ > + =A0splay_tree_foreach (table->definitions, foreach_macro, fn); > +} > =A0 > =A0#endif /* MACROTAB_H */ > Index: testsuite/gdb.base/macscp.exp > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > RCS file: /cvs/src/src/gdb/testsuite/gdb.base/macscp.exp,v > retrieving revision 1.7 > diff -u -r1.7 macscp.exp > --- testsuite/gdb.base/macscp.exp=A0=A0=A0=A0=A0=A0=A03 May 2008 22:30:51= -0000=A0=A0=A0=A0=A0=A0=A01.7 > +++ testsuite/gdb.base/macscp.exp=A0=A0=A0=A0=A0=A0=A021 May 2008 00:25:0= 9 -0000 > @@ -428,3 +428,23 @@ > =A0gdb_test "print M" \ > =A0 =A0 =A0"No symbol \"M\" in current context\." \ > =A0 =A0 =A0"print expression with macro after undef." > + > +gdb_test "macro define M 5" \ > + =A0"" \ > + =A0"basic macro define" > + > +gdb_test "print M" \ > + =A0" =3D 5" \ > + =A0"expansion of defined macro" > + > +gdb_test "macro list" \ > + =A0"macro define M 5" \ > + =A0"basic macro list" > + > +gdb_test "macro undef M" \ > + =A0"" \ > + =A0"basic macro undef" > + > +gdb_test "print M" \ > + =A0 =A0"No symbol \"M\" in current context\." \ > + =A0 =A0"print expression with macro after user undef." Would it make sense to have a test with macro arguments, and a test where a user macro overrides a source macro? --=20 Pedro Alves