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

hashSetSeq[^1].incl segfaults in VM #10981

Closed
ghost opened this issue Apr 8, 2019 · 11 comments
Closed

hashSetSeq[^1].incl segfaults in VM #10981

ghost opened this issue Apr 8, 2019 · 11 comments
Assignees
Labels
Compiler Crash VM see also `const` label

Comments

@ghost
Copy link

ghost commented Apr 8, 2019

Example

import sets
static:
   var someSets = @[initHashSet[int]()]
   someSets[^1].incl(0) # <-- segfaults
   echo someSets

Current Output

SIGSEGV: Illegal storage access. (Attempt to read from nil?)

Expected Output

@[{0}]

Possible Solution

Workarounds: someSets[someSets.len - 1]
This just delays (nondeterministically) the segfault to later usage of VM.

Additional Information

$ nim -v
Nim Compiler Version 0.19.9 [Linux: amd64]
Compiled at 2019-04-07
Copyright (c) 2006-2019 by Andreas Rumpf

git hash: f6ad071a46a2bec57db453343d8d8b75d3d16ac2
active boot switches: -d:release
@krux02 krux02 added VM see also `const` label Compiler Crash labels Apr 8, 2019
@krux02 krux02 self-assigned this Apr 8, 2019
@nc-x
Copy link
Contributor

nc-x commented Apr 11, 2019

Gives the correct output with debug compiler (koch temp) and no crashes. ¯\_(ツ)_/¯

@narimiran
Copy link
Member

Gives the correct output with debug compiler (koch temp) and no crashes.

Can you re-check that?
Because on my end it crashes regardless if i use the latest devel, temp, Nim v0.19.4 or Nim v0.19.0

@nc-x
Copy link
Contributor

nc-x commented Apr 16, 2019

@narimiran

λ nim --version
Nim Compiler Version 0.19.9 [Windows: amd64]
Compiled at 2019-04-16
Copyright (c) 2006-2019 by Andreas Rumpf

git hash: ce024a73bf9050a18b0cb7c6a985127c86ba14c7
active boot switches: -d:release

λ nim c t.nim
...removed hints...
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

λ nim_temp c t.nim
...removed hints...
@[{0}]
...removed hints...

@narimiran
Copy link
Member

@nc-x Hm, can you show the output of nim_temp -v? This might give us a clue when it worked.

@nc-x
Copy link
Contributor

nc-x commented Apr 17, 2019

@narimiran
nim_temp (koch temp) --version => ce024a7
With koch temp I don't get a crash in any of the commits that I have tried.

@ghost
Copy link
Author

ghost commented Apr 17, 2019

  1. The bug seemed to be introduced sometime between 17.2 and 18.0. On my computer (and repl.it which uses old nim) it works on 17.2 and fails on 18.0. With relevant api changes, initHashSet -> initSet
  2. The issue goes away when I remove everything not necessary for the example from the sets module. The more I removed the more reliably it would work. Where current contents of set module always crashes and all except relevant code path always works.

@loloicci
Copy link
Contributor

Similar bug happened in HashSet of Table.

Example
loloicci/nimly#24 (comment)

Additional Information
version
Nim: fabc2a7

@GULPF
Copy link
Member

GULPF commented May 5, 2019

Cannot reproduce on devel

@nc-x
Copy link
Contributor

nc-x commented May 5, 2019

IDK how nim secret works but it still crashes there.

@pgkos
Copy link
Contributor

pgkos commented Aug 10, 2019

After a bit of debugging, I have found this problematic code:

proc `[]`*[T](s: var openArray[T]; i: BackwardsIndex): var T {.inline.} =

279     LdDeref  r4, r1, r0                   #lib/system.nim(4034, 15)
280     LdDeref  r5, r1, r0                   #lib/system.nim(4034, 18)
281     LenSeq   r6, r5, r128                 #lib/system.nim(4034, 19)
282     Conv     r7, r2, int, BackwardsIndex  #lib/system.nim(4034, 29)
285     SubInt   r8, r6, r7                   #lib/system.nim(4034, 24)
286     LdArr    r5, r4, r8                   #lib/system.nim(4034, 14)
287     AddrNode r3, r5, r0                   #lib/system.nim(4034, 14)
288     AsgnRef  r0, r3, r1                   #lib/system.nim(4033, 1)
289     Ret      r0, r0, r0                   #lib/system.nim(4034, 3)

The last four instructions are:

286     LdArr    r5, r4, r8  #tos.r5.node = source node
287     AddrNode r3, r5, r0  #tos.r3.nodeAddr = addr(tos.r5.node)
288     AsgnRef  r0, r3, r1  #tos.r0.nodeAddr = tos.r3.nodeAddr
289     Ret      r0, r0, r0  #return tos.r0

The proc passes the return value in the register 0, which after this point has a dangling nodeAddr value pointing to the node field of register 5 of the local stack frame.

@pgkos
Copy link
Contributor

pgkos commented Aug 11, 2019

Minimal example:

type Bar = ref object
  baz: int

proc foo(b: var Bar, i: int): var int =
  if i == 0:
    return b.baz
  else:
    return foo(b, i - 1)

proc quux(n: int) =
  if n > 0:
    quux(n - 1)

proc qux(c: var int) =
  for i in 0 ..< 10000:  # force gc collection of old stack frames
    quux(10)
  echo c  # use c after the loop only

static:
  var b = Bar(baz: 2)
  qux(foo(b, 100))

Araq added a commit that referenced this issue Sep 19, 2019
@Araq Araq closed this as completed in 162d74d Sep 19, 2019
loloicci added a commit to loloicci/nimly that referenced this issue Sep 25, 2019
Some bugs with NimVM are fixed in nim-lang/Nim#10981.
It maybe makes our macros stable, so I remove some echos in compile and
Version bump.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Compiler Crash VM see also `const` label
Projects
None yet
Development

No branches or pull requests

6 participants