Re: [PATCH v2] Add support for symbol addition to the Python API

[Tom Tromey <tom@tromey.com>]

>>>>> Matheus Branco Borella <dark.ryu.550@gmail.com> writes:

> I had to walk away from this for a while. I'm pinging it now and I've updated
> the code so that it works on master.

Thank you for the patch.

> This patch adds support for symbol creation and registration. It currently
> supports adding type symbols (VAR_DOMAIN/LOC_TYPEDEF), static symbols
> (VAR_DOMAIN/LOC_STATIC) and goto target labels (LABEL_DOMAIN/LOC_LABEL).

Symbol domains recently went through a change.

Also, a patch that changes the Python API requires a documentation
change and also an entry in NEWS.

> In the same vein, PC-based function name lookup also does not work, although
> there may be a way to have the feature work using overlays.

I guess because nothing makes any blocks.  However this seems like a
kind of big issue to me, because it means that by-name lookups will
appear to succeed ("function xyz is at address 0xaaaaa") but then
stopping in that function won't show the name.

> +  virtual void register_msymbol (const std::string& name,

gdb style puts the "&" next to "name", not next to "string".
There's a lot of instances of this.

> +/* Data being held by the gdb.ObjfileBuilder.
> + *
> + * This structure needs to have its constructor run in order for its lifetime
> + * to begin. Because of how Python handles its objects, we can't just reconstruct
> + * the object structure as a whole, as that would overwrite things the runtime
> + * cares about, so these fields had to be broken off into their own structure. */

gdb doesn't use the "leading *" style of comment.

> +  /* We need to tell GDB what architecture the objfile uses. */
> +  if (has_stack_frames ())
> +    of->per_bfd->gdbarch = get_frame_arch (get_selected_frame (nullptr));
> +  else
> +    of->per_bfd->gdbarch = current_inferior ()->arch ();

gdb exposes a gdb.Architecture, maybe we could let the Python code
specify this.

> +/* Parses a language from a string (coming from Python) into a language
> + * variant. */
> +
> +static enum language
> +parse_language (const char *language)
> +{
> +  if (strcmp (language, "c") == 0)
> +    return language_c;
> +  else if (strcmp (language, "objc") == 0)
> +    return language_objc;

I think this should call language_enum instead.

> +  if (language_name == nullptr)
> +    language_name = "auto";

I think it's kind of weird to use auto here.

> +/* Builds the object file. */
> +static PyObject *
> +objbdpy_build (PyObject *self, PyObject *args)
> +{
> +  auto builder = validate_objfile_builder_object (self);
> +  if (builder == nullptr)
> +    return nullptr;
> +
> +  if (builder->inner.installed)
> +    {
> +      PyErr_SetString (PyExc_ValueError, "build() cannot be run twice on the \
> +		       same object");
> +      return nullptr;
> +    }
> +  auto of = build_new_objfile (*builder);

There's a rule in the Python layer in gdb that code that calls into gdb
has to wrap the call in a try/catch and use GDB_PY_HANDLE_EXCEPTION.
This is because a lot of gdb code can throw exceptions, but letting an
exception cross the Python boundary is catastrophic.

> +  auto objpy = objfile_to_objfile_object (of).get ();
> +  Py_INCREF(objpy);
> +  return objpy;

objfile_to_objfile_object returns a new reference so I think the incref
is wrong here.

We try to avoid explicit inc/dec-refs in gdb anyway.

Tom