From 1c9e91c405452242b78f1061672fa769c2f57ee2 Mon Sep 17 00:00:00 2001
From: zerbina <100542850+zerbina@users.noreply.github.com>
Date: Thu, 8 Aug 2024 21:32:56 +0200
Subject: [PATCH] vmgen: fix incorrect code generation for `lent`/`var` deref
 (#1409)

## Summary

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

Fixes https://github.com/nim-works/nimskull/issues/1407.

## 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
---
 compiler/vm/vmgen.nim           | 19 +++++++++----------
 tests/vm/tvmgen_regressions.nim | 20 +++++++++++++++++++-
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/compiler/vm/vmgen.nim b/compiler/vm/vmgen.nim
index c530a4a7488..acada37c8ad 100644
--- a/compiler/vm/vmgen.nim
+++ b/compiler/vm/vmgen.nim
@@ -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:
@@ -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
@@ -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:
@@ -3083,7 +3082,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
diff --git a/tests/vm/tvmgen_regressions.nim b/tests/vm/tvmgen_regressions.nim
index 1af09d44ff2..e8ad9553487 100644
--- a/tests/vm/tvmgen_regressions.nim
+++ b/tests/vm/tvmgen_regressions.nim
@@ -30,4 +30,22 @@ block wrong_getast:
     let x = [getAst(m())]
     doAssert x[0].intVal == 1
 
-  m2()
\ No newline at end of file
+  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)