Re: [PATCH v2 3/5] target/s390x: Fix cc_calc_sla_64() missing overflows

[David Hildenbrand]

On 12.01.22 05:39, Ilya Leoshkevich wrote:
> An overflow occurs for SLAG when at least one shifted bit is not equal
> to sign bit. Therefore, we need to check that `shift + 1` bits are
> neither all 0s nor all 1s. The current code checks only `shift` bits,
> missing some overflows.

Right, "shifted + 1" here means, the shifted bits + the sign bit.

But doesn't the

if (src & sign) {
    match = mask;
} else {
    match = 0;
}

logic handle that?

If the sign is false, the shifted bits (mask) have to be 0.
If the sign bit is true, the shifted bits (mask) have to be set.

Do you have an example that would be broken?

> 
> Fixes: cbe24bfa91d2 ("target-s390: Convert SHIFT, ROTATE SINGLE")
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  target/s390x/tcg/cc_helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/tcg/cc_helper.c b/target/s390x/tcg/cc_helper.c
> index c2c96c3a3c..b6acffa3e8 100644
> --- a/target/s390x/tcg/cc_helper.c
> +++ b/target/s390x/tcg/cc_helper.c
> @@ -297,7 +297,8 @@ static uint32_t cc_calc_sla_32(uint32_t src, int shift)
>  
>  static uint32_t cc_calc_sla_64(uint64_t src, int shift)
>  {
> -    uint64_t mask = ((1ULL << shift) - 1ULL) << (64 - shift);
> +    /* Do not use (1ULL << (shift + 1)): it triggers UB when shift is 63.  */
> +    uint64_t mask = ((((1ULL << shift) - 1) << 1) + 1) << (64 - (shift + 1));
>      uint64_t sign = 1ULL << 63;
>      uint64_t match;
>      int64_t r;

This looks like some black magic :)

-- 
Thanks,

David / dhildenb