From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21039 invoked by alias); 1 Sep 2011 22:46:29 -0000 Received: (qmail 21027 invoked by uid 22791); 1 Sep 2011 22:46:27 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,TW_BJ,T_TO_NO_BRKTS_FREEMAIL X-Spam-Check-By: sourceware.org Received: from mail-yi0-f41.google.com (HELO mail-yi0-f41.google.com) (209.85.218.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 01 Sep 2011 22:46:12 +0000 Received: by yib2 with SMTP id 2so1926440yib.0 for ; Thu, 01 Sep 2011 15:46:11 -0700 (PDT) MIME-Version: 1.0 Received: by 10.236.77.105 with SMTP id c69mr2132214yhe.113.1314917171729; Thu, 01 Sep 2011 15:46:11 -0700 (PDT) Received: by 10.236.34.162 with HTTP; Thu, 1 Sep 2011 15:46:11 -0700 (PDT) In-Reply-To: References: <1314198654-9008-1-git-send-email-ratmice@gmail.com> <1314198654-9008-4-git-send-email-ratmice@gmail.com> Date: Thu, 01 Sep 2011 22:57:00 -0000 Message-ID: Subject: Re: [PATCH 3/7] [python] API for macros: Add gdb.Macro class. From: Matt Rice To: pmuldoon@redhat.com Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 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-09/txt/msg00020.txt.bz2 On Tue, Aug 30, 2011 at 5:45 AM, Phil Muldoon wrote: > matt rice writes: >> +macropy__definition_to_py (const struct macro_definition *macro) > > A small nit for me personally, but all of the other Python API classes > use one "_" to separate the function library name from the function > name. =A0This and others. Hmm... It seems as though all the Python API classes use 'foopy_' prefixes _only_ for the 'public methods available through python', all of these 2x"_" functions are private functions, I understand that static functions such as these do not pollute the global namespace, but I find the tab completion behavior when debugging gdb itself that having prefixes on these helps considerably. Other python API seems to use random function names for these (symtab_, set_, sal_, etc). thus I use the 2x_ mainly because of the 'beginning with _' reservation for compiler/libc. Anyhow, it doesn't seem like removing one of the '_'s is really the thing t= o do, maybe macropy_pvt_* (i'll see if that works but some of the functions are getting fairly long.) >> +macropy__validate_self (PyObject *self) >> +{ >> + =A0macro_object *me; >> + >> + =A0if (! PyObject_TypeCheck (self, ¯o_object_type)) >> + =A0 =A0{ >> + =A0 =A0 =A0PyErr_SetString (PyExc_RuntimeError, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0_("Typecheck failure conver= ting macro.")); >> + =A0 =A0 =A0return NULL; >> + =A0 =A0} >> + >> + =A0me =3D (macro_object *) self; >> + >> + =A0if (me->objfile_validator->is_valid =3D=3D 0) >> + =A0 =A0{ >> + =A0 =A0 =A0PyErr_SetString (PyExc_RuntimeError, _("Macro is invalid.")= ); >> + =A0 =A0 =A0return NULL; >> + =A0 =A0} >> + >> + =A0return me; >> +} > > As you are pointing to an existing copy of self, and returning it, you > *may* need to incref. =A0It depends if any of the functions downstream > decref it. =A0Please check. this is basically the same as the FOOPY_REQUIRE_VALID macro's it shouldn't need to, for macros we don't really have a symtab_object_to_symtab or sal_object_to_sal type of function to do the type check in, putting that into e.g. MACROPY_REQUIRE_VALID requires using the PyObject * argument twice, I also found the assignment in the macro to be weird :/ I should probably rename it macropy__require_valid(). >> +static int >> +macropy_compare (PyObject *self, PyObject *o2) >> +{ >> + =A0PyObject *my_str =3D macropy_str (self); >> + =A0PyObject *other_str =3D NULL; >> + =A0PyObject *an_error; >> + =A0int err_code =3D -1; >> + =A0int comparison_result =3D -1; >> + >> + =A0if (!my_str) >> + =A0 =A0goto check_for_errors_and_return; >> + >> + =A0if (PyObject_TypeCheck (o2, ¯o_object_type)) >> + =A0 =A0{ >> + =A0 =A0 =A0other_str =3D macropy_str (o2); >> + =A0 =A0 =A0if (!other_str) >> + =A0 =A0 goto check_for_errors_and_return; >> + >> + =A0 =A0 =A0err_code =3D PyObject_Cmp (my_str, other_str, &comparison_r= esult); >> + =A0 =A0} > > Use PyObject_Compare Hmm, I'm very skeptical of PyObject_Compare() because of its lack of a way to identify if there was an error that may have been cleared by something, e.g. gdb_py_print_stack()... this may not be possible, but it may be if we figure out a way to have Macro's comparison method 'monkey patchable'. >> + =A0else >> + =A0 =A0{ >> + =A0 =A0 =A0err_code =3D PyObject_Cmp (my_str, o2, &comparison_result); >> + =A0 =A0} > > Ditto. > >> + =A0check_for_errors_and_return: >> + =A0 =A0Py_XDECREF (my_str); >> + =A0 =A0Py_XDECREF (other_str); >> + >> + =A0 =A0/* Because -1 is also Less Than we cannot use gdbpy_print_stack= () >> + =A0 =A0 =A0 which clears the error if set python print-stack is off. >> + =A0 =A0 =A0 Which would lead to (gdb) python print x =3D=3D x >> + =A0 =A0 =A0 returning False with no error message displayed. >> + >> + =A0 =A0 =A0 alternately if an error is set and the return value is not= 1, >> + =A0 =A0 =A0 python will complain. >> + >> + =A0 =A0 =A0 Thus if there is an error, we must normalize it >> + =A0 =A0 =A0 making sure that both an error and the return >> + =A0 =A0 =A0 value are -1. Should we encounter one. =A0*/ >> + =A0 =A0an_error =3D PyErr_Occurred (); >> + =A0 =A0if (an_error =3D=3D NULL && err_code !=3D -1) >> + =A0 =A0 =A0{ >> + =A0 =A0 =A0 =A0/* a valid comparison */ >> + =A0 =A0 =A0 =A0return comparison_result; >> + =A0 =A0 =A0} >> + =A0 =A0else if (an_error =3D=3D NULL && err_code =3D=3D -1) >> + =A0 =A0 =A0{ >> + =A0 =A0 /* an unknown error */ >> + =A0 =A0 PyErr_SetString (PyExc_RuntimeError, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0_("mystery error during mac= ro comparison.")); > > > Don't overwrite the exception here. =A0And if PyObject_Compare has > returned -1, there will be an exception so this can't happen. =A0You could > probably just remove the condition entirely and just fold over to the > else below. that comment should also read 'and the return value is not -1' instead of 'not 1'... as per the above, we aren't overwriting the exception, in this case 'an_error' is the current exception and it is NULL, while PyObject_Cmp returned -1 signalling to us that there was an error. if we return -1 without an error set they compare as 'less than', and the user may have no idea that an error was even set. unless we can prove this can never ever happen with any version of python I prefer PyObject_Cmp, which has the error code return value in place of PyObject_Compare which relies on an PyErr_Occurred e.g. If i figure out a way to add monkey patching of '__cmp__' on gdb.Macro objects, its plausible that a user defined comparison function could end up in a call to gdb_py_print_stack(), and a silent failure. >> + =A0 =A0 =A0 =A0return -1; >> + =A0 =A0 =A0} >> + =A0 =A0else /* a known error */ >> + =A0 =A0 =A0return -1; >> +} >> + > > >> +static long >> +macropy_hash (PyObject *o) >> +{ >> + =A0long result =3D -1; >> + =A0PyObject *my_str =3D macropy_str (o); >> + >> + =A0if (my_str) >> + =A0 =A0result =3D PyObject_Hash (my_str); >> + >> + =A0/* The docs say PyObject_Hash should return -1 on error, >> + =A0 =A0 Don't believe it because it is not always true, >> + =A0 =A0 The exception gets raised regardless of return value, >> + =A0 =A0 But we should follow the documented return strategy of -1 on e= rror. > > Even if you are right, are you right for all versions of Python? =A0I > guess the PyErr_Occurred check is harmless. no idea about all versions, i checked with 2.7, concurr it's harmless. I'll try and make it less definitive/mention the version. >> +static PyMethodDef macro_object_methods[] =3D { >> + =A0{ "argv", macropy_argv_method, METH_NOARGS, >> + =A0 =A0"argv () -> List.\n\ >> +Return a list containing the names of the arguments for a function like= \ >> +macro." }, > > "function-like", imo. > >> + =A0{ "definition", macropy_definition_method, METH_NOARGS, >> + =A0 =A0"definition () -> String.\n\ >> +Return the macro's definition." }, >> + =A0{ "include_trail", macropy_include_trail_method, METH_NOARGS, >> + =A0 =A0"include_trail () -> List.\n\ >> +Return a list containing tuples with the filename and line number descr= ibing \ >> +how and where this macro came to be defined." }, > > Your help and your return explanation don't match. =A0It should return a > Tuple (if it doesn't, please fix it). =A0And please change =A0-> List to = -> > Tuple. They do, but it's awkward e.g. a list OF tuples, so [tuple, tuple, tuple] >> + =A0struct objfile *objfile; >> +}; >> + >> +/* A macro_callback_fn for use with macro_foreach and friends that adds >> + =A0 a gdb.Macro object to the python list USER_DATA->LIST. >> + =A0 USER_DATA should be of type struct macropy_user_data. >> + >> + =A0 A null USER_DATA->OBJFILE is currently be considered an error, >> + =A0 this is subject to change. >> + >> + =A0 Aborts the iteration on error, and forces the macro iterator funct= ion >> + =A0 to return macro_walk_aborted check the iterators macro_walk_result. >> + =A0 On error it is the callers responsibility to dispose of the USER_D= ATA >> + =A0 structure and its contents. =A0The appropriate python exception >> + =A0 that caused the error has already been set. >> +*/ >> +enum macro_walk_status >> +pymacro_add_macro_to_list (const char *name, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const struct macro_definition *definiti= on, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct macro_source_file *source, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int line, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0void *user_data); >> + >> +/* Create a new macro (gdb.Macro) object that encapsulates >> + =A0 the gdb macro representation. =A0OBJFILE is the objfile that conta= ins >> + =A0 the macro table. =A0NAME is the name of the macro. =A0DEF the macr= os definition >> + =A0 SRC the source file containing the macro. LINE definitions locatio= n. =A0*/ >> +PyObject * >> +macropy_object_create (struct objfile *objfile, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const char *name, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const struct macro_definition *= def, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct macro_source_file *src, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int line); >> + >> +#endif /* GDB_PY_MACRO_H */ > > Any reason why these need their own include? =A0(over python-internal.h). Nope, just didn't see these all collected there. > If these are called from anywhere other than python/* they are not.