[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Instruction counting instrumentation for ARM,
From: |
Laurent Desnogues |
Subject: |
Re: [Qemu-devel] [PATCH] Instruction counting instrumentation for ARM, 2nd version |
Date: |
Mon, 15 Jun 2009 15:26:30 +0200 |
On Fri, Jun 12, 2009 at 12:56 PM, Sami Kiminki<address@hidden> wrote:
[...]
>
> There are also other ways to implement instruction counting (see
> responses to our previous patch [1]). As to our approach, I believe this
> is the cleanest way to implement this specific instrumentation. However,
> from the general point of view, I'm not so sure. I'd like to ask for
> more comments and options.
Apart from the fact your patch doesn't contain instrumentation.[ch]
it is bogus in several places and shows why I think the approach
is probably not the best.
E.g.:
@@ -6300,6 +6809,7 @@ static void disas_arm_insn(CPUState * en
if (insn & (1 << 20)) {
gen_helper_mark_exclusive(cpu_env, cpu_T[1]);
switch (op1) {
+ instr_count_inc(ARM_INSTRUCTION_LDREX);
case 0: /* ldrex */
tmp = gen_ld32(addr, IS_USER(s));
break;
This is misplaced.
- if (op1 & 2)
+ if (op1 & 2) {
gen_helper_double_saturate(tmp2, tmp2);
- if (op1 & 1)
+ if (op1 & 1) instr_count_inc(ARM_INSTRUCTION_QDSUB);
+ else instr_count_inc(ARM_INSTRUCTION_QDADD);
+ }
+ if (op1 & 1) {
gen_helper_sub_saturate(tmp, tmp, tmp2);
- else
+ instr_count_inc(ARM_INSTRUCTION_QSUB);
+ }
+ else {
gen_helper_add_saturate(tmp, tmp, tmp2);
+ instr_count_inc(ARM_INSTRUCTION_QADD);
+ }
Here your two last instr_count_inc will overwrite the first two ones.
So you don't distinguish between QDADD/QDSUB and
QADD/QSUB.
I also would like to see a more general instrumentation framework
but your approach is too intrusive and so it's too easy to introduce
hard to detect bugs.
Cheers,
Laurent