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

[Ilya Leoshkevich <iii@linux.ibm.com>]

On Wed, 2022-01-12 at 09:59 +0100, David Hildenbrand wrote:
> 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.

IIUC this logic handles sign bit + "shift - 1" bits. So if the last
shifted bit is different, the overflow is not detected.

> 
> Do you have an example that would be broken?

sla-2 test covers this. I added a similar one for SLAG in v3 and
it fails as well.

> > 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 :)

Yeah, I felt this way too, but didn't come up with anything better and
just left a comment warning not to simplify.