From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36682) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dsq9s-0000VY-Nc for qemu-devel@nongnu.org; Fri, 15 Sep 2017 08:55:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dsq9n-0004oC-Hm for qemu-devel@nongnu.org; Fri, 15 Sep 2017 08:55:48 -0400 Received: from roura.ac.upc.es ([147.83.33.10]:54984) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dsq9m-0004nG-T2 for qemu-devel@nongnu.org; Fri, 15 Sep 2017 08:55:43 -0400 From: =?utf-8?Q?Llu=C3=ADs_Vilanova?= References: <150505986682.19604.11937392314067517230.stgit@frigg.lan> <150506083546.19604.543091497330269756.stgit@frigg.lan> <169db8d7-fe49-05c4-aca7-ad818b12c9c5@linaro.org> <87a81xpasb.fsf@frigg.lan> <7e4260a8-89a3-9c52-4f09-afe9035ceac2@twiddle.net> Date: Fri, 15 Sep 2017 15:55:33 +0300 In-Reply-To: <7e4260a8-89a3-9c52-4f09-afe9035ceac2@twiddle.net> (Richard Henderson's message of "Thu, 14 Sep 2017 09:15:18 -0700") Message-ID: <87ingki0je.fsf@frigg.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 4/7] tcg: Add support for "inlining" regions of code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson Cc: Richard Henderson , qemu-devel@nongnu.org, Stefan Hajnoczi Richard Henderson writes: > On 09/14/2017 08:20 AM, Llu=C3=ADs Vilanova wrote: >> Richard Henderson writes: >>=20 >>> On 09/10/2017 09:27 AM, Llu=C3=ADs 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 the= se >>>> exit points. >>>>=20 >>>> This patch adds support for "inline points" (where the tracing code wi= ll >>>> 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. >>=20 >>> I am not keen on this. >>=20 >>> Is there a reason you can't just emit the tracing code at the appropria= te place >>> to begin with? Perhaps I have to wait to see how this is used... >>=20 >> As I tried to briefly explain on next patch, the main problem without in= lining >> 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 em= itted > 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 gu= est 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 exec= ution time), we get the case I described (we don't want to call trace_tb_after_tc= g() 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 addit= ional 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, ...); } Cheers, Lluis