Re: [PATCH] MI disassemble opcode support

Joel Brobecker
2010-12-28T05:47:53Z

> 2010-12-10 Andrew Burgess <aburgess@broadcom.com>
>
> * disasm.c (dump_insns): Build the opcodes into a stream so
> that we can dump them as a field for MI.
>
> gdb/doc/
>
> 2010-12-10 Andrew Burgess <aburgess@broadcom.com>
>
> * gdb.texinfo: Update to reflect changes in mi/mi-cmd-disas.c
>
> gdb/mi/
>
> 2010-12-10 Andrew Burgess <aburgess@broadcom.com>
>
> * mi-cmd-disas.c (mi_cmd_disassemble): Allow mode to control
> dumping of instruction opcodes.
>
> gdb/testsuite/
>
> 2010-12-10 Andrew Burgess <aburgess@broadcom.com>
>
> * gdb.mi/mi-disassemble.exp, gdb.mi/mi2-disassemble.exp: Update
> expected output to reflect changes in gdb/mi/mi-cmd-disas.c and
> add new tests to check opcode dumping works.

Pre-approved with the following changes made:

I think a NEWS entry might be worthwhile (this can be treated as a
separate patch - maybe ask Eli if he thinks it's significant enough
to be mentioned there).

The alignment of your ChangeLog entries is incorrect. Everything should
be aligned on a tabulation. Therefore:

* gdb.mi/mi-disassemble.exp, gdb.mi/mi2-disassemble.exp: Update
expected output to reflect changes in gdb/mi/mi-cmd-disas.c and
add new tests to check opcode dumping works.

A general comment: The ChangeLog should explain the WHAT. If you feel
that you need to explain the WHY, that comment should be in the code.
For instance:

> + const char *spacer = "";
> +
> + /* Temporary stream for building up the opcodes */

You can put the reason why you are building the opcode in a stream here
instead of inside the ChangeLog.

> + fprintf_filtered(opcode_stream->stream, "%s%02x",
> + spacer, (unsigned)data);
> + spacer = " ";

Formatting: space before the first '(', and also space after the
unsigned cast. I know the second one is not your doing, but might
as well fix it.

> error
> ("mi_cmd_disassemble: Usage: [-f filename -l linenum [-n howmany]] [-s startaddr -e endaddr] [--] mixed_mode.");

This error message need to be updated (replace "mixed_mode" by "mode").

> + /* Convert the mode into a set of disassembly flags */

Period and 2 spaces at the end of the sentence.

--
Joel