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

[Tom Tromey <tromey@redhat.com>]

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

Siva> The attached patch adds a new method 'disassemble' to the class
Siva> gdb.Architecture.  I have not yet added docs and tests, but will do so
Siva> after I get feedback that adding such a method is OK.

Why is it a method on gdb.Architecture and not elsewhere?
For example, we have gdb.Inferior.read_memory.

Siva> +  if (!PyArg_ParseTuple (args, "KK|i", &low, &high, &opcodes))

Usually if we support more than one argument, we allow keywords as well.

Siva> +  if (opcodes)
Siva> +    flags = DISASSEMBLY_RAW_INSN;

Why an int treated as a boolean rather than a real boolean?

Siva> +  TRY_CATCH (except, RETURN_MASK_ALL)
Siva> +    {
Siva> +      gdb_disassembly (gdbarch, py_out, NULL, flags, -1, low, high);

Why is py_out a global instead of something created and destroyed here?

Siva> +  if (! (PyList_Check (temp) && PyList_Size (temp) > 0))
Siva> +    return NULL;

You can't return NULL without setting the exception.

Siva> +struct ui_out *py_out;

I'd rather avoid new globals, and I don't see what this one adds.

Siva> +/* This data structure captures the Python version of ui_out.  The Python
Siva> +   version is not used to display output to a user, but to capture the results
Siva> +   from GDB's internals in to a Python data structure.  Hence, it does not have
Siva> +   any representation for table headers.  However, it can be viewed as a
Siva> +   recursive table structure wherin the highest level is a list of rows.  All
Siva> +   rows in this list can either be a list themselves, or all of them can be
Siva> +   dicts holding the table's fields.  If they were lists, then they follow the
Siva> +   same recurrsive structure as the higher levels.

Typo, "recursive".

I like this -- I've wanted it before -- but I wonder whether it handles
all cases.  See http://sourceware.org/bugzilla/show_bug.cgi?id=11688#c6

Siva> +static struct row_data *
Siva> +new_row (struct row_data *parent)
Siva> +{
Siva> +  struct row_data *row;
Siva> +
Siva> +  row = (struct row_data *) xmalloc (sizeof (struct row_data));

You don't need the cast.

Siva> +PyObject *
Siva> +fetch_and_reset_py_out_object (struct ui_out *ui_out)

All new functions should have an intro comment.

Siva> +  temp = py_out_data->table->data;
Siva> +  py_out_data->table->data = PyList_New (0);

What if PyList_New returns NULL here?
Then the exception will be set, making problems later.

Siva> +  /* Commit the row to the parent list.  */
Siva> +  PyList_Append (py_out_data->current_row->parent_row->data,
Siva> +                 py_out_data->current_row->data);

What if this fails?

This applies to many calls into Python in the patch.

Siva> +static struct ui_out_impl py_ui_out_impl = 
Siva> +{
Siva> +  0,                        /* table_begin  */
Siva> +  0,                        /* table_body  */
Siva> +  0,                        /* table_end  */

Why are these NULL?

Siva> +  0                         /* is_mi_like_p  */

It seems to me that this should be 1.
(Well, really, this is all a hack that should go away, but I doubt I'll
live to see that.)

Tom