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

[Pedro Alves <palves@redhat.com>]

(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