Re: [PATCH] target/arm: Avoid over-length shift in arm_cpu_sve_finalize() error case

[Richard Henderson <richard.henderson@linaro.org>]

On 7/4/23 18:00, Alex Bennée wrote:
> 
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> If you build QEMU with the clang sanitizer enabled, you can see it
>> fire when running the arm-cpu-features test:
>>
>> $ QTEST_QEMU_BINARY=./build/arm-clang/qemu-system-aarch64 ./build/arm-clang/tests/qtest/arm-cpu-features
>> [...]
>> ../../target/arm/cpu64.c:125:19: runtime error: shift exponent 64 is too large for 64-bit type 'unsigned long long'
>> [...]
>>
>> This happens because the user can specify some incorrect SVE
>> properties that result in our calculating a max_vq of 0.  We catch
>> this and error out, but before we do that we calculate
>>
>>   vq_mask = MAKE_64BIT_MASK(0, max_vq);$
>>
>> and the MAKE_64BIT_MASK() call is only valid for lengths that are
>> greater than zero, so we hit the undefined behaviour.
> 
> Hmm that does make me worry we could have more land mines waiting to be
> found. Would converting MAKE_64BIT_MASK into an inline function and
> asserting be a better solution?

I'd be tempted to keep a macro, and use __builtin_constant_p to make sure this expands to 
a constant if possible.  Ideally constants would be diagnosed at compile-time and runtime 
values get runtime asserts.


r~