Fix slot array corrupted by wrenInterpret()

[ayuusweetfish]

Fixes #710 (discussions happen there) and add a test case. Credit largely goes to @liquid600pgm for providing detailed analysis.

[mhermier]

AFAIK this change only fix a small subset of errors related to wrenEnsureSlot. Otherwise could be enough to fix this particular issue.

[ayuusweetfish]

Yes, but slot manipulation between multiple wrenInterpret() calls could be a common usage, so a dedicated test can be a good idea. Other similar issues I know involve some kind of re-entrancy (invoking Wren API inside foreign methods).

[mhermier]

Yes but theses cases are in fact pretty common, but a little bit hard to predict to produce reliable test cases. Le lun. 10 févr. 2020 à 11:27, kawa-yoiko <notifications@github.com> a écrit :

…

Yes, but slot manipulation between multiple wrenInterpret() calls could be a common usage, so a dedicated test can be a good idea. Other similar issues I know involve some kind of re-entrancy (invoking Wren API inside foreign methods). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#730?email_source=notifications&email_token=AGHNXFIFJUJPFLO6ULNRMULRCETZ5A5CNFSM4KSISHE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELH7R2Q#issuecomment-584055018>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGHNXFIV5RECR6DKCJKNXB3RCETZ5ANCNFSM4KSISHEQ> .

[ayuusweetfish]

Indeed, it can be difficult to ensure test coverage for similar issues.

Maybe tests for unfixed issues should be added when they get understood more and fixed. Before that, black-box tests emulating common usage patterns can partly fulfill the need.

[mhermier]

Sure, we can delay resolution of other bugs. The same question always arise at the end of the question, is the API good to correctly fix the situation, and AFAIK no one achieve to convince at least me that the API needs to be changed.

[mhermier]

I meant that the API needs to be changed. Sorry for my bad phrase construction.

[ayuusweetfish]

I meant that the API needs to be changed.

The API related to slot array or something else? Would be interested to hear your reasons.

[mhermier]

There are various small problems in the API, but the most visible one is the managed reference to the execution context from the WrenVM object. A simple use case to show the problem is: how do you retrieve a returned object after wrenInterpret? Also some problems comes from speed trades of, that are influenced by that WrenVM design. The most visible issue, is the apiStack value caching that is easy to corrupt. And despite the effort to hide/limit it, the VM is reentrant in some places and tends to corrupt apiStack, in some situations.

[ruby0x1]

Thanks @kawa-yoiko this is a good step forward, and thanks to @liquid600pgm too.