-
Notifications
You must be signed in to change notification settings - Fork 39
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
vmgen: fix two overflow bugs with
set
s (#801)
## Summary Fix either the compiler crashing or the VM failing at run-time with an over- or underflow defect for `incl`, `contains`, and construction operations for `set`s where the element range crossed the `int64` upper boundary. The fixed bugs only affected VM bytecode generation and code running in the VM. ## Details There were two related problems: - the relevant `vmgen` logic performed all math with signed integer values - preparing (i.e., offsetting) a run-time value for a `set` operation always happened via `SubInt` (subtract signed integer), ignoring the signed-ness of the operands When the lower bound of the elements' range was beyond the `int64` upper bound, this didn't cause problems: reinterpreting the integer bits always yields negative values there. For example, for a `set[Low..High]` where `Low == high(uint64)-3` and `High = high(uint)`, subtracting the lower inclusive-bound (-4) from the upper inclusive-bound (-1) both didn't cause and overflow and also resulted in the correct value (3). However, when `Low == uint64 high(int64)` and `High == Low + 4`, this doesn't work. When the operand to an operation involving such sets was a literal integer outside the `int64` range, the compiler crashed - when it was a run-time value, the VM erroneously reported an overflow. The logic for generating the code for loading set elements now uses `Int128` to get around over- and underflow problems, and the `Subu` operation is emitted for offsetting unsigned run-time values. In addition, generating the bytecode for loading literal unsigned integers now happens via `loadInt`, meaning that the values are now loaded via `LdImmInt` if they're less than 2^23. Previously, literal unsigned integers were always loaded via the slightly less efficient `LdConst` operation.
- Loading branch information
Showing
2 changed files
with
91 additions
and
24 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
discard """ | ||
targets: "c vm !js" | ||
description: ''' | ||
Test the `incl`, `contains`, and set construction operation for sets where | ||
the element range crosses the signed 64-bit integer upper bound | ||
''' | ||
""" | ||
|
||
# knownIssue: sets with elements beyond 2^53-1 don't work the JavaScript | ||
# backend | ||
|
||
const | ||
Low = uint64(high(int64)) | ||
High = Low + 3 | ||
|
||
type | ||
Range = range[Low .. High] | ||
SetType = set[Range] | ||
|
||
const | ||
a = Range(Low + 0) | ||
b = Range(Low + 1) | ||
c = Range(Low + 2) | ||
d = Range(Low + 3) | ||
|
||
var | ||
s: SetType | ||
val: Range | ||
|
||
# test `incl` and `contains` and set construction with a constant value that | ||
# is still in the int64 range | ||
s.incl a | ||
doAssert s.contains(a) | ||
doAssert s == SetType({ a }) | ||
doAssert card(s) == 1 | ||
|
||
# now test with a constant value that's outside of the int64 range | ||
s.incl d | ||
doAssert d in s | ||
doAssert s == SetType({ a, d }) | ||
doAssert card(s) == 2 | ||
|
||
s = {} | ||
|
||
# test with a run-time value that's in the int64 range | ||
val = a | ||
s.incl val | ||
doAssert s.contains(a) | ||
doAssert s.contains(val) | ||
doAssert s == SetType({ a }) # check that the constant set agrees | ||
doAssert s == SetType({ val }) | ||
doAssert card(s) == 1 | ||
|
||
# now test with a run-time value that's outside int64 range | ||
val = d | ||
s.incl val | ||
doAssert s.contains(d) | ||
doAssert s.contains(val) | ||
doAssert s == SetType({ a, d }) | ||
doAssert s == SetType({ a, val }) |