From c95d1fbb846bac63d36e741441ea8529e09142f6 Mon Sep 17 00:00:00 2001 From: Erik Carstensen Date: Mon, 8 Jul 2024 15:50:37 +0200 Subject: [PATCH] Generalize WNEGCONSTCOMP to warn for all constants outside a type's range --- RELEASENOTES-1.4.md | 5 +- py/dml/ctree.py | 22 ++++++-- py/dml/ctree_test.py | 4 +- py/dml/messages.py | 16 +++--- py/dml/types.py | 11 ++++ test/1.4/errors/T_WNEGCONSTCOMP.dml | 36 ------------ test/1.4/errors/T_WTYPELIMITS.dml | 88 +++++++++++++++++++++++++++++ 7 files changed, 131 insertions(+), 51 deletions(-) delete mode 100644 test/1.4/errors/T_WNEGCONSTCOMP.dml create mode 100644 test/1.4/errors/T_WTYPELIMITS.dml diff --git a/RELEASENOTES-1.4.md b/RELEASENOTES-1.4.md index 64dfd534..ab1201a2 100644 --- a/RELEASENOTES-1.4.md +++ b/RELEASENOTES-1.4.md @@ -312,4 +312,7 @@ - `release 6 6313` - `release 7 7026` - `note 6` The warning message for comparing a value of unsigned type to a negative constant has been improved to also warn for unsigned types shorter than 64 bits. -- `release 6 6315` \ No newline at end of file +- `release 6 6315` +- `note 6` Added warnings for comparing an integer to constant values + outside the integer's range. E.g., given a variable `x` of type `uint8`, + warnings will now be reported on expressions like `x >= 256` or `x == 3.14`. diff --git a/py/dml/ctree.py b/py/dml/ctree.py index 31bddcfe..06941e1b 100644 --- a/py/dml/ctree.py +++ b/py/dml/ctree.py @@ -1392,10 +1392,18 @@ def make(cls, site, lh, rh): and lh.constant and rh.constant): return mkBoolConstant(site, cls.eval_const(lh.value, rh.value)) if lhtype.is_int: + if rh.constant and rhtype.is_arith and ( + cls.eval_const(lhtype.min(), rh.value) + == cls.eval_const(lhtype.max(), rh.value)): + report(WTYPELIMITS(site, rh, lhtype)) lh_maybe_negative = lhtype.signed lh = as_int(lh) lhtype = realtype(lh.ctype()) if rhtype.is_int: + if lh.constant and lhtype.is_arith and ( + cls.eval_const(lh.value, rhtype.min()) + == cls.eval_const(lh.value, rhtype.max())): + report(WTYPELIMITS(site, lh, rhtype)) rh_maybe_negative = rhtype.signed rh = as_int(rh) rhtype = realtype(rh.ctype()) @@ -1404,9 +1412,7 @@ def make(cls, site, lh, rh): and lh_maybe_negative != rh_maybe_negative): (signed_expr, unsigned_expr) = ((lh, rh) if lh_maybe_negative else (rh, lh)) - if signed_expr.constant and signed_expr.value < 0: - report(WNEGCONSTCOMP(site, signed_expr, - unsigned_expr.ctype())) + # we must convert (uint64)x < (int64)y to DML_lt(x, y), because # C:'s < would do an unsigned comparison. No need to do this if y # is a small constant, though. @@ -1602,10 +1608,18 @@ def make(cls, site, lh, rh): AddressOfMethod)) return mkBoolConstant(site, False) if lhtype.is_int: + if rh.constant and rhtype.is_arith and ( + not (lhtype.min() <= rh.value <= lhtype.max()) + or (rhtype.is_float and rh.value % 1)): + report(WTYPELIMITS(site, rh, lhtype)) lh_maybe_negative = lhtype.signed lh = as_int(lh) lhtype = realtype(lh.ctype()) if rhtype.is_int: + if lh.constant and lhtype.is_arith and ( + not (rhtype.min() <= lh.value <= rhtype.max()) + or (lhtype.is_float and lh.value % 1)): + report(WTYPELIMITS(site, lh, rhtype)) rh_maybe_negative = rhtype.signed rh = as_int(rh) rhtype = realtype(rh.ctype()) @@ -1618,8 +1632,6 @@ def make(cls, site, lh, rh): # unsigned to a constant literal. (signed_expr, unsigned_expr) = ((lh, rh) if lh_maybe_negative else (rh, lh)) - if signed_expr.constant and signed_expr.value < 0: - report(WNEGCONSTCOMP(site, signed_expr, unsigned_expr.ctype())) if not (signed_expr.constant and 0 <= signed_expr.value < 1 << 63): return mkApply( site, mkLit( diff --git a/py/dml/ctree_test.py b/py/dml/ctree_test.py index ec8e2750..7ffa14ad 100644 --- a/py/dml/ctree_test.py +++ b/py/dml/ctree_test.py @@ -1186,9 +1186,9 @@ def compare_numeric(self, lh, rh, op, result): rh_var = variable('rh', rh.ctype()) var_cond = op(site, lh_var, rh_var) # compare constant and variable - logging.ignore_warning('WNEGCONSTCOMP') + logging.ignore_warning('WTYPELIMITS') mix_cond = op(site, lh, rh_var) - logging.enable_warning('WNEGCONSTCOMP') + logging.enable_warning('WTYPELIMITS') self.assertIsInstance(var_cond.ctype(), types.TBool) self.assertIsInstance(mix_cond.ctype(), types.TBool) return ['%s = %s;' % (lh.ctype().declaration('lh'), lh.read()), diff --git a/py/dml/messages.py b/py/dml/messages.py index 8d7b9cc2..3278d204 100644 --- a/py/dml/messages.py +++ b/py/dml/messages.py @@ -2110,16 +2110,18 @@ def log(self): self.other_site, "original non-throwing declaration") -class WNEGCONSTCOMP(DMLWarning): - """DML uses a special method when comparing an unsigned and signed integer, - meaning that comparing a negative constant to an unsigned integer always - has the same result, which is usually not the intended behaviour.""" +class WTYPELIMITS(DMLWarning): + """When comparing an integer with a constant outside the range of the + integer's type, the comparison always has the same result. One + common example is that a value of type `uint64` always is + considered to be different from -1; in this case, the constant + can be rewritten as `cast(-1, uint64)`. + """ def __init__(self, site, expr, ty): - DMLWarning.__init__(self, site) + super().__init__(site) self.expr = expr self.ty = ty - fmt = ("Comparing negative constant to unsigned integer has a constant " - + "result") + fmt = 'Comparison with constant outside the range of data type' def log(self): DMLError.log(self) self.print_site_message( diff --git a/py/dml/types.py b/py/dml/types.py index a440417d..5aa4bd63 100644 --- a/py/dml/types.py +++ b/py/dml/types.py @@ -511,6 +511,17 @@ def __init__(self, bits, signed, members=None, const=False): is_arith = True is_endian = False + def min(self): + if self.signed: + return -(1 << (self.bits - 1)) + else: + return 0 + def max(self): + if self.signed: + return (1 << (self.bits - 1)) - 1 + else: + return (1 << self.bits) - 1 + @property def is_bitfields(self): return self.members is not None diff --git a/test/1.4/errors/T_WNEGCONSTCOMP.dml b/test/1.4/errors/T_WNEGCONSTCOMP.dml deleted file mode 100644 index fcdd4f24..00000000 --- a/test/1.4/errors/T_WNEGCONSTCOMP.dml +++ /dev/null @@ -1,36 +0,0 @@ -/* - © 2021 Intel Corporation - SPDX-License-Identifier: MPL-2.0 -*/ -dml 1.4; - -device test; - -method init() { - local uint64 a = -1; - /// WARNING WNEGCONSTCOMP - assert a != -1; - /// WARNING WNEGCONSTCOMP - assert a > -1; - /// WARNING WNEGCONSTCOMP - assert !(a < -1); - /// WARNING WNEGCONSTCOMP - assert a >= -1; - /// WARNING WNEGCONSTCOMP - assert !(a <= -1); - local uint8 a8 = -1; - /// WARNING WNEGCONSTCOMP - assert a8 != -1; - /// WARNING WNEGCONSTCOMP - assert a8 > -1; - - // Sanity - local int64 b = -1; - // No warning - assert b == -1; - // No warning - assert !(b < -1); - - // Test workaround - assert a == cast(-1, uint64); -} diff --git a/test/1.4/errors/T_WTYPELIMITS.dml b/test/1.4/errors/T_WTYPELIMITS.dml new file mode 100644 index 00000000..76c81fdd --- /dev/null +++ b/test/1.4/errors/T_WTYPELIMITS.dml @@ -0,0 +1,88 @@ +/* + © 2021 Intel Corporation + SPDX-License-Identifier: MPL-2.0 +*/ +dml 1.4; + +device test; + +method init() { + local uint64 a = -1; + // Test all operands + /// WARNING WTYPELIMITS + assert a != -1; + /// WARNING WTYPELIMITS + assert -1 != a; + /// WARNING WTYPELIMITS + assert !(-1 == a); + /// WARNING WTYPELIMITS + assert a > -1; + /// WARNING WTYPELIMITS + assert !(a < -1); + /// WARNING WTYPELIMITS + assert a >= -1; + /// WARNING WTYPELIMITS + assert !(a <= -1); + + // Upper and lower bounds are tight, regardless if the constant is on RH or + // LH side + local uint8 a8 = -1; + /// WARNING WTYPELIMITS + assert a8 != -1; + /// WARNING WTYPELIMITS + assert -1 != a8; + // no warning + assert a8 != 0; + assert 0 != a8; + /// WARNING WTYPELIMITS + assert a8 != 256; + /// WARNING WTYPELIMITS + assert 256 != a8; + // no warning + assert a8 == 255; + assert 255 == a8; + /// WARNING WTYPELIMITS + assert a8 >= 0; + /// WARNING WTYPELIMITS + assert 0 <= a8; + /// no warning + assert a8 > 0; + assert 0 < a8; + /// WARNING WTYPELIMITS + assert !(a8 > 255); + /// WARNING WTYPELIMITS + assert !(255 < a8); + /// no warning + assert a8 >= 255; + assert 255 <= a8; + + // Limits for signed integers are tight + local int8 i8 = 128; + /// WARNING WTYPELIMITS + assert i8 != 128; + // no warning + assert i8 != 127; + /// WARNING WTYPELIMITS + assert i8 != -129; + // no warning + assert i8 == -128; + + // Non-integer float constants also give warnings + /// WARNING WTYPELIMITS + assert i8 != 0.1; + /// WARNING WTYPELIMITS + assert 0.1 != i8; + // no warning + assert i8 != 0.0; + assert 0.0 != i8; + + // Sanity + local int64 b = -1; + // No warning + assert b == -1; + // No warning + assert !(b < -1); + + // Test workaround + assert a == cast(-1, uint64); +}