Blue Swirl writes:
> On Mon, Oct 25, 2010 at 11:13 AM, Lluís <xscr...@gmx.net> 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
