On 09/26/2017 09:31 AM, Lluís Vilanova wrote:
> Lluís Vilanova writes:
>
>> Richard Henderson writes:
>>> On 09/14/2017 08:20 AM, Lluís Vilanova wrote:
>>>> Richard Henderson writes:
>>>>
>>>>> On 09/10/2017 09:27 AM, Lluís Vilanova wrote:
>>>>>> TCG BBLs and instructions have multiple exit points from where to raise
>>>>>> tracing events, but some of the necessary information in the generic
>>>>>> disassembly infrastructure is not available until after generating these
>>>>>> exit points.
>>>>>>
>>>>>> This patch adds support for "inline points" (where the tracing code will
>>>>>> be placed), and "inline regions" (which identify the TCG code that must
>>>>>> be inlined). The TCG compiler will basically copy each inline region to
>>>>>> any inline points that reference it.
>>>>
>>>>> I am not keen on this.
>>>>
>>>>> Is there a reason you can't just emit the tracing code at the appropriate
>>>>> place
>>>>> to begin with? Perhaps I have to wait to see how this is used...
>>>>
>>>> As I tried to briefly explain on next patch, the main problem without
>>>> inlining
>>>> is that we will see guest_tb_after_trans twice on the trace for each TB in
>>>> conditional instructions on the guest, since they have two exit points
>>>> (which we
>>>> capture when emitting goto_tb in TCG).
>
>>> Without seeing the code, I suspect this is because you didn't examine the
>>> argument to tcg_gen_exit_tb. You can tell when goto_tb must have been
>>> emitted
>>> and avoid logging twice.
>
>> The generated tracing code for 'guest_*_after' must be right before the
>> "goto_tb" opcode at the end of a TB (AFAIU generated by
>> tcg_gen_lookup_and_goto_ptr()), and we have two of those when decoding a
>> guest
>> conditional jump.
>
>> If we couple this with the semantics of the trace_*_tcg functions (trace the
>> event at translation time, and generate TCG code to trace the event at
>> execution
>> time), we get the case I described (we don't want to call
>> trace_tb_after_tcg()
>> or trace_insn_after_tcg() twice for the same TB or instruction).
>
>> That is, unless I've missed something.
>
>
>> The only alternative I can think of is changing tracetool to offer an
>> additional
>> API that provides separate functions for translation-time tracing and
>> execution-time generation. So from this:
>
>> static inline void trace_event_tcg(CPUState *cpu, TCGv_env env, ...)
>> {
>> trace_event_trans(cpu, ...);
>> if (trace_event_get_vcpu_state(cpu, EVENT_EXEC)) {
>> gen_helper_trace_event_exec(env, ...);
>> }
>> }
>
>> We can extend it into this:
>
>> static inline void gen_trace_event_exec(TCGv_env env, ...)
>> if (trace_event_get_vcpu_state(cpu, EVENT_EXEC)) {
>> gen_helper_trace_event_exec(env, ...);
>> }
>> }
>> static inline void trace_event_tcg(CPUState *cpu, TCGv_env env, ...)
>> {
>> trace_event_trans(cpu, ...);
>> gen_trace_event_exec(env, ...);
>> }
>
> Richard, do you prefer to keep the "TCG inline" feature or switch the internal
> tracing API to this second approach?
I don't think I fully understand what you're proposing. The example
transformation above is merely syntactic and has no functional change.
As previously stated, I'm not keen on the "tcg inline" approach. I would
prefer that you hook into tcg_gen_{exit_tb,goto_tb,goto_ptr} functions within
tcg/tcg-op.c to log transitions between TBs.
r~
