PlutoLang / Pluto

A superset of Lua 5.4 with a focus on general-purpose programming.
https://pluto-lang.org
MIT License
367 stars 22 forks source link

Fix safecall not adjusting LOADNIL when more than 1 fixed return is needed #802

Closed Sainan closed 5 months ago

XmiliaH commented 5 months ago

The use of a safecall with multireturns as in c(_G:m?()) will hit the assert in https://github.com/PlutoLang/Pluto/blob/17438aec969da70500d7817bc3a3fe15a5b77fb2/src/lcode.cpp#L2035.

Sainan commented 5 months ago

Yes, I explicitly commented this out in 8cb7e895307fbc11157a1a31ee27ff362dfb7dea because we use OP_CONCAT to fix the L->top, which seems a bit unusual to Lua.

XmiliaH commented 5 months ago

Oh, missed that. But it seems that it does not work when debug hooks are installed:

debug.sethook(function()end, 'c', 1)

print(_G?:m?())

This should print one nil but prints three values.

Sainan commented 5 months ago

I'm afraid this is about where my Lua VM knowledge ends.

XmiliaH commented 5 months ago

I can tell you that the top is changed in: https://github.com/PlutoLang/Pluto/blob/17438aec969da70500d7817bc3a3fe15a5b77fb2/src/ldebug.cpp#L982-L983

But changing that would make this feature not PUC Lua compatible.

Sainan commented 5 months ago

Yeah, this is quite unfortunate. Is there any harm in removing these 2 lines? Because that seems to actually "fix" it.

Otherwise, I guess we limit safecall to only return 1 value. Which would also suck, but at least ensure we're not abusing the VM in terrible ways.

XmiliaH commented 5 months ago

You cannot just remove the two lines. The top is not updated with every instruction, so the real top can be above the current value in top. Therefore, top is adjusted there to the maximum stack usage of the function. It is would be removed, you could overwrite active stack slots with the hook function call.