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

Pedro Alves
2015-10-20T11:29:21Z

(Closing the loop on v1... Sorry for the delay.)

On 10/12/2015 09:44 AM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
>> owner@sourceware.org] On Behalf Of Pedro Alves
>> Sent: Friday, October 9, 2015 2:51 PM
>> To: Metzger, Markus T; dje@google.com
>> Cc: gdb-patches@sourceware.org
>> Subject: Re: [PATCH 2/6] disasm: add struct disas_insn to describe to-be-
>> disassembled instruction
>
>
>> 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.
>
> I added the following to the beginning of the commit message to motivate
> the patch:
>
> The "record instruction-history" command prints for each instruction, in
> addition to the instruction's disassembly:
>
> - the instruction number in the recorded execution trace
> - a '?' before the instruction if it was executed speculatively
>
> To allow the "record instruction-history" command to use GDB's disassembly
> infrastructure, we extend dump_insn to optionally print those additional
> fields.

Ah, that's clears things, thanks.

>
>
>> Also, being a user/frontend visible change, shouldn't these new
>> fields be documented and mentioned in NEWS?
>
> There is no UI change and thus also no before/after example.
>
> The new optional fields are currently not used. They will be used by the
> "record instruction-history" command in the last patch of this series.
> Even then, there is no UI change. Both the "record instruction-history"
> and the "disassemble" command behave as they did before.
>

OK.

> There is a change to the MI output of "record instruction-history".
> As I didn't do any conscious MI support for record btrace, I don't expect
> it to be working. I'm using the ui_out_* functions so there might be some
> form of MI support. AFAIK it is not being used.

I see. Note that when an MI frontend invokes a CLI command (through
-interpreter-exec console ..."), the output it gets is still CLI output.
The fields passed to ui_out_* functions only get converted to MI attributes
if the command entered was a real MI command. IOW, if there's no MI command
equivalent of "record instruction-history", then there's no such thing as
'MI output of "record instruction-history"'.

This sentence:

"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."

was what made me believe there was some MI command that would now output
that field. But it now sounds to me that e.g., -data-disassemble output does
not really change. (If it does change, then we need to extend the manual where
it documents the "Result" of that command (see "GDB/MI Data Manipulation").)

Thanks,
Pedro Alves