This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC - Python Scripting] New method gdb.Architecture.disassemble
>>>>> "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