search

Fluorohydride / ygopro-core

ygopro script engine.
MIT License
327 stars 134 forks source link

update Lua interpreter #582

Closed salix5 closed 5 months ago

salix5 commented 6 months ago

Fix the following problems of Lua interpreter

The parameter in param_list can be pointer or integer

The parameter should be a union instead of double casting. Now it is stored in union lua_param.

The Lua thread created by interpreter might be collected by GC

https://www.lua.org/manual/5.4/manual.html#lua_newthread Threads are subject to garbage collection, like any Lua object.

https://www.lua.org/manual/5.4/manual.html#2.5 The only guarantees are that Lua will not collect an object that may still be accessed in the normal execution of the program, and it will eventually collect an object that is inaccessible from Lua. (Here, inaccessible from Lua means that neither a variable nor another live object refer to the object.) Because Lua has no knowledge about C code, it never collects objects accessible through the registry (see §4.3), which includes the global environment (see §2.2).

Now the new thread is kept in the registry to avoid GC.

The yielded state is used again to call another function

https://github.com/ProjectIgnis/EDOPro-Core/commit/7651500e569cddcf6497025f168194d29a7a3ac7

https://www.lua.org/manual/5.4/manual.html#4 As in most C libraries, the Lua API functions do not check their arguments for validity or consistency. However, you can change this behavior by compiling Lua with the macro LUA_USE_APICHECK defined.

If we define the macro LUA_USE_APICHECK and build Lua, it will use assertion.

Test: CL1: Maxx C CL2: PSY-Framegear Gamma

Resolution: CL2: PSY-Framegear Gamma Select a PSY-Frame Driver Try to Special Summon PSY-Framegear Gamma

The process will abort because the thread is not LUA_OK.

Lua does not accept using the yielded thread again to call another function. Therefore, current_state should be restored after lua_resume returns.

The parameters pushed by duel_select_synchro_material

duel_select_synchro_material duel_select_tuner_material These functions will:

Since we have restored the current_state after yielding, the parameters should be pushed onto the stack of main thread. Now they are moved to main thread by lua_xmove.

Test

This version has been tested by:

Reference

https://github.com/ProjectIgnis/EDOPro-Core/blob/master/interpreter.h https://github.com/ProjectIgnis/EDOPro-Core/blob/master/interpreter.cpp https://github.com/ProjectIgnis/EDOPro-Core/commit/7651500e569cddcf6497025f168194d29a7a3ac7

Related issue

https://github.com/Fluorohydride/ygopro-core/pull/417 https://github.com/Fluorohydride/ygopro/issues/2407

@edo9300 My revision: https://github.com/ProjectIgnis/EDOPro-Core/blob/bc3e680bb3041e4abda0ef39c3acb9dac64b2757/interpreter.cpp#L523 For basic types and pointers, std::exchange might be unnecessary?

@mercury233 @purerosefallen

purerosefallen commented 5 months ago

Is this one only working on Lua 5.4? In prod most servers deployed Lua 5.3

edo9300 commented 5 months ago

@edo9300 My revision: https://github.com/ProjectIgnis/EDOPro-Core/blob/bc3e680bb3041e4abda0ef39c3acb9dac64b2757/interpreter.cpp#L523 For basic types and pointers, std::exchange might be unnecessary?

It's more about using the right library functions to keep the code cleaner, the optimizer will take care of making that code the exact same as if you were to do it manually in any case. Also, for future reference (even for linking), you should use my repo, rather than the (very outdated) mirror under our org.

salix5 commented 5 months ago

Is this one only working on Lua 5.4? In prod most servers deployed Lua 5.3

It works in Lua 5.3.6

@edo9300 My revision: https://github.com/ProjectIgnis/EDOPro-Core/blob/bc3e680bb3041e4abda0ef39c3acb9dac64b2757/interpreter.cpp#L523 For basic types and pointers, std::exchange might be unnecessary?

It's more about using the right library functions to keep the code cleaner, the optimizer will take care of making that code the exact same as if you were to do it manually in any case. Also, for future reference (even for linking), you should use my repo, rather than the (very outdated) mirror under our org.

Thank you!