From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29566 invoked by alias); 16 Aug 2011 15:16:59 -0000 Received: (qmail 29557 invoked by uid 22791); 16 Aug 2011 15:16:57 -0000 X-SWARE-Spam-Status: No, hits=-1.6 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_BL_SPAMCOP_NET,RCVD_IN_DNSWL_LOW,TW_RG,T_TO_NO_BRKTS_FREEMAIL X-Spam-Check-By: sourceware.org Received: from mail-gy0-f169.google.com (HELO mail-gy0-f169.google.com) (209.85.160.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 16 Aug 2011 15:16:32 +0000 Received: by gyg10 with SMTP id 10so7655gyg.0 for ; Tue, 16 Aug 2011 08:16:32 -0700 (PDT) MIME-Version: 1.0 Received: by 10.236.176.197 with SMTP id b45mr16861050yhm.198.1313507791940; Tue, 16 Aug 2011 08:16:31 -0700 (PDT) Received: by 10.236.34.193 with HTTP; Tue, 16 Aug 2011 08:16:31 -0700 (PDT) In-Reply-To: References: Date: Tue, 16 Aug 2011 15:16:00 -0000 Message-ID: Subject: Re: [patch] PR python/10807 API for macros. 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-08/txt/msg00333.txt.bz2 On Tue, Aug 16, 2011 at 3:01 AM, Phil Muldoon wrote: > Matt Rice writes: > >> In the gdb.Macro class, =A0I didn't used methods instead of attributes, >> because the values are lazily computed/can return different objects if >> called multiple times, thus attributes seemed weird, even though I >> think the Macro methods are fairly attribute like. =A0so if anyone has >> any opinion here on attributes vs methods. > > Thanks. > >> +typedef struct >> +{ >> + =A0PyObject_HEAD; >> + =A0const char *name; >> + =A0struct macro_source_file *src_file; >> + =A0int src_line; >> +} macro_object; >> + >> +static PyTypeObject macro_object_type; > > We've been trying to put comments in typedef/struct definitions to > narrate each field. =A0Some of the older Python files do not have them, > but the newer ones do. =A0Just a one line comment on each field is fine. > >> +static PyObject * >> +argv_to_py (struct macro_definition *macro) >> +{ >> + =A0PyObject *ret =3D NULL; >> + >> + =A0if (macro->kind =3D=3D macro_function_like) >> + =A0 =A0{ >> + =A0 =A0 =A0Py_ssize_t i; >> + =A0 =A0 =A0PyObject *ret =3D PyList_New (macro->argc); >> + >> + =A0 =A0 =A0if (ret =3D=3D NULL) >> + =A0 =A0 return NULL; >> + >> + =A0 =A0 =A0for (i =3D 0; i < macro->argc; i++) >> + =A0 =A0 =A0 =A0{ >> + =A0 =A0 =A0 PyObject *item =3D PyString_FromString (macro->argv[i]); >> + >> + =A0 =A0 =A0 if (!item) >> + =A0 =A0 =A0 =A0 goto err_ret; >> + >> + =A0 =A0 =A0 if (PyList_SetItem (ret, i, item) !=3D 0) >> + =A0 =A0 =A0 =A0 =A0 =A0goto err_ret; >> + =A0 =A0 =A0 =A0} >> + >> + =A0 =A0 =A0return ret; >> + =A0 =A0} >> + >> + =A0Py_RETURN_NONE; >> + >> + =A0err_ret: >> + =A0 =A0Py_XDECREF (ret); >> + =A0 =A0return NULL; >> +} > > Some minor nits. > > Some indents looks a little weird, but it might just be the mailer. =A0You > don't need Py_XDECREF here as you already accounted for the PyList being > NULL. =A0So just Py_DECREF. > >> + >> +static PyObject * >> +include_trail_to_py(struct macro_definition *macro, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const char *name, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct macro_source_file *src_file, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int src_line) >> +{ >> + =A0PyObject *tuple =3D PyTuple_New (2); >> + =A0PyObject *result =3D PyList_New (0); >> + =A0PyObject *tmp; >> + =A0struct macro_source_file *file; >> + =A0int line; >> + >> + =A0if (!tuple || !result) >> + =A0 =A0goto err_exit; > > Even though either one of these indicates a failure case, and cannot > continue, you need to check the individual return status right after > instantiation. =A0This is so you return the correct exception back to the > user. > >> + =A0file =3D macro_definition_location (src_file, src_line, name, &line= ); >> + =A0if (!file) >> + =A0 =A0goto err_exit; > > I'm not sure it is right to do this. =A0macro_definition_location is a GDB > function that can return NULL, not out of error, but because there is no > definition of a splay tree node for that definition. =A0Looking at the > functions you call, and macro_definition_location calls, this can > expected and does not signal a Python error condition. =A0While, at this > point it is right to exit because there is nothing more you can do, > returning NULL from a Python function raises the expectation there is a > Python error with an accompanying exception. =A0If I remember correctly, > Python will complain about this. =A0Perhaps it would be better in this > case to return None, and document the return strategy in the comments at > the function header. > >> + >> + =A0tmp =3D PyString_FromString (file->filename); >> + =A0if (!tmp) >> + =A0 =A0goto err_exit; >> + =A0PyTuple_SetItem (tuple, 0, tmp); >> + >> + =A0tmp =3D PyInt_FromLong (line); >> + =A0if (!tmp) >> + =A0 =A0goto err_exit; >> + =A0if (PyTuple_SetItem (tuple, 1, tmp) !=3D 0) >> + =A0 =A0goto err_exit; >> + =A0if (PyList_Append (result, tuple) !=3D 0) >> + =A0 =A0goto err_exit; >> + =A0Py_DECREF (tuple); > >> + =A0while (file->included_by) >> + =A0 =A0{ >> + =A0 =A0 =A0tuple =3D PyTuple_New (2); >> + >> + =A0 =A0 =A0if (!tuple) >> + =A0 =A0 =A0 =A0goto err_exit; >> + >> + =A0 =A0 =A0tmp =3D PyString_FromString (file->included_by->filename); >> + =A0 =A0 =A0if (!tmp) >> + =A0 =A0 =A0 =A0goto err_exit; >> + =A0 =A0 =A0if (PyTuple_SetItem (tuple, 0, tmp) !=3D 0) >> + =A0 =A0 =A0 =A0goto err_exit; >> + >> + =A0 =A0 =A0tmp =3D PyInt_FromLong (file->included_at_line); >> + =A0 =A0 =A0if (!tmp) >> + =A0 =A0 =A0 =A0goto err_exit; >> + =A0 =A0 =A0if (PyTuple_SetItem (tuple, 1, tmp) !=3D 0) >> + =A0 =A0 =A0 =A0goto err_exit; >> + >> + =A0 =A0 =A0if (PyList_Append (result, tuple) !=3D 0) >> + =A0 =A0 goto err_exit; >> + =A0 =A0 =A0Py_DECREF (tuple); >> + >> + =A0 =A0 =A0file =3D file->included_by; >> + =A0 =A0} >> + >> + =A0return result; >> + >> + =A0err_exit: >> + =A0 =A0Py_XDECREF (tuple); >> + =A0 =A0Py_DECREF (result); > > Either of these could be NULL, so you need to use XDECREF in both cases. > >> + =A0 =A0return NULL; >> +} >> + >> +/* Create a new macro (gdb.Macro) object that encapsulates the >> + =A0 macro_definition structure from GDB. =A0*/ >> +PyObject * >> +macropy_object_create (struct macro_definition *macro, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const char *name, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct macro_source_file *ms, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int line) > > I normally try to document parameter usage here. =A0And other publicly > facing functions, too. > > =A0+{ >> + =A0macro_object *macro_obj; >> + >> + =A0macro_obj =3D PyObject_New (macro_object, ¯o_object_type); >> + =A0if (macro_obj) >> + =A0 =A0{ >> + =A0 =A0 =A0/* Save enough to lookup the macro again in the methods. >> + =A0 =A0 =A0Then do a lookup and lazily copy things into PyObjects. >> + =A0 =A0 =A0 =A0 Consecutive lookups should be OK, because of the splay= tree. >> + =A0 =A0 =A0 =A0 We cannot save a macro definition due to inconsistent = memory >> + =A0 =A0 =A0management. =A0We rely on the fact that the macro_source_fi= le >> + =A0 =A0 =A0 =A0 is not released until exit. >> + >> + =A0 =A0 =A0It seems we may need to move to using a 'macro_scope' >> + =A0 =A0 =A0if we want a python api for user-defined macros. */ > > > What do you mean by inconsistent memory management? =A0Can you expand/exp= lain > this. =A0Do macro definitions have a life-cycle in GDB? depends on the source of the macro, xmalloc directly for user-defined, or the macro table's obstack. from macroscope.c: /* A table of user-defined macros. Unlike the macro tables used for symtabs, this one uses xmalloc for all its allocation, not an obstack, and it doesn't bcache anything; it just xmallocs things. So it's perfectly possible to remove things from this, or redefine things. */ I'm second guessing myself that caching the macro_source file is safe thoug= h. >> +static PyObject * >> +macropy_str (PyObject *self) >> +{ >> + =A0PyObject *result =3D PyString_FromString ("> + =A0macro_object *me =3D (macro_object *) self; >> + =A0struct macro_definition *macro; >> + =A0PyObject *argv =3D macropy_argv_method (self, Py_None); >> + =A0PyObject *definition =3D macropy_definition_method (self, Py_None); >> + =A0PyObject *include_trail =3D macropy_include_trail_method (self, Py_= None); >> + =A0PyObject *is_function_like =3D macropy_is_function_like_method (sel= f, Py_None); >> + =A0PyObject *name =3D PyString_FromString (me->name); >> + >> + =A0if (!definition || !is_function_like || !argv >> + =A0 =A0 =A0|| !include_trail || !name || !result) >> + =A0 =A0goto err_ret; > > > Because all of these are defined in the local block, what exception will > be returned on what failure? If RESULT fails, then ARGV, I think the > exception written for RESULT will be overwritten. =A0I know it is a pain, > but I think you need to check each one. > > >> +static int >> +macropy_compare (PyObject *self, PyObject *o2) >> +{ >> + =A0PyObject *my_str =3D macropy_str (self); >> + =A0int result; >> + >> + =A0if (!my_str) >> + =A0 =A0return -1; >> + >> + =A0if (PyObject_TypeCheck (o2, ¯o_object_type)) >> + =A0 =A0{ >> + =A0 =A0 =A0PyObject *other_str =3D macropy_str (o2); >> + >> + =A0 =A0 =A0if (other_str) >> + =A0 =A0 =A0 =A0result =3D PyObject_Compare (my_str, other_str); > > You need to cope with errors here. =A0Use PyErr_Occurred() or > gdbpy_print_stack. > > >> + =A0 =A0 =A0else >> + =A0 =A0 result =3D -1; >> + >> + =A0 =A0 =A0Py_DECREF (my_str); >> + =A0 =A0 =A0Py_XDECREF (other_str); >> + =A0 =A0 =A0return result; >> + =A0 =A0} >> + >> + =A0result =3D PyObject_Compare (my_str, o2); >> + > > Same as above. > >> + =A0Py_DECREF (my_str); >> + =A0return result; >> +} >> + >> +static long >> +macropy_hash(PyObject *o) >> +{ >> + =A0long result; >> + =A0PyObject *my_str =3D macropy_str (o); >> + >> + =A0if (!my_str) >> + =A0 =A0return -1; >> + >> + =A0result =3D PyObject_Hash (my_str); > > Same as above, re failure. =A0Why do you need a custom hash function? needed the hash function for storing in sets for the same reason that set([{"a" : "b"}]) doesn't work Traceback (most recent call last): File "", line 1, in TypeError: unhashable type: 'dict' mostly I was just using it in the tests, the only use case I can imagine is once we have a symtab_and_line macros() function you could use it to find the difference between 2 'macro scopes' >> + =A0PyObject *tmp; >> + >> + =A0if (! PyObject_TypeCheck (mud->list, &PyList_Type)) >> + =A0 =A0goto err_exit; > > These comments are somewhat complex, and really follow the three paragraph > comments belows. So take them as one big comment. > > Normally, a function that does not return a Python object has to > deal with stack printing/error detection internally. Because this is not > a Python function (IE returning NULL signifies an error, you have to > check and deal with the error here) =A0this is done with > gdbpy_print_stack. =A0However .... > >> + =A0if (mud->errors !=3D 0) >> + =A0 =A0goto err_exit; >> + >> + =A0tmp =3D macropy_object_create (definition, name, source, line); >> + =A0if (tmp) >> + =A0 =A0{ >> + =A0 =A0 =A0if (PyList_Append (mud->list, tmp) !=3D 0) >> + =A0 =A0 goto err_exit; > > ... > >> +static PyObject * >> +stpy_macros (PyObject *self, PyObject *args) >> +{ >> + =A0struct symtab *st =3D symtab_object_to_symtab (self); >> + =A0struct macro_user_data mud; >> + =A0PyObject *result; >> + >> + =A0STPY_REQUIRE_VALID (self, st); >> + >> + =A0result =3D PyList_New (0); >> + =A0if (result =3D=3D NULL) >> + =A0 =A0return NULL; >> + >> + =A0mud.list =3D result; >> + =A0mud.errors =3D 0; >> + =A0macro_for_each (st->macro_table, add_macro_to_list, &mud); >> + >> + =A0if (mud.errors !=3D 0) >> + =A0 =A0{ >> + =A0 =A0 =A0Py_DECREF (result); >> + =A0 =A0 =A0return NULL; > > I'm really puzzled what to do here. =A0I'm assuming the iterator won't > stop because the helper function has encountered errors (at least, the > function pointer prototype is a void return). =A0Because macro_for_each is > a GDB iterator function that calls (through a function pointer and other > GCC helpers) add_macro_to_list, which itself calls Python functions, > each iteration can raise a Python error. =A0If we print and cope with the > error for each iteration in the helper as suggested above, the user will > see each exception and no exception data is overwritten. =A0Also there > seems (to me, at least) no way to abort the iterator earlier. > > OTOH we want to make sure that this function returns correctly, > according to how Python functions should. =A0So we SHOULD return NULL here > if there were errors, but if we do what I suggested above, every > exception will already be printed and cleared. =A0So returning NULL here > will cause Python to complain. =A0But I also don't want previous iteration > exceptions overwritten either. Maybe your way is right in that we only > report the last iterations exception. =A0Or maybe we should construct our > own exception and return that. =A0I do not know. =A0I'd appreciate > comments here from the maintainers. another approach may be to use a 'macro iterator' method which calls a python function with the converted arguments. I initially had macro_object just copy everything into a python object. but the macros() implementations used far too much memory (in a hello world) building a full list with each macro containing an include_trail, with a macro iterator we wouldn't return a list, just call the callback for each macro, it'd mean error handling would have to happen in said python callback function, but we'd be free of any concerns with caching the macro_source_file, otoh memory size probably won't go away it just means we have punted said concerns to the user, if they try to store it in a list, they'd probably run into the same thing. >> --- /dev/null >> +++ b/gdb/testsuite/gdb.python/py-macro.c >> @@ -0,0 +1,72 @@ >> +#ifdef DEF_MACROS > > Needs a copyright/license header. > > >> +# >> +# You should have received a copy of the GNU General Public License >> +# along with this program. =A0If not, see . >> + >> +# This file is part of the GDB testsuite. =A0It tests the mechanism >> +# exposing macros to Python. >> + >> +if $tracelevel then { >> + =A0 =A0strace $tracelevel >> +} >> + >> +load_lib gdb-python.exp >> + >> +set testfile "py-macro" >> +set srcfile ${testfile}.c >> +set binfile ${objdir}/${subdir}/${testfile} >> +get_compiler_info ${binfile} >> +if [test_compiler_info gcc*] { >> + =A0 =A0lappend options additional_flags=3D-g3 >> +} else { >> + =A0untested ${testfile}.exp >> + =A0return -1 >> +} >> + >> +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} $optio= ns] } { >> + =A0 =A0untested ${testfile}.exp >> + =A0 =A0return -1 >> +} >> + >> +# Start with a fresh gdb. >> +gdb_exit >> +gdb_start >> +gdb_reinitialize_dir $srcdir/$subdir >> +gdb_load ${binfile} >> + >> +# Skip all tests if Python scripting is not enabled. >> +if { [skip_python_tests] } { continue } > > This is something we should do for all Python tests, so I am not just > singling this one out. =A0But we should probably make this test earlier. > > Thanks for writing this patch. =A0I am really looking forward to it being > checked in when it is approved! > > Cheers, > > Phil >