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

[Peter Maydell]

On 16 August 2018 at 13:05, 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.
> CBZ in IT block is an unpredictable behavior, and honouring the condition code
> is allowed by the spec in this case (constrained unpredictable, ARMv8, section
> K1.1.7).
>
> 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.
>
> Signed-off-by: Roman Kapl <address@hidden>
> ---
> v1 -> v2
>  Split arm_conditional_skip into arm_gen_condlabel and arm_skip_unless to
>  a) cover all usages
>  b) do not force callers to ^1 the condition
>
>  Add note about CBZ in IT block to the commit message.
>
>  target/arm/translate.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index f845da7c63..e54e0ca2ba 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -8480,6 +8480,22 @@ static void gen_srs(DisasContext *s,
>      s->base.is_jmp = DISAS_UPDATE;
>  }
>
> +/* Generate a labe used for skipping this instruction */

"label"

> +static void arm_gen_condlabel(DisasContext *s)
> +{
> +    if (!s->condjmp) {
> +        s->condlabel = gen_new_label();
> +        s->condjmp = 1;
> +    }
> +}
> +
> +/* Skip this instruction if the ARM condition is false */
> +static void arm_skip_unless(DisasContext *s, uint32_t cond)
> +{
> +    arm_gen_condlabel(s);
> +    arm_gen_test_cc(cond, s->condlabel ^ 1);

This has applied the ^ 1 to the wrong argument. This causes
a compile error (at least if you're building with debug):

/home/petmay01/linaro/qemu-from-laptop/qemu/target/arm/translate.c: In
function ‘arm_skip_unless’:
/home/petmay01/linaro/qemu-from-laptop/qemu/target/arm/translate.c:8496:40:
error: invalid operands to binary ^ (have ‘TCGLabel * {aka struct
TCGLabel *}’ and ‘int’)
     arm_gen_test_cc(cond, s->condlabel ^ 1);
                                        ^

Rather than make you resend the patch, I'll just fix this up locally.

thanks
-- PMM