[Qemu-devel] [PATCH] Instruction counting instrumentation for ARM v3

[Sami Kiminki]

Hi,

Attached is a version of Timo Töyry's patch which should fix the issues
pointed out below. I'm not expecting this to get committed, but thought
to share it anyway, should anyone have use for it.

- Sami



On Mon, 2009-06-15 at 15:26 +0200, Laurent Desnogues wrote:
> 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

qemu-0.10.4-instrcount-2.patch
Description: Text Data