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

VM regression #7261

Closed
Araq opened this issue Feb 26, 2018 · 5 comments
Closed

VM regression #7261

Araq opened this issue Feb 26, 2018 · 5 comments
Labels
Regression VM see also `const` label

Comments

@Araq
Copy link
Member

Araq commented Feb 26, 2018

This is a regression reported on the forum, see https://forum.nim-lang.org/t/3582

const file = """

sprites.png
size: 1024,1024
format: RGBA8888
filter: Linear,Linear
repeat: none
char/slide_down
  rotate: false
  xy: 730, 810
  size: 204, 116
  orig: 204, 116
  offset: 0, 0
  index: -1
"""

type
  AtlasPage = object
    width, height: int
    path: string
  
  CtsStream = object
    data: string
    pos: int

# ************
# Crappy string stream implementation, because Nim's doesn't work at compile time:
# Probably not interesting, but seems necessary to reproduce the bug.
# ************

proc atEnd(stream: CtsStream): bool =
  stream.pos >= stream.data.len

proc readChar(stream: var CtsStream): char =
  if stream.atEnd:
    result = '\0'
  else:
    result = stream.data[stream.pos]
    inc stream.pos

proc readLine(s: var CtsStream, line: var string): bool =
  # This is pretty much copied from the standard library:
  line.setLen(0)
  while true:
    var c = readChar(s)
    if c == '\c':
      c = readChar(s)
      break
    elif c == '\L': break
    elif c == '\0':
      if line.len > 0: break
      else: return false
    line.add(c)
  result = true

proc peekLine(s: var CtsStream, line: var string): bool =
  let oldPos = s.pos
  result = s.readLine(line)
  s.pos = oldPos

proc initCtsStream(data: string): CtsStream =
  CtsStream(
    pos: 0,
    data: data
  )

# ********************
# Interesting stuff happens here:
# ********************

proc parseAtlas(stream: var CtsStream) =
  var pages: seq[AtlasPage] = @[]
  
  while not stream.atEnd:
    # Read a page:
    pages.add(AtlasPage())
    var page = addr pages[^1]
    var line = ""
    discard stream.readLine(line)
    
    while peekLine(stream, line) and line != "":
      echo page == nil
      discard stream.readLine(line)
      let u0 = 100 / page.width # <----------------------- Crash happens here! --------

static:
  var stream = initCtsStream(file)
  parseAtlas(stream)
  echo "Done!"
@Araq Araq added VM see also `const` label Regression labels Feb 26, 2018
@Araq
Copy link
Member Author

Araq commented Feb 28, 2018

Workaround:

 var page = addr pages[pages.len-1]

@genotrance
Copy link
Contributor

No longer crashes with #head, is it fixed?

-------- OUTPUT --------
Hint: used config file 'c:\users\gt\Desktop\DL\programming\nimdevel\config\nim.cfg' [Conf]
Hint: system [Processing]
Hint: temp [Processing]
false
stack trace: (most recent call last)
temp.nim(88)             temp
temp.nim(84)             parseAtlas
temp.nim(84, 26) Error: attempt to access a nil address
------------------------

@Araq
Copy link
Member Author

Araq commented Sep 14, 2018

Still not fixed, it shouldn't die, it should work.

@LemonBoy
Copy link
Contributor

I believe this is just another case of #1781, the snippet above can be reproduced by replacing the parseAtlas function with the following simplified one:

proc parseAtlas(stream: var CtsStream) =
  var pages = @[AtlasPage()]
  var line = ""

  block:
    let page = addr pages[^1]
    discard stream.peekLine(line)
    discard stream.peekLine(line)
    echo page[]

When the VM compiles this function it generates the following bytecode:

calling parseAtlas
115	LdNull	r2, 4	#vmbug:62
116	LdNull	r3, 5	#vmbug:62
117	LdNullReg	r4, 1	#vmbug:62
118	LdNull	r5, 6	#vmbug:62
119	WrArr	r3, r4, r5	#vmbug:62
120	AddImmInt	r4, r4, r129	#vmbug:62
121	AsgnComplex	r2, r3, r1	#vmbug:62
122	LdNull	r6, 0	#vmbug:63
123	LdConst	r7, "" (11)	#vmbug:63
124	AsgnStr	r6, r7, r1	#vmbug:63
125	LdNull	r8, 7	#vmbug:66
126	LdConst	r9, [] (12)	#vmbug:66
127	AddrReg	r10, r2, r0	#vmbug:66
128	LdImmInt	r11, 1	#vmbug:66
# Call []
129	IndCallAsgn	r8, r9, r3	#vmbug:66
# From here on `r8` is our pointer to the array element
130	LdConst	r12, peekLine (13)	#vmbug:67
131	AsgnRef	r13, r1, r0	#vmbug:67
132	AddrReg	r14, r6, r0	#vmbug:67
133	IndCallAsgn	r4, r12, r3	#vmbug:67
134	LdConst	r16, peekLine (13)	#vmbug:68
135	AsgnRef	r17, r1, r0	#vmbug:68
136	AddrReg	r18, r6, r0	#vmbug:68
137	IndCallAsgn	r15, r16, r3	#vmbug:68
138	LdConst	r20, $ (14)	#system:2763
# Well, fuck. Somehow `r8` now points to a `nkIntLit`!
139	LdDeref	r21, r8, r0	#vmbug:69
140	IndCallAsgn	r19, r20, r2	#vmbug:69
141	Echo	r19, r1, r0	#vmbug:69
142	Ret	r0, r0, r0	#vmbug:62
143	Eof	r0, r0, r0	#vmbug:62

The problem seems to be in the definition of []:

144	LdDeref	r4, r1, r0	#system:3629
145	LdDeref	r5, r1, r0	#system:3629
146	LenSeq	r6, r5, r128	#system:3629
147	Conv	r7, r2, int, BackwardsIndex	#system:3629
150	SubInt	r8, r6, r7	#system:3629
151	LdArr	r5, r4, r8	#system:3629
152	AddrNode	r3, r5, r0	#system:3629
153	AsgnRef	r0, r3, r1	#system:3628
154	Ret	r0, r0, r0	#system:3629
155	Eof	r0, r0, r0	#system:3629

The problem seems to be that even though the r5 slot is marked as slotTempPerm it is permanent for the callee scope only.

As you can see the problem goes away if you turn the [] declarations into templates.

@Araq
Copy link
Member Author

Araq commented Dec 5, 2018

Still an issue today, 2018-12-05.

Araq added a commit that referenced this issue Sep 19, 2019
@Araq Araq closed this as completed in 162d74d Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Regression VM see also `const` label
Projects
None yet
Development

No branches or pull requests

3 participants