Rework stack-frame handling in the VM #255

zerbina · 2022-03-18T15:28:55Z

Instead of having the stack frames as ref objects that are chained
together via their next field, store them in a seq in TCtx. This
makes it easier to reason about them, and also connects the frames
to their owning context.

While the frame list should ideally be first-in last-out (stack), this
isn't currently possible due to how exception handling is implemented.

In addition, the stack trace now always includes the entry function
and also the number of skipped frames. A nil access error when
no entry function exists (happens when a statement is evaluated)
is also fixed.

Notes for reviewers:

The name "tos" is still used for variables/parameters storing the stack frame index. I was not able to figure out what it's supposed to mean/stand for. If wanted, I can replace it's usages with something more descriptive

compiler/vm/vm.nim

zerbina · 2022-03-18T20:30:21Z

Hmm, it seems like shallowCopy for seq doesn't work as I expected it to with ORC

compiler/vm/vm.nim

saem · 2022-03-20T18:22:31Z

I think tos stands for "top of stack"

saem

This looks good to me.

I do think simplifying the stack frame printing with a trivial loop and a comment saying what it's supposed to do would help a fair bit. See my review comment.

compiler/vm/vm.nim

saem · 2022-03-20T18:21:14Z

compiler/vm/vm.nim

+  let bottom = high(c.sframes)
+  let top = max(0, bottom - recursionLimit)
+
+  # TODO: Skipped number of entries don't get reported? How is/was this intended to be handled?


I think the might be a bug, but for a comment on the whole proc it shouldn't be a recursive implementation, it can easily be a loop:

The intention:

print the most recent frame last

descend the stack and do so again

until we run out of stack or reach depth

if any frames remain, say how many we skipped

The above results in a message such as: "skipped x frames, calls that led up to printing".

compiler/vm/vm.nim

zerbina · 2022-03-21T16:48:01Z

compiler/vm/vmdef.nim

    prc*: PSym                 # current prc; proc that is evaluated
    slots*: seq[TFullReg]      # parameters passed to the proc + locals;
                              # parameters come first
-    next*: PStackFrame         # for stacking
+    next*: StackFrameIndex         # for stacking


next could use Option[StackFrameIndex] instead of just StackFrameIndex. This would have the advantage of the default value meaning "no previous frame" (not the case right now), but would also require more space and make access a bit slower.

A sentinel index would work as well?

Hmm, the real trick would be extending optional, or at least a new data type, that allows for "in range" sentinel/nil like values.

Can do this later, but it's a pretty common thing in DOD land.

zerbina · 2022-03-21T16:48:38Z

compiler/vm/vm.nim

 proc execute(c: var TCtx, start: int): PNode =
-  var tos = PStackFrame(prc: nil, comesFrom: 0, next: nil)
+  # XXX: instead of building the object first and then adding it to the list, it could


What's the preferred style here? Adding to the list first and then modifying the element might be a bit more efficient (due to no copy/move having to take place)

It's not a hot loop in my opinion.

For the thought exercise:
Since it's a seq of capacity x, then it's automatically initialized to the zero value per element for each frame. Can we of we set the len to 1 afterwards, then just set the one field to -1, should do it?

Instead of having the stack frames as `ref` objects that are chained together via their `next` field, store them in a `seq` in `TCtx`. This makes it easier to reason about them, and also connects the frames to their owning context. While the frame list should ideally be first-in last-out (stack), this isn't currently possible due to how exception handling is implemented. In addition, the stack trace now always includes the entry function and also the number of skipped frames. A nil access error when no entry function exists (happens when a statement is evaluated) is also fixed.

zerbina · 2022-03-21T17:02:41Z

I transformed the recursive call into a loop and also implemented reporting for the number of skipped frames. There was also a possible nil access error that I fixed.

Compilation under ORC should also succeed again.

Since error reporting is the domain of @haxscramper, maybe they can weigh in on if they're okay with the reporting related changes.

