[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 3/5] target/s390x: Fix cc_calc_sla_64() missing overflows
From: |
Ilya Leoshkevich |
Subject: |
Re: [PATCH v2 3/5] target/s390x: Fix cc_calc_sla_64() missing overflows |
Date: |
Wed, 12 Jan 2022 13:51:38 +0100 |
User-agent: |
Evolution 3.42.2 (3.42.2-1.fc35) |
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.
[PATCH v2 5/5] tests/tcg/s390x: Test shift instructions, Ilya Leoshkevich, 2022/01/11
[PATCH v2 4/5] target/s390x: Fix shifting 32-bit values for more than 31 bits, Ilya Leoshkevich, 2022/01/11