| Message ID | 20220112043948.224405-4-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/s390x: Fix shift instructions | expand |
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 :)
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.
>> 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. Ah, right, because of the - 1ULL ... [...] >> 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. > I wonder if all we want is const uint64_t sign = 1ULL << 63; uint64_t mask = (-1ULL << (63 - shift)) & ~sign; For shift = * 0: 0000000...0b * 1: 0100000...0b * 2: 0110000...0b * 63: 0111111...1b Seems to survive your tests.
On Wed, 2022-01-12 at 16:45 +0100, David Hildenbrand wrote: > > > 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. > > Ah, right, because of the - 1ULL ... > > [...] > > > > 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. > > > > I wonder if all we want is > > const uint64_t sign = 1ULL << 63; > uint64_t mask = (-1ULL << (63 - shift)) & ~sign; > > For shift = > * 0: 0000000...0b > * 1: 0100000...0b > * 2: 0110000...0b > * 63: 0111111...1b > > Seems to survive your tests. -1ULL does indeed help a lot to simplify this. I don't think we even need to mask out the sign, since it should be the same as the other bits anyway. So just uint64_t mask = -1ULL << (63 - shift); appears to be enough.
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;
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. 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(-)