Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #14339, #13511, #14420: fixes limited VM support for addr #16002

Merged
merged 5 commits into from
Nov 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions compiler/vm.nim
Original file line number Diff line number Diff line change
Expand Up @@ -746,10 +746,13 @@ proc rawExecute(c: PCtx, start: int, tos: PStackFrame): TFullReg =
regs[ra].regAddr = addr(regs[rb])
of opcAddrNode:
decodeB(rkNodeAddr)
if regs[rb].kind == rkNode:
case regs[rb].kind
of rkNode:
regs[ra].nodeAddr = addr(regs[rb].node)
of rkNodeAddr: # bug #14339
regs[ra].nodeAddr = regs[rb].nodeAddr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't that mean that when you do somePtr = addr someOtherPtr it will behave like somePtr = someOtherPtr?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe so; eg:

when true:
  type Foo = ref object
    f0: int
  proc main=
    var f = Foo(f0: 3)
    var f2 = f.addr
    var f3 = f2.addr
    var f4 = f3
    f4.f0 += 1 # like f3.f0
    f4[].f0 += 1 # like f2.f0
    f4[][].f0 += 1 # like f.f0
    f4[][][].f0 += 1 # like f[].f0
    doAssert f[].f0 == 7
  static: main()
  main()

=> CT behaves like RT

Copy link
Contributor

@Clyybber Clyybber Nov 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm struggling to come up with a concrete example, but please explain why it is correct (?) and maybe add a comment.
Your example isn't exactly convincing because f4 is ptr ptr Foo and it gets automatically dereferenced. If you change Foo to be a non-ref object it fails.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you change Foo to be a non-ref object it fails.

just filed #16020, but this is a pre-existing bug and this PR exercises a different code path, see https://gist.github.com/timotheecour/fc575c054cddba5bea32df98d1c50cdc

Hmm, I'm struggling to come up with a concrete example, but please explain why it is correct (?) and maybe add a comment.

TODO on me

Copy link
Member Author

@timotheecour timotheecour Nov 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Clyybber my understanding is there's an implicit deref that occurs in code paths that lead here due to semantic transform that generates a temporary for block return expressions (without result = expr or return expr`)
eg:

when true: # D20201125T111240
  template main() =
    type
      Node = ref object
        val: int
    proc bar(c: Node): var int =
      discard
      c.val #  before PR: Error: limited VM support for 'addr' (but nim was pointing at wrong lineinfo)
    var a = Node()
    discard a.bar()
  static: main()

which is transformed into:

proc bar(c: Node): var int =
  result = block:
    discard
    c.val

instead of:

proc bar(c: Node): var int =
  discard
  result = c.val

this seems related to #16137

This PR is still good IMO (more things now work, and hopefully nothing breaks), but I agree that the code transformation needs to be revisited as @cooldome mentioned here: #14420 (comment)

Copy link
Contributor

@Clyybber Clyybber Nov 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code transformation is correct and is not what my question is about.
I'm asking how treating ... = addr someAddress as if it were ... = someAddress is correct/sound here.

else:
stackTrace(c, tos, pc, "limited VM support for 'addr'")
stackTrace(c, tos, pc, "limited VM support for 'addr', got kind: " & $regs[rb].kind)
of opcLdDeref:
# a = b[]
let ra = instr.regA
Expand Down
25 changes: 25 additions & 0 deletions tests/macros/tmacros_various.nim
Original file line number Diff line number Diff line change
Expand Up @@ -245,3 +245,28 @@ xbenchmark:
discard inputtest
fastSHA("hey")


block: # bug #13511
type
Builder = ref object
components: seq[Component]
Component = object

proc add(builder: var Builder, component: Component) {.compileTime.} =
builder.components.add(component)

macro debugAst(arg: typed): untyped =
## just for debugging purpose.
discard arg.treeRepr
return arg

static:
var component = Component()
var builder = Builder()

template foo(): untyped =
## WAS: this doc comment causes compilation failure.
builder

debugAst:
add(foo(), component)
80 changes: 80 additions & 0 deletions tests/misc/taddr.nim
Original file line number Diff line number Diff line change
Expand Up @@ -123,3 +123,83 @@ block:
block: # pending bug #15959
when false:
proc byLent2[T](a: T): lent type(a[0]) = a[0]

proc test14420() = # bug #14420
# s/proc/template/ would hit bug #16005
block:
type Foo = object
x: float

proc fn(a: var Foo): var float =
## WAS: discard <- turn this into a comment (or a `discard`) and error disappears
# result = a.x # this works
a.x # WAS: Error: limited VM support for 'addr'

