[Qemu-devel] Re: instrument

[Lluís]

Blue Swirl writes:

> On Mon, Oct 25, 2010 at 11:13 AM, Lluís <address@hidden> wrote:
>> Blue Swirl writes:
>> 
>>> A lot of internal state needs to be made available to the
>>> instrumentation code, which is a problem if that is considered as an
>>> API.
>> 
>> I don't get what you mean by that. In principle, only the macro
>> arguments and the cpu instrumentation state are used.

> I was referring to instr_disas, cpu_single_env hacks.

Aaahhh... yes, those are _extremely_ nasty hacks.

The 'instr_disas' case is mainly because:

1) It is not passed down to all gen_* functions in
   target-i386/translate.c, and I thought passing it would be too
   disruptive (plus could add some extra overheads if the compiler does
   not do a good job optimizing the argument passing).

2) On other non-CISC targets, obtaining all the opcode and register
   usage information can be easily accomplished by, e.g.,
   embedding/parsing the info on the opcode translation tables (e.g.,
   'opcodes' table in target-ppc).

The 'cpu_single_env' case I think is less serious, as it is expected to
contain a valid pointer when translating and executing code (all inside
cpu_exec). The hack I added in linux-user/exec.c was just for an early
initialization thing.

Maybe there's a better approach, but that's the only way I could find to
access the current per-CPU instrumentation state.


>> Hmmm... as I see it, if tracing provided a two-level backend, I could
>> work this out.
>> 
>> I mean, for me, trace-points are just user-definable macros, I could
>> provide instrumentation points through that, such that trace.h can
>> include (if directed to do it) an extra user-provided header. Then,
>> trace.h could look like:
>> 
>>  #include "qemu-common.h"
>> 
>>  #include "trace-override.h" // user defines trace_whatever
>> 
>>  #ifndef trace_whatever
>>  // maybe provide a separate trace-events-override file and do not
>>  // generate this with tracetool from trace-events
>>  static inline void trace_whatever(args...)
>>  {
>>    call trace backend
>>  }
>>  #endif

> I had a pretty similar concept in mind when I suggested that
> tracepoints could be extended, the syntax for trace-events could
> become from:
> disable apic_set_irq(int apic_irq_delivered) "coalescing %d"
> to something like:
> disable apic_set_irq(int apic_irq_delivered) "coalescing %d"
> apic_irq_instr(void)

Well, if it is to be openly exposed, I could rewrite tracetool to parse
lines in "trace-events" like:

 [disable|instrument] <point_name>(<args>) <format>

Thus, using the keyword "instrument" would indicate "tracetool" to avoid
generating any code for that point, and include a pre-defined header
where the user is expected to provide the declaration/implementation of
that point.

This is less elegant than providing two fully-separate layers (e.g.,
QEMU code only calls instrumentation points, and enabling tracing
provides a default instrumentation implementation that calls the
selected tracing method), but is much easier to maintain, as it spans a
single "trace-events" file and is managed by a single "tracetool" file.

I'd prefer building tracing as a specific instrumentation case, but then
probably some developers would suddenly want to kill me :)


Still, two points remain open:

1) Some events cannot be translated into a simple tracing call.  The
   only way I see to circumvent this is to add some keyword to the point
   name, such that tracetool is able to generate a TCG call to the
   tracing backend.

   For example, calling 'trace_name' refers to 'name' in "trace-events",
   and calling 'instr_name' refers to 'instr_name' in
   "trace-events". Although the naming scheme is not simmetric.

2) Without some notion of the per-CPU instrumentation state, points
   inserted during TCG code generation will show an excessive slow down
   on environments with selective dynamic tracing.


>> Of course, instrumentation points found during code generation still
>> need a different treatment even if they are to be used as plain
>> trace-points, as they're not called at run-time unless you generate a
>> call to a helper, so maybe a new trace-point type should be handled on
>> 'tracetool' (like trace_generate_<whatever> or something like that),
>> that generates a call to the tracing backend instead of simply calling
>> it.
>> 
>> This way, trace generation from these points can be neatly integrated
>> with all the other tracing points.

> Maybe the tracepoints should be separate from these helpers (which may
> or may not have tracepoints).

Well, using "disable" on a point prefixed with "instr_" could default to
producing an empty stub even when tracing is configured in.


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth