Re: [RFC - Python Scripting] New method gdb.Architecture.disassemble

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Siva" == Siva Chandra <sivachandra@google.com> writes:

Siva> Thanks for your detailed reply. I now have a patch which does not use
Siva> ui_out. Instead of calling gdb_disassembly, it essentially
Siva> re-implements disasm.c:dump_insns. The patch is attached.

I don't have any problem with this approach.
I do have some nits on the patch though.

Siva> +/* Implementation of gdb.Architecture.disassemble (self, low, high) -> List.
Siva> +   Returns a list of instructions in a memory address range.  Each instruction
Siva> +   in the list is a dict object with the following string keys:
Siva> +

You can write a shorter comment here and just put all the informative
stuff into the docs.

Siva> +  static char unknown_str[] = { '<', 'u', 'n', 'k', 'n', 'o', 'w', 'n', '>', '\0' };

This line looks too long.

I think it isn't too hard to reorganize so that this can be an ordinary
const char *.

Siva> +  result_list = PyList_New (0);
Siva> +  if (!result_list)
Siva> +    {
Siva> +      PyErr_SetString (PyExc_MemoryError,
Siva> +                       _("Unable to create a list of disassembled "
Siva> +                         "instructions."));
Siva> +      return NULL;

You don't need PyErr_SetString here.
PyList_New will have done that already.
So just return NULL.

Siva> +      struct ui_file *memfile = mem_fileopen ();
Siva> +      PyObject *insn_dict = PyDict_New ();
Siva> +      volatile struct gdb_exception except;
Siva> +
Siva> +      if (!insn_dict)
Siva> +        {
Siva> +          Py_DECREF (result_list);
Siva> +          PyErr_SetString (PyExc_MemoryError,
Siva> +                           _("Unable to create a dict for instruction."));
Siva> +
Siva> +          return NULL;

Here too.

This leaks memfile on failure.

Siva> +        }
Siva> +      if (PyList_Append (result_list, insn_dict))
Siva> +        {
Siva> +          Py_DECREF (result_list);
Siva> +          Py_DECREF (insn_dict);
Siva> +
Siva> +          return NULL;  /* PyList_Append Sets the exception.  */

This leaks it too.

Siva> +          if (funcname)
Siva> +            xfree (funcname);
Siva> +          if (filename)
Siva> +            xfree (filename);
Siva> +          if (asm_code)
Siva> +            xfree (asm_code);

xfree handles NULL arguments fine, so remove the "if"s.

Siva> +      if (PyDict_SetItemString (insn_dict, "addr",
Siva> +                                gdb_py_long_from_ulongest (pc))
Siva> +          || PyDict_SetItemString (insn_dict, "asm",
Siva> +                                   PyString_FromString (asm_code))

I think PyString_FromString handles const char * arguments fine.
So you can make this:

    asm_code == NULL ? unknown_str : asm_code

and avoid some hair above and below, and fix up unknown_str as well.

Siva> +        {
Siva> +          Py_DECREF (result_list);
Siva> +          PyErr_SetString (PyExc_MemoryError,
Siva> +                           _("Unable to add fields to instruction dict."));
Siva> +
Siva> +          return NULL;

You don't need the PyErr_SetString.
And this leaks some memory.

Siva> +      if (funcname && funcname != unknown_str)
Siva> +        xfree (funcname);
Siva> +      if (filename && filename != unknown_str)
Siva> +        xfree (filename);
Siva> +      if (asm_code && asm_code != unknown_str)
Siva> +        xfree (asm_code);

No need to check nullity; but with the other changes this could all be
unconditional.

Tom