proc fn2(a: var Foo): var float =
result = a.x # this works
a.x # WAS: Error: limited VM support for 'addr'

var a = Foo()
discard fn(a)
discard fn2(a)

block:
proc byLent2[T](a: T): lent T =
runnableExamples: discard
a
proc byLent3[T](a: T): lent T =
runnableExamples: discard
result = a
var a = 10
let x3 = byLent3(a) # works
let x2 = byLent2(a) # WAS: Error: internal error: genAddr: nkStmtListExpr

block:
type MyOption[T] = object
case has: bool
of true:
value: T
of false:
discard
func some[T](val: T): MyOption[T] =
result = MyOption[T](has: true, value: val)
func get[T](opt: MyOption[T]): lent T =
doAssert opt.has
# result = opt.value # this was ok
opt.value # this had the bug
let x = some(10)
doAssert x.get() == 10

template test14339() = # bug #14339
block:
type
Node = ref object
val: int
proc bar(c: Node): var int =
var n = c # was: Error: limited VM support for 'addr'
c.val
var a = Node()
discard a.bar()
block:
type
Node = ref object
val: int
proc bar(c: Node): var int =
var n = c
doAssert n.val == n[].val
n.val
var a = Node(val: 3)
a.bar() = 5
when nimvm:
doAssert a.val == 5
else:
when not defined(js): # pending bug #16003
doAssert a.val == 5

template main =
# xxx wrap all other tests here like that so they're also tested in VM
test14420()
test14339()

static: main()
main()
92 changes: 50 additions & 42 deletions tests/stdlib/tcritbits.nim
Original file line number Diff line number Diff line change
@@ -1,61 +1,69 @@
import sequtils, critbits
import std/[sequtils,critbits]

template main =
var r: CritBitTree[void]
r.incl "abc"
r.incl "xyz"
r.incl "def"
r.incl "definition"
r.incl "prefix"
r.incl "foo"

var r: CritBitTree[void]
r.incl "abc"
r.incl "xyz"
r.incl "def"
r.incl "definition"
r.incl "prefix"
r.incl "foo"
doAssert r.contains"def"

doAssert r.contains"def"
r.excl "def"
assert r.missingOrExcl("foo") == false
assert "foo" notin toSeq(r.items)

r.excl "def"
assert r.missingOrExcl("foo") == false
assert "foo" notin toSeq(r.items)
assert r.missingOrExcl("foo") == true

assert r.missingOrExcl("foo") == true
assert toSeq(r.items) == @["abc", "definition", "prefix", "xyz"]

assert toSeq(r.items) == @["abc", "definition", "prefix", "xyz"]
assert toSeq(r.itemsWithPrefix("de")) == @["definition"]
var c = CritBitTree[int]()

assert toSeq(r.itemsWithPrefix("de")) == @["definition"]
var c = CritBitTree[int]()
c.inc("a")
assert c["a"] == 1

c.inc("a")
assert c["a"] == 1
c.inc("a", 4)
assert c["a"] == 5

c.inc("a", 4)
assert c["a"] == 5
c.inc("a", -5)
assert c["a"] == 0

c.inc("a", -5)
assert c["a"] == 0
c.inc("b", 2)
assert c["b"] == 2

c.inc("b", 2)
assert c["b"] == 2
c.inc("c", 3)
assert c["c"] == 3

c.inc("c", 3)
assert c["c"] == 3
c.inc("a", 1)
assert c["a"] == 1

c.inc("a", 1)
assert c["a"] == 1
var cf = CritBitTree[float]()

var cf = CritBitTree[float]()
cf.incl("a", 1.0)
assert cf["a"] == 1.0

cf.incl("a", 1.0)
assert cf["a"] == 1.0
cf.incl("b", 2.0)
assert cf["b"] == 2.0

cf.incl("b", 2.0)
assert cf["b"] == 2.0
cf.incl("c", 3.0)
assert cf["c"] == 3.0

cf.incl("c", 3.0)
assert cf["c"] == 3.0
assert cf.len == 3
cf.excl("c")
assert cf.len == 2

assert cf.len == 3
cf.excl("c")
assert cf.len == 2
var cb: CritBitTree[string]
cb.incl("help", "help")
for k in cb.keysWithPrefix("helpp"):
doAssert false, "there is no prefix helpp"

var cb: CritBitTree[string]
cb.incl("help", "help")
for k in cb.keysWithPrefix("helpp"):
doAssert false, "there is no prefix helpp"
block: # bug #14339
var strings: CritBitTree[int]
discard strings.containsOrIncl("foo", 3)
doAssert strings["foo"] == 3

main()
static: main()