This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH][gdb/python] Add interface to access minimal_symbols
- From: Tom Tromey <tom at tromey dot com>
- To: Tom de Vries <tdevries at suse dot de>
- Cc: Simon Marchi <simon dot marchi at ericsson dot com>, gdb-patches at sourceware dot org, Phil Muldoon <pmuldoon at redhat dot com>, Tom Tromey <tom at tromey dot com>
- Date: Thu, 29 Nov 2018 15:32:32 -0700
- Subject: Re: [PATCH][gdb/python] Add interface to access minimal_symbols
- References: <20181004211115.GA31056@delia> <39c4336d-c749-6f79-5a29-0b764fc4935e@ericsson.com> <211c4746-389a-93b7-faf9-c8f9b6245541@suse.de>
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
Tom> This patch adds a new gdb.MinSymbol object to export the minimal_symbol
Tom> interface.
I hate to bikeshed the name to start with, but I'm going to anyway. Why
the abbreviated name rather than gdb.MinimalSymbol? Or alternatively
why not something less "gdb-ish", like gdb.LinkerSymbol?
Tom> +Typical symbols like functions, variables, etc are represented by
Tom> +@code{gdb.Symbol} objects in Python. Some symbols are defined with less
Tom> +information associated with them, like linker script variables
Tom> +or assembly labels. Python represents these minimal symbols in @value{GDBN}
Tom> +with the @code{gdb.MinSymbol} object.
I think this is mildly misleading and perhaps should also mention that
linker symbols will generally end up in here.
Tom> +@defvar MinSymbol.filename
Tom> +The file name of the source file where the minimal symbol is defined.
As mentioned elsewhere in the thread, I think this field is largely not
useful. I feel like we once considered removing it from minsyms
entirely. So, I tend to think this one should just be dropped, unless
you have some specific need for it (which would be interesting to hear
about).
Tom> +@defvar MinSymbol.section_name
Tom> +The name of the section in the object file containing this minimal symbol.
Are there platforms where sections do not have names?
What will happen to this on those?
Tom> + return PyString_FromString (minsym->filename);
I suspect filename can be NULL here.
But see above.
Tom> + if (section != NULL)
Tom> + {
Tom> + name = bfd_section_name (objfile->obfd, section->the_bfd_section);
Tom> + if (name != NULL)
Tom> + return PyString_FromString (name);
Tom> + }
Tom> +
Tom> + Py_RETURN_NONE;
Ok, I see the answer to my question here. This should be documented.
Tom> + type = builtin_type (minsym_gdbarch (self))->builtin_func_ptr;
Tom> +
Tom> + if (minsym_type (self) == type)
Tom> + Py_RETURN_TRUE;
This seems kind of roundabout to me.
Tom> + type = builtin_type (minsym_gdbarch (self))->builtin_data_ptr;
Tom> +
Tom> + if (minsym_type (self) == type)
Tom> + Py_RETURN_TRUE;
Same here.
Tom> + minsym_object *minsym_obj = (minsym_object *)self;
Space after the ")".
Tom> +static void
Tom> +set_symbol (minsym_object *obj, struct bound_minimal_symbol *bound)
I think a const reference for "bound" would be more natural here.
Tom> +static PyObject *
Tom> +bound_minsym_to_minsym_object (struct bound_minimal_symbol *bound)
Here too.
Tom> + if (sfile_obj != NULL && sfile_obj != Py_None)
Tom> + {
Tom> + sfile_tmp = gdbpy_obj_to_string (sfile_obj);
If this returns NULL then this function should early-return as well.
Tom> + if (bound_minsym.minsym != NULL)
Tom> + msym_obj = bound_minsym_to_minsym_object (&bound_minsym);
If bound_minsym_to_minsym_object returns NULL, then an exception has
been set, so this function must return NULL. But:
Tom> + if (msym_obj != NULL)
Tom> + return msym_obj;
... that isn't happening.
So the logic needs a bit of reordering here.
Tom> +/* Initialize gdb.MinSymbol. Return -1 on error, 0 on success. */
Tom> +
Tom> +int
Tom> +gdbpy_initialize_minsymbols (void)
Don't need the "void" here any more.
Tom> +/* Return struct objfile reference that is wrapped by the SELF object. */
Tom> +
Tom> +struct objfile *
Tom> +objectfile_object_to_objfile (PyObject *self)
I think this should be named objfile_object_to_objfile instead.
Tom> +++ b/gdb/testsuite/gdb.python/py-minsymbol.exp
Some of the lines here are too long and can reasonably be split.
I think it would also make sense to check for the minsym for "main"
itself.
Tom