Re: [PULL 04/13] target/rx: TCG helpers

[Peter Maydell]

On Tue, 17 Mar 2020 at 16:43, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> From: Yoshinori Sato <ysato@users.sourceforge.jp>
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> [PMD: Removed tlb_fill, extracted from patch of Yoshinori Sato
>  'Convert to CPUClass::tlb_fill']
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Message-Id: <20200224141923.82118-6-ysato@users.sourceforge.jp>
> Acked-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Somewhat late, but I've just noticed a bug in the helper_set_fpsw()
function introduced in this patch. The function has changed a little
since but the bug is still there in the version in git:

> +void helper_set_fpsw(CPURXState *env, uint32_t val)
> +{
> +    static const int roundmode[] = {
> +        float_round_nearest_even,
> +        float_round_to_zero,
> +        float_round_up,
> +        float_round_down,
> +    };
> +    uint32_t fpsw = env->fpsw;
> +    fpsw |= 0x7fffff03;
> +    val &= ~0x80000000;
> +    fpsw &= val;
> +    FIELD_DP32(fpsw, FPSW, FS, FIELD_EX32(fpsw, FPSW, FLAGS) != 0);

FIELD_DP32() does not update its first argument, it merely reads
it. It returns the new value with the field change applied, so
you need to use it like this:

    fpsw = FIELD_DP32(fpsw, ....);

Would somebody like to write a patch ?

(I noticed this because I just made the same mistake in some new
code I was writing, so I did a quick grep of the codebase to see
if there were any instances of it already present. I think the macro
magic used in the definitions of FIELD_DP* to provide a compile error
if you pass a value that's bigger than the target field has the
unfortunate side effect of suppressing the compiler warning that the
whole statement has no effect.)

thanks
-- PMM