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

[Philippe Mathieu-Daudé]

On 12/9/21 17:04, Peter Maydell wrote:
> 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

I tried adding __attribute__((warn_unused_result)) to depositXX()
but it doesn't trigger, apparently due to how FIELD_DP32() macro
is written.

> you need to use it like this:
> 
>     fpsw = FIELD_DP32(fpsw, ....);
> 
> Would somebody like to write a patch ?

Will do, thanks for the report.

> (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
>