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?
Thanks,
Lluis
