Re: [Qemu-devel] [PATCH 3/7] trace: Add event "guest_inst_info_before"

[Lluís Vilanova]

Richard Henderson writes:

> On 09/10/2017 09:23 AM, Lluís Vilanova wrote:
>> Signed-off-by: Lluís Vilanova <address@hidden>
>> ---
>> accel/tcg/translator.c |   18 ++++++++++++++++++
>> trace-events           |    9 +++++++++
>> 2 files changed, 27 insertions(+)
>> 
>> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
>> index 287d27b4f7..6598931171 100644
>> --- a/accel/tcg/translator.c
>> +++ b/accel/tcg/translator.c
>> @@ -70,6 +70,8 @@ void translator_loop(const TranslatorOps *ops, 
>> DisasContextBase *db,
>> 
>> while (true) {
>> target_ulong pc_insn = db->pc_next;
>> +        TCGv_i32 insn_size_tcg = 0;
>> +        int insn_size_opcode_idx;

> Initializing a TCGv_i32 is wrong.
> And surely insn_size_opcode is surely uninitialized?

>> +        if (TRACE_GUEST_INST_INFO_BEFORE_EXEC_ENABLED) {
>> +            insn_size_tcg = tcg_temp_new_i32();
>> +            insn_size_opcode_idx = tcg_op_buf_count();
>> +            tcg_gen_movi_i32(insn_size_tcg, 0xdeadbeef);
>> +
>> +            trace_guest_inst_info_before_tcg(
>> +                cpu, tcg_ctx.tcg_env, pc_insn, insn_size_tcg);
>> +
>> +            tcg_temp_free_i32(insn_size_tcg);

> There's no reason you can't declare insn_size_tcg right here and avoid the
> incorrect initialization above.

Yes, I guess I did not move the declaration here by error after refactoring the
code.


> Is there a reason to have both "guest_insn" and "guest_insn_info"?

I initially wanted to have a bare-bones event with simple information, and an
*_info variant with more detailed information like register usage and physical
 addresses (which would be disabled by default to avoid performance impact).

We had a discussion long time ago that led to decide that register usage
information as I implemented it was only partial (it did not capture register
usage helpers), and thus was not worth adding.

Since physical address information is not gonna be added in this series (if at
all), what do you say about hoisting instruction length info into
guest_insn_before/after and dropping the *_info variants?


Thanks,
  Lluis