haxscramper · 2022-03-21T17:38:31Z

If you consider it necessary, you can add or remove report kinds and change how things are printed. If you think it would be better, then go for it. IIRC error messages from the VM are barely tested (if tested at all), so any improvement would be a win.

saem · 2022-03-22T00:49:48Z

compiler/vm/vm.nim


  var res = SemReport(kind: rsemVmStackTrace)
  res.currentExceptionA = c.currentExceptionA
  res.currentExceptionB = c.currentExceptionB

-  aux(c, sframe, pc, 0, res)
+  block:
+    var i = sframe


I prefer the var/let block syntax, but don't change it, mostly fyi

saem · 2022-03-22T00:51:23Z

compiler/vm/vm.nim

-
-      aux(c, sframe.next, sframe.comesFrom, depth + 1, res)
-      res.stacktrace.add((sym: sframe.prc, location: c.debug[pc]))
+  ## Generate and report the stack trace starting at frame `sframe` (inclusive).


Nailed it! 🤩

saem

Bors r+

bors · 2022-03-22T01:33:04Z

Build succeeded:

zerbina commented Mar 18, 2022

View reviewed changes

compiler/vm/vm.nim Outdated Show resolved Hide resolved

zerbina force-pushed the vm-stack-frames branch from 4d982d0 to 9fe2b48 Compare March 18, 2022 15:44

alaviss reviewed Mar 20, 2022

View reviewed changes

compiler/vm/vm.nim Outdated Show resolved Hide resolved

saem requested changes Mar 20, 2022

View reviewed changes

zerbina force-pushed the vm-stack-frames branch from 9fe2b48 to 3ce065a Compare March 21, 2022 16:42

zerbina commented Mar 21, 2022

View reviewed changes

zerbina force-pushed the vm-stack-frames branch from 3ce065a to fb03691 Compare March 21, 2022 16:52

saem reviewed Mar 22, 2022

View reviewed changes

saem approved these changes Mar 22, 2022

View reviewed changes

bors bot merged commit f9a6578 into nim-works:devel Mar 22, 2022

zerbina deleted the vm-stack-frames branch March 25, 2022 18:23

haxscramper added this to the VM backend refactoring milestone Nov 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework stack-frame handling in the VM #255

Rework stack-frame handling in the VM #255

zerbina commented Mar 18, 2022 •

edited

Loading

zerbina commented Mar 18, 2022

saem commented Mar 20, 2022

saem left a comment

saem Mar 20, 2022

zerbina Mar 21, 2022

saem Mar 22, 2022

saem Mar 22, 2022 •

edited

Loading

zerbina Mar 21, 2022

saem Mar 22, 2022

zerbina commented Mar 21, 2022

haxscramper commented Mar 21, 2022

saem Mar 22, 2022

saem Mar 22, 2022

saem left a comment

bors bot commented Mar 22, 2022

Rework stack-frame handling in the VM #255

Rework stack-frame handling in the VM #255

Conversation

zerbina commented Mar 18, 2022 • edited Loading

zerbina commented Mar 18, 2022

saem commented Mar 20, 2022

saem left a comment

Choose a reason for hiding this comment

saem Mar 20, 2022

Choose a reason for hiding this comment

zerbina Mar 21, 2022

Choose a reason for hiding this comment

saem Mar 22, 2022

Choose a reason for hiding this comment

saem Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

zerbina Mar 21, 2022

Choose a reason for hiding this comment

saem Mar 22, 2022

Choose a reason for hiding this comment

zerbina commented Mar 21, 2022

haxscramper commented Mar 21, 2022

saem Mar 22, 2022

Choose a reason for hiding this comment

saem Mar 22, 2022

Choose a reason for hiding this comment

saem left a comment

Choose a reason for hiding this comment

bors bot commented Mar 22, 2022

zerbina commented Mar 18, 2022 •

edited

Loading

saem Mar 22, 2022 •

edited

Loading