Stack corruption

[mhermier]

Here is a reduced test to the long standing stack corruption issue:

// Level of required nesting is dependant on INITIAL_CALL_FRAMES
var Fn1 = Fn.new {|acc| Fiber.yield(acc) }
var Fn2 = Fn.new {|acc| Fn1.call(acc, null) }
var Fn3 = Fn.new {|acc| Fn2.call(acc, null) }
var Fn4 = Fn.new {|acc| Fn3.call(acc, null) }
var Fiber1 = Fiber.new { Fn4.call(42) }

System.print(Fiber1.call()) // expect: 42
System.print(Fiber1.call("caramba")) // expect: caramba

Instead of returning caramba, second call to it returns 42. I have not been able to weaponize it to make the test crash, but at least it can be tested somehow.

[mhermier]

Another one based on #770

var Fib1 = Fn.new {|n|
  if (n <= 2) {
    return 1
  } else {
    return Fib1.call(n-1) + Fib1.call(n-2)
  }
}

var Fib2 = Fn.new {|n|
  if (n <= 2) {
    return 1
  } else {
    var left = Fib2.call(n-1)
    var right = Fib2.call(n-2)
    return left + right
  }
}

System.print(Fib2.call(12)) // expect: 144
System.print(Fib1.call(12)) // expect: 144

Just in case, reducing it to:

var Fib = Fn.new {|n|
  if (n <= 2) {
    return 1
  } else {
    return Fib.call(n-1) + Fib.call(n-2)
  }
}

System.print(Fib.call(12)) // expect: 144

Trigger the bug but does not make it visible to user for some reasons.

[ruby0x1]

Thanks, I think we have a fix for this, at least in my engine where I have a fix applied for another bug, this one is giving expected results too.

[mhermier]

Could you issue a pull request of that fix before merging? Since that stuff is in the critical path, and can have possible huge impacts in speed.

I also have a crude patch that fix that issue by doing a STORE_FRAME/LOAD_FRAME around the switch (method->type) block that work quite well. It is a little bit expensive (in term of time), but seems to cover all the case.

Another solution would be to revert, the function call change.

So before you push, please allow to evaluate your solution.

[ruby0x1]

Don't forget the benchmarks exist.

The first is baseline, the second 2 are the "heavy" fix. STORE/LOAD frame around the whole primitive block.

This is the better fix, just waiting for Bob to give me some feedback before I bring it in.

[mhermier]

Could you provide both patch for review. Numbers are too pretty good to be true.

[mhermier]

#801 Seems to be another variation of the issue.

[mathewmariani]

When will this MR be available?

[ruby0x1]

I have no estimate. Waiting for @munificent to send some thoughts my way.

(It IS 2020 though, it's reasonable to expect some delay due to priorities)

[ruby0x1]

All cases listed above are working as expected with #807 merged!