Skip to content

Commit

Permalink
fix(vmgen): incorrect code generation for lent/var deref
Browse files Browse the repository at this point in the history
Summary
=======

Fix reading from a `var`/`lent` view of an unsigned integer (with bit-
width <= 32) producing incorrect values, when using the VM.

Details
=======

* change `genDerefView` to take the `cnkDerefView` node as input, so
  that it has access to the type of the result
* the type of the *view* (not the storage type) was previously passed
  to the `genRegLoad` call, resulting in no `NarrowU` instruction to be
  emitted for the memory access, and the value thus being sign-extended
  • Loading branch information
zerbina committed Aug 8, 2024
1 parent c46ddf2 commit 3a4f2e7
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 11 deletions.
19 changes: 9 additions & 10 deletions compiler/vm/vmgen.nim
Original file line number Diff line number Diff line change
Expand Up @@ -2466,18 +2466,17 @@ proc genAsgnToLocal(c: var TCtx, le, ri: CgNode) =
gen(c, ri, dest)

proc genDerefView(c: var TCtx, n: CgNode, dest: var TDest; load = true) =
## Generates and emits the code for a view dereference, where `n` is the
## expression that evaluates to a view. `load` indicates whether the
## *handle* of the underlying location or the value stored in it should be
## put into `dest`.
## Generates and emits the code for a view dereference. `load` indicates
## whether the *handle* of the underlying location or the value stored in
## it should be put into `dest`.
let
isPtr = isPtrView(n)
needsLoad = load and fitsRegister(n.typ.skipTypes(abstractVar))
isPtr = isPtrView(n.operand)
needsLoad = load and fitsRegister(n.typ)

if isPtr or needsLoad:
# we need to process the operand further, and thus need a temporary
prepare(c, dest, n.typ) # XXX: the passed type is incorrect
let tmp = c.genx(n)
let tmp = c.genx(n.operand)
var src = tmp

if isPtr:
Expand All @@ -2493,7 +2492,7 @@ proc genDerefView(c: var TCtx, n: CgNode, dest: var TDest; load = true) =
c.freeTemp(tmp)
else:
# no processing required; load the handle directly into `dest`
c.gen(n, dest)
c.gen(n.operand, dest)

proc genAsgn(c: var TCtx; le, ri: CgNode; requiresCopy: bool) =
case le.kind
Expand Down Expand Up @@ -2530,7 +2529,7 @@ proc genAsgn(c: var TCtx; le, ri: CgNode; requiresCopy: bool) =
c.freeTemp(dest)
else:
var dest = noDest
genDerefView(c, le.operand, dest, load=false)
genDerefView(c, le, dest, load=false)
putIntoLoc(c, ri, dest, 0, opcWrLoc, opcWrLoc)
c.freeTemp(dest)
of cnkDeref:
Expand Down Expand Up @@ -3070,7 +3069,7 @@ proc gen(c: var TCtx; n: CgNode; dest: var TDest) =
of cnkDerefView:
assert isLocView(n.operand.typ)
# a view indirection
genDerefView(c, n.operand, dest)
genDerefView(c, n, dest)
of cnkHiddenAddr:
assert isLocView(n.typ)
# load the source operand as a handle
Expand Down
20 changes: 19 additions & 1 deletion tests/vm/tvmgen_regressions.nim
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,22 @@ block wrong_getast:
let x = [getAst(m())]
doAssert x[0].intVal == 1

m2()
m2()

block wrong_uint_view_deref:
# reading from a lent/var indirection produced the wrong value for non-full-
# width integer types, when the highest bit was set
proc f_lent[T](x: var T): lent T = x
proc f_var[T](x: var T): var T = x

var
a = high(uint8)
b = high(uint16)
c = high(uint32)

doAssert f_lent(a) == high(uint8)
doAssert f_lent(b) == high(uint16)
doAssert f_lent(c) == high(uint32)
doAssert f_var(a) == high(uint8)
doAssert f_var(b) == high(uint16)
doAssert f_var(c) == high(uint32)

0 comments on commit 3a4f2e7

Please sign in to comment.