From 22e5b5e3cc8e659fbde9af55b271ff416fbe0f9c Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Sun, 30 Jul 2023 15:47:26 +0000 Subject: [PATCH 1/7] tests: add a test for the expected behaviour --- .../casestmt/tcase_with_uint_values.nim | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 tests/lang_stmts/casestmt/tcase_with_uint_values.nim diff --git a/tests/lang_stmts/casestmt/tcase_with_uint_values.nim b/tests/lang_stmts/casestmt/tcase_with_uint_values.nim new file mode 100644 index 00000000000..e216a190854 --- /dev/null +++ b/tests/lang_stmts/casestmt/tcase_with_uint_values.nim @@ -0,0 +1,37 @@ +discard """ + targets: "c js vm" + description: ''' + Ensures that case statements work with uint operands and of-branch values + not representable with the same-sized signed integer type + ''' +""" + +proc test[T: uint64|uint; S]() = + const Border = T(high(S)) # the highest possible signed integer value + + proc inRange(x: T): int {.noinline.} = # don't fold at compile-time + type Range = range[Border-5..Border+5] + # work-around so that the set construction expression compiles + + case x + of (high(T)-3)..high(T): 0 # range that's fully beyond the border + of (Border-4)..(Border+4): 1 # range that crosses the border + of {Range(Border-5), Border+5}: 2 # set with values in and beyond the border + else: 3 + + doAssert inRange(0) == 3 + doAssert inRange(high(T)) == 0 + when defined(vm): + # knownIssue: ranges in of-branches that cross the signed/unsigned border + # don't work correctly in the VM, due to their values being + # treated as signed integers + doAssert inRange(Border) == 3 + doAssert inRange(Border+1) == 3 + else: + doAssert inRange(Border) == 1 + doAssert inRange(Border+1) == 1 + doAssert inRange(Border-5) == 2 + doAssert inRange(Border+5) == 2 + +test[uint64, int64]() +test[uint, int]() \ No newline at end of file From b038e5d2a6f4a72ea95d9125c247ce5d74bd1936 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Sun, 30 Jul 2023 15:47:27 +0000 Subject: [PATCH 2/7] semexprs: allow `uint64` as `case` operand The `uint64` is considered an ordinal type by `isOrdinal`, and not supporting it here seems like an oversight. --- compiler/sem/semstmts.nim | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/sem/semstmts.nim b/compiler/sem/semstmts.nim index 5b365811b81..05c5e0234fe 100644 --- a/compiler/sem/semstmts.nim +++ b/compiler/sem/semstmts.nim @@ -1660,7 +1660,7 @@ proc semCase(c: PContext, n: PNode; flags: TExprFlags): PNode = var typ = commonTypeBegin var hasElse = false let caseTyp = skipTypes(n[0].typ, abstractVar-{tyTypeDesc}) - const shouldChckCovered = {tyInt..tyInt64, tyChar, tyEnum, tyUInt..tyUInt32, tyBool} + const shouldChckCovered = {tyInt..tyInt64, tyChar, tyEnum, tyUInt..tyUInt64, tyBool} case caseTyp.kind of shouldChckCovered: chckCovered = true From 5c6c42c668566b78a1a3c303bc45bfaadf07ba00 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Sun, 30 Jul 2023 15:47:27 +0000 Subject: [PATCH 3/7] nimsets: refactor and fix `toTreeSet` Keep the `firstOrd` result as an `Int128`, fixing the compiler crashing with integer overflow defects when the `first` + element value is outside the `int64` range. In addition, use `let` where it makes sense, and replace appending to the `nkRange` node with setting the elements directly. --- compiler/ast/nimsets.nim | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/compiler/ast/nimsets.nim b/compiler/ast/nimsets.nim index f3a9cb187a4..82de6e076b6 100644 --- a/compiler/ast/nimsets.nim +++ b/compiler/ast/nimsets.nim @@ -84,12 +84,12 @@ proc toBitSet*(conf: ConfigRef; s: PNode): TBitSet = inclTreeSet(result, conf, s) proc toTreeSet*(conf: ConfigRef; s: TBitSetView, settype: PType, info: TLineInfo): PNode = + let + elemType = settype[0] + first = firstOrd(conf, elemType) var - a, b, e, first: BiggestInt # a, b are interval borders - elemType: PType - n: PNode - elemType = settype[0] - first = firstOrd(conf, elemType).toInt64 + a, b, e: BiggestInt # a, b are interval borders + result = newNodeI(nkCurly, info) result.typ = settype result.info = info @@ -107,12 +107,12 @@ proc toTreeSet*(conf: ConfigRef; s: TBitSetView, settype: PType, info: TLineInfo if a == b: result.add aa else: - n = newNodeI(nkRange, info) + var n = newNodeI(nkRange, info, 2) n.typ = elemType - n.add aa + n[0] = aa let bb = newIntTypeNode(b + first, elemType) bb.info = info - n.add bb + n[1] = bb result.add n e = b inc(e) From c4fdfb55742f12995b47edeb21d50943fd665d69 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Sun, 30 Jul 2023 15:47:27 +0000 Subject: [PATCH 4/7] ccgexprs: separate integer-literal code-gen Move the logic into a standalone procedure, in preparation for the following fix. In addition, integer-literal nodes reaching the C code generator are now required to be typed. --- compiler/backend/ccgexprs.nim | 33 +++++++++++++-------------------- compiler/backend/cgen.nim | 1 + 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/compiler/backend/ccgexprs.nim b/compiler/backend/ccgexprs.nim index bafeca8012b..0ee7c651f72 100644 --- a/compiler/backend/ccgexprs.nim +++ b/compiler/backend/ccgexprs.nim @@ -39,29 +39,22 @@ proc intLiteral(i: BiggestInt): Rope = proc intLiteral(i: Int128): Rope = intLiteral(toInt64(i)) +proc intLiteral(p: BProc, i: Int128, ty: PType): Rope = + assert ty != nil + case skipTypes(ty, abstractVarRange).kind + of tyChar: intLiteral(i) + of tyBool: + if i != Zero: "NIM_TRUE" + else: "NIM_FALSE" + of tyInt64: int64Literal(toInt64(i)) + of tyUInt64: uint64Literal(toUInt64(i)) + else: + "(($1) $2)" % [getTypeDesc(p.module, ty), intLiteral(i)] + proc genLiteral(p: BProc, n: PNode, ty: PType): Rope = case n.kind of nkCharLit..nkUInt64Lit: - var k: TTypeKind - if ty != nil: - k = skipTypes(ty, abstractVarRange).kind - else: - case n.kind - of nkCharLit: k = tyChar - of nkUInt64Lit: k = tyUInt64 - of nkInt64Lit: k = tyInt64 - else: k = tyNil # don't go into the case variant that uses 'ty' - case k - of tyChar, tyNil: - result = intLiteral(n.intVal) - of tyBool: - if n.intVal != 0: result = ~"NIM_TRUE" - else: result = ~"NIM_FALSE" - of tyInt64: result = int64Literal(n.intVal) - of tyUInt64: result = uint64Literal(uint64(n.intVal)) - else: - result = "(($1) $2)" % [getTypeDesc(p.module, - ty), intLiteral(n.intVal)] + result = intLiteral(p, getInt(n), ty) of nkNilLit: let k = if ty == nil: tyPointer else: skipTypes(ty, abstractVarRange).kind if k == tyProc and skipTypes(ty, abstractVarRange).callConv == ccClosure: diff --git a/compiler/backend/cgen.nim b/compiler/backend/cgen.nim index 19280d62ed9..3b2215d9131 100644 --- a/compiler/backend/cgen.nim +++ b/compiler/backend/cgen.nim @@ -564,6 +564,7 @@ proc genStmts*(p: BProc, t: PNode) proc expr(p: BProc, n: PNode, d: var TLoc) proc putLocIntoDest(p: BProc, d: var TLoc, s: TLoc) proc intLiteral(i: BiggestInt): Rope +proc intLiteral(p: BProc, i: Int128, ty: PType): Rope proc genLiteral(p: BProc, n: PNode): Rope proc raiseExit(p: BProc) From a5a69fc61ec26563bdb37cfd08ed1627c8293068 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Sun, 30 Jul 2023 15:47:28 +0000 Subject: [PATCH 5/7] ccgstmts: support ranges crossing the `int64` upper border For generating code for `of`-branches, don't operate with the signed `intVal` values, but turn them into `Int128` values first. When using C compilers that don't support case ranges, this fixes the compiler crashing with overflow defects. --- compiler/backend/ccgstmts.nim | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/compiler/backend/ccgstmts.nim b/compiler/backend/ccgstmts.nim index a4b087b3897..4f014ec56b5 100644 --- a/compiler/backend/ccgstmts.nim +++ b/compiler/backend/ccgstmts.nim @@ -542,7 +542,7 @@ proc branchHasTooBigRange(b: PNode): bool = for it in b: # last son is block if (it.kind == nkRange) and - it[1].intVal - it[0].intVal > RangeExpandLimit: + getInt(it[1]) - getInt(it[0]) > RangeExpandLimit: return true proc ifSwitchSplitPoint(p: BProc, n: PNode): int = @@ -563,10 +563,13 @@ proc genCaseRange(p: BProc, branch: PNode) = genLiteral(p, branch[j][0]), genLiteral(p, branch[j][1])]) else: - var v = copyNode(branch[j][0]) - while v.intVal <= branch[j][1].intVal: - lineF(p, cpsStmts, "case $1:$n", [genLiteral(p, v)]) - inc(v.intVal) + let + typ = branch[j][0].typ + upper = getInt(branch[j][1]) + var v = getInt(branch[j][0]) + while v <= upper: + lineF(p, cpsStmts, "case $1:$n", [intLiteral(p, v, typ)]) + inc(v) else: lineF(p, cpsStmts, "case $1:$n", [genLiteral(p, branch[j])]) From 6f248358973c1f380189eec1e61fe68150a11a26 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Sun, 30 Jul 2023 15:47:28 +0000 Subject: [PATCH 6/7] jsgen: support ranges crossing the `int64` upper border * move code-generation for integer literals into a standalone procedure * same as with the C code generator, use `Int128` values when iterating over the range --- compiler/backend/jsgen.nim | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/compiler/backend/jsgen.nim b/compiler/backend/jsgen.nim index d9524564c74..598b811d5a5 100644 --- a/compiler/backend/jsgen.nim +++ b/compiler/backend/jsgen.nim @@ -825,10 +825,16 @@ proc genRaiseStmt(p: PProc, n: PNode) = useMagic(p, "reraiseException") line(p, "reraiseException();\L") +func intLiteral(v: Int128, typ: PType): string = + if typ.kind == tyBool: + if v == Zero: "false" + else: "true" + else: $v + proc genCaseJS(p: PProc, n: PNode) = var cond: TCompRes - totalRange = 0 + totalRange = Zero genLineDir(p, n) gen(p, n[0], cond) let stringSwitch = skipTypes(n[0].typ, abstractVar).kind == tyString @@ -845,15 +851,15 @@ proc genCaseJS(p: PProc, n: PNode) = for j in 0.. 65535: localReport(p.config, n.info, BackendReport(kind: rbackJsTooCaseTooLarge)) - while v.intVal <= e[1].intVal: - gen(p, v, cond) - lineF(p, "case $1:$n", [cond.rdLoc]) - inc(v.intVal) + while v <= upper: + lineF(p, "case $1:$n", [intLiteral(v, e[0].typ)]) + inc v else: if stringSwitch: case e.kind @@ -2409,10 +2415,7 @@ proc gen(p: PProc, n: PNode, r: var TCompRes) = of nkSym: genSym(p, n, r) of nkCharLit..nkUInt64Lit: - if n.typ.kind == tyBool: - r.res = if n.intVal == 0: rope"false" else: rope"true" - else: - r.res = rope(n.intVal) + r.res = intLiteral(getInt(n), n.typ) r.kind = resExpr of nkNilLit: if isEmptyType(n.typ): From f87ca72a44e47c6e0ac54c1e253ec951b623a673 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Sun, 30 Jul 2023 15:47:28 +0000 Subject: [PATCH 7/7] tests: disable the test for JS Compiling it doesn't crash the compiler anymore, at least. --- tests/lang_stmts/casestmt/tcase_with_uint_values.nim | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/lang_stmts/casestmt/tcase_with_uint_values.nim b/tests/lang_stmts/casestmt/tcase_with_uint_values.nim index e216a190854..23dd77e133c 100644 --- a/tests/lang_stmts/casestmt/tcase_with_uint_values.nim +++ b/tests/lang_stmts/casestmt/tcase_with_uint_values.nim @@ -1,11 +1,14 @@ discard """ - targets: "c js vm" + targets: "c !js vm" description: ''' Ensures that case statements work with uint operands and of-branch values not representable with the same-sized signed integer type ''' """ +# knownIssue: compiles correctly for JS, but doesn't work at run-time because +# of the improper large integer support + proc test[T: uint64|uint; S]() = const Border = T(high(S)) # the highest possible signed integer value