Re: [PATCH 2/6] disasm: add struct disas_insn to describe to-be-disassembled instruction

Pedro Alves
2015-10-09T12:51:17Z

On 09/21/2015 03:54 PM, Markus Metzger wrote:
> Add a new struct disas_insn to add additional fields describing the
> to-be-disassembled instruction. The additional fields are:
>
> number an optional instruction number, zero if omitted.
> is_speculative a predicate saying whether the instruction was
> executed speculatively.
>
> Replace the PC parameter of dump_insn with a pointer to the above struct.
>
> If non-zero, the instruction number is printed first. It will also appear
> as a new optional field "insn-number" in MI. The field will be present if
> insn_num is non-zero.
>
> If is_speculative is set, speculative execution will be indicated by a "?"
> following the new instruction number field. Unless the PC is omitted, it
> will overwrite the first byte of the PC prefix. It will appear as a new
> optional field "is-speculative" in MI. The field will contain "?" and will
> be present if is_speculative is set.
>

I think the log would be much clearer if the rationale was specified in
terms of why this is necessary, and if we saw a before/after example.

Also, being a user/frontend visible change, shouldn't these new
fields be documented and mentioned in NEWS?

> --- a/gdb/disasm.h
> +++ b/gdb/disasm.h
> @@ -27,11 +27,25 @@
> #define DISASSEMBLY_FILENAME (0x1 << 3)
> #define DISASSEMBLY_OMIT_PC (0x1 << 4)
> #define DISASSEMBLY_SOURCE (0x1 << 5)
> +#define DISASSEMBLY_SPECULATIVE (0x1 << 6)
>
> struct gdbarch;
> struct ui_out;
> struct ui_file;
>
> +/* An instruction to be disassembled. */
> +
> +struct disas_insn {

{ goes on the next line.

Thanks,
Pedro Alves