Re: [Qemu-devel] [PATCH] target/arm: crash on conditional instr in it block

[Roman Kapl]

Hi and thanks for review,

On 08/14/2018 08:12 PM, Peter Maydell wrote:
> On 14 August 2018 at 17:54, Roman Kapl <address@hidden> wrote:
> > If an instruction is conditional (like CBZ) and it is executed conditionally
> > (using the ITx instruction), a jump to undefined label is generated.
> >
> > Fix the 'skip on condtion' code to create a new label only if it does not
> > already exist. Previously multiple labels were created, but only the last one of
> > them was set.
>
> Hi; thanks for the bug report and the patch.
>
> This case (CBZ inside an IT block) is architecturally UNPREDICTABLE,
> but we certainly shouldn't crash QEMU.

Hm... I am not able to find that claim in my ARMv7 reference manual (but 
I am no ARM expert). However, I know that the assembler won't let you 
generate it. ARMv8 seems to deprecate most IT uses anyway.
> For ARMv7 we have very wide latitude in what we can do; for ARMv8
> the "CONSTRAINED UNPREDICTABLE" part of the Arm ARM limits us a
> bit (see section K1.1.7). We can do one of:
>   * generate an UNDEFINED exception
>   * execute the instruction as if the condition code check had passed
>   * execute the instruction as if the condition check failed (ie NOP)
> (and we're allowed to behave differently from execution to execution
> so "honour the condition code check" is ok too I think).
>
> If we don't UNDEF then we have to be a little careful about the
> IT state, but I think we're OK there.
>
> So we could if we like choose to just UNDEF in these cases,
> something like:
>     if (dc->condexec_mask && thumb_insn_unpredictable_in_it(dc, insn) {
>         gen_exception_insn(...)
>     }
> where we currently check for "always unconditional even in
> an IT block" insns.
>
> But I guess that just bringing conditional branches into line
> with what we already do for other insns in this category is
> a reasonable approach. I have a comment below but otherwise
> the patch looks good.
>
> >   target/arm/translate.c | 32 ++++++++++++++++++--------------
> >   1 file changed, 18 insertions(+), 14 deletions(-)
> >
> > diff --git a/target/arm/translate.c b/target/arm/translate.c
> > index f845da7c63..f7c03a36e6 100644
> > --- a/target/arm/translate.c
> > +++ b/target/arm/translate.c
> > @@ -8480,6 +8480,16 @@ static void gen_srs(DisasContext *s,
> >       s->base.is_jmp = DISAS_UPDATE;
> >   }
> >
> > +/* Skip this instruction if the condition is true */
> > +static void arm_conditional_skip(DisasContext *s, uint32_t cond)
> > +{
> > +    if (!s->condjmp) {
> > +        s->condlabel = gen_new_label();
> > +        s->condjmp = 1;
> > +    }
> > +    arm_gen_test_cc(cond, s->condlabel);
> > +}
>
> This is a nice piece of refactoring. I think it would make
> more sense to have it do "skip if condition is false", though:
> currently every callsite has to xor the condition with 1.
>
> (You could call it arm_skip_insn_unless(), maybe.)

Well, personally I am not a big fan of unless, double negation etc. 
(they require an extra mental effort for me). But if you prefer, I will 
send a second patch with this change.

> > +
> >   static void disas_arm_insn(DisasContext *s, unsigned int insn)
> >   {
> >       unsigned int cond, val, op1, i, shift, rm, rs, rn, rd, sh;
> > @@ -8709,9 +8719,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int 
> > insn)
> >       if (cond != 0xe) {
> >           /* if not always execute, we generate a conditional jump to
> >              next instruction */
> > -        s->condlabel = gen_new_label();
> > -        arm_gen_test_cc(cond ^ 1, s->condlabel);
> > -        s->condjmp = 1;
> > +        arm_conditional_skip(s, cond ^ 1);
> >       }
> >       if ((insn & 0x0f900000) == 0x03000000) {
> >           if ((insn & (1 << 21)) == 0) {
> > @@ -11205,9 +11213,7 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t 
> > insn)
> >                   /* Conditional branch.  */
> >                   op = (insn >> 22) & 0xf;
> >                   /* Generate a conditional jump to next instruction.  */
> > -                s->condlabel = gen_new_label();
> > -                arm_gen_test_cc(op ^ 1, s->condlabel);
> > -                s->condjmp = 1;
> > +                arm_conditional_skip(s, op ^ 1);
> >
> >                   /* offset[11:1] = insn[10:0] */
> >                   offset = (insn & 0x7ff) << 1;
> > @@ -12131,8 +12137,10 @@ static void disas_thumb_insn(DisasContext *s, uint32_t 
> > insn)
> >           case 1: case 3: case 9: case 11: /* czb */
> >               rm = insn & 7;
> >               tmp = load_reg(s, rm);
> > -            s->condlabel = gen_new_label();
> > -            s->condjmp = 1;
> > +            if (!s->condjmp) {
> > +                s->condlabel = gen_new_label();
> > +                s->condjmp = 1;
> > +            }
> >               if (insn & (1 << 11))
> >                   tcg_gen_brcondi_i32(TCG_COND_EQ, tmp, 0, s->condlabel);
> >               else
> > @@ -12295,9 +12303,7 @@ static void disas_thumb_insn(DisasContext *s, uint32_t 
> > insn)
> >               break;
> >           }
> >           /* generate a conditional jump to next instruction */
> > -        s->condlabel = gen_new_label();
> > -        arm_gen_test_cc(cond ^ 1, s->condlabel);
> > -        s->condjmp = 1;
> > +        arm_conditional_skip(s, cond ^ 1);
> >
> >           /* jump to the offset */
> >           val = (uint32_t)s->pc + 2;
> > @@ -12676,9 +12682,7 @@ static void thumb_tr_translate_insn(DisasContextBase 
> > *dcbase, CPUState *cpu)
> >           uint32_t cond = dc->condexec_cond;
> >
> >           if (cond != 0x0e) {     /* Skip conditional when condition is AL. */
> > -            dc->condlabel = gen_new_label();
> > -            arm_gen_test_cc(cond ^ 1, dc->condlabel);
> > -            dc->condjmp = 1;
> > +            arm_conditional_skip(dc, cond ^ 1);
> >           }
> >       }
> >
> > --
>
> thanks
> -- PMM