Scripting: Fix Lua 5.1/LuaJIT and Lua 5.2 problems #3376

nuive · 2024-12-16T22:01:08Z

What's added:

-New option available for CMake USE_LUA option. Setting it to "JIT" will force CMake to look for LuaJIT when building.

What's fixed:

-Lua 5.1/JIT couldn't even load a script.
-Updated lua pairs/ipairs handling (Lua 5.1/JIT and 5.2 weren't able to manage things like mSTList iteration). Now pairs/ipairs work like Lua 5.4 (except for a lua_callk which stays lua_call).
-Custom objects show their propper names using Lua versions prior to 5.3.

What's not fixed:

I didn't find anything else needing a fix regarding specific Lua versions.

endrift

Tagging in @ahigerd for this. I'm not sure why some of these changes were made, and I'm concerned about the provenance of some of the code.

src/platform/cmake/FindLuaJIT.cmake

endrift · 2024-12-22T08:45:24Z

src/script/engines/lua.c

+const char* luaL_tolstring (lua_State* lua, int idx, size_t* len) {
+	if (luaL_callmeta(lua, idx, "__tostring")) {
+		if (!lua_isstring(lua, -1)) {
+			luaL_error(lua, "'__tostring' must return a string");
+		}
+	} else {
+		switch (lua_type(lua, idx)) {
+			case LUA_TNUMBER:
+			case LUA_TSTRING:
+				lua_pushvalue(lua, idx);
+				break;
+			case LUA_TBOOLEAN:
+				lua_pushstring(lua, (lua_toboolean(lua, idx) ? "true" : "false"));
+				break;
+			case LUA_TNIL:
+				lua_pushliteral(lua, "nil");
+				break;
+			default:
+				luaL_getmetafield(lua, idx, "__name");
+				int tt = lua_type(lua, -1);
+				const char* name = (tt == LUA_TSTRING) ? lua_tostring(lua, -1) :
+				                                         luaL_typename(lua, idx);


Don't copy source code directly from Lua, please. The license may permit this, but it'd be much better to replace the one call to this function with something like what Lua 5.1 does in its print function: it just calls tostring on the argument directly to coerce it.

Ironic: My print() shim calls luaL_tostring explicitly to avoid doing that.

That said, 5.1's tostring doesn't honor __name, so it becomes a question for @endrift: Is the better debugging/introspection worth the extra work?

src/script/engines/lua.c

CMakeLists.txt

src/platform/cmake/FindLuaJIT.cmake

src/script/engines/lua.c

ahigerd · 2024-12-23T17:40:45Z

I think the implementation of _luaLoad is missing some fenv fixes.

nuive · 2024-12-23T20:39:59Z

I think the implementation of _luaLoad is missing some fenv fixes.

Like what?

ahigerd · 2024-12-23T21:06:18Z

I think the implementation of _luaLoad is missing some fenv fixes.

Like what?

      // Create new _ENV
      lua_newtable(luaContext->lua);
  
      // Make the old _ENV the __index in the metatable
      lua_newtable(luaContext->lua);
      lua_pushliteral(luaContext->lua, "__index");
      lua_getupvalue(luaContext->lua, -4, 1);
      lua_rawset(luaContext->lua, -3);
  
      lua_pushliteral(luaContext->lua, "__newindex");
      lua_getupvalue(luaContext->lua, -4, 1);
      lua_rawset(luaContext->lua, -3);
  
      lua_setmetatable(luaContext->lua, -2);
  
      lua_pushliteral(luaContext->lua, "script");
      lua_newtable(luaContext->lua);

This is _ENV manipulation which means it needs to be handled using setfenv instead. It's probably a little nontrivial to do correctly but if you don't do it then require() won't behave correctly.

nuive · 2024-12-24T00:45:35Z

      // Create new _ENV
      lua_newtable(luaContext->lua);
  
      // Make the old _ENV the __index in the metatable
      lua_newtable(luaContext->lua);
      lua_pushliteral(luaContext->lua, "__index");
      lua_getupvalue(luaContext->lua, -4, 1);
      lua_rawset(luaContext->lua, -3);
  
      lua_pushliteral(luaContext->lua, "__newindex");
      lua_getupvalue(luaContext->lua, -4, 1);
      lua_rawset(luaContext->lua, -3);
  
      lua_setmetatable(luaContext->lua, -2);
  
      lua_pushliteral(luaContext->lua, "script");
      lua_newtable(luaContext->lua);

This is _ENV manipulation which means it needs to be handled using setfenv instead. It's probably a little nontrivial to do correctly but if you don't do it then require() won't behave correctly.

Yes, I know. In fact mGBA built with Lua 5.1 breaks if this is not handled propperly.

However if you look at the submited changes there's this:

		// Create new _ENV
		lua_newtable(luaContext->lua);

		// Make the old _ENV the __index in the metatable
		lua_newtable(luaContext->lua);
		lua_pushliteral(luaContext->lua, "__index");
#if LUA_VERSION_NUM >= 502
		lua_getupvalue(luaContext->lua, -4, 1);
#else
		lua_getfenv(luaContext->lua, -4);
#endif
		lua_rawset(luaContext->lua, -3);

		lua_pushliteral(luaContext->lua, "__newindex");
#if LUA_VERSION_NUM >= 502
		lua_getupvalue(luaContext->lua, -4, 1);
#else
		lua_getfenv(luaContext->lua, -4);
#endif
		lua_rawset(luaContext->lua, -3);

		lua_setmetatable(luaContext->lua, -2);

		lua_pushliteral(luaContext->lua, "script");
		lua_newtable(luaContext->lua);

		if (dirname[0]) {
			lua_pushliteral(luaContext->lua, "require");
			lua_pushstring(luaContext->lua, dirname);
			lua_pushcclosure(luaContext->lua, _luaRequireShim, 1);
			lua_rawset(luaContext->lua, -5);

			lua_pushliteral(luaContext->lua, "dir");
			lua_pushstring(luaContext->lua, dirname);
			lua_rawset(luaContext->lua, -3);
		}

		if (name[0] == '@') {
			lua_pushliteral(luaContext->lua, "path");
			lua_pushstring(luaContext->lua, &name[1]);
			lua_rawset(luaContext->lua, -3);
		}

		lua_rawset(luaContext->lua, -3);
#if LUA_VERSION_NUM >= 502
		lua_setupvalue(luaContext->lua, -2, 1);
#else
		lua_setfenv(luaContext->lua, -2);
#endif
		luaContext->func = luaL_ref(luaContext->lua, LUA_REGISTRYINDEX);
		return true;

Woudln't this be enough? Am I missing any (set/get)fenv?

ahigerd · 2024-12-24T00:58:37Z

Hmm... I may have misread the code. That might be correct after all.

nuive · 2024-12-28T20:02:29Z

I think I made all requested changes...

ahigerd · 2024-12-29T23:51:36Z

src/script/engines/lua.c

+	lua_pop(lua, 1);
+}
+
+const char* _luaTolstring(lua_State* lua, int idx, size_t* len) {


@endrift Bikeshed question: Should this function wrap luaL_tostring for 5.3+, or should we polyfill luaL_tostring so that 5.3+ code compiles under 5.1/5.2?

(Tangentially related, but not a task for this PR: Is there a particular reason why the internal functions in this file aren't declared static?)

ahigerd · 2024-12-29T23:54:14Z

src/platform/cmake/FindLuaJIT.cmake

+    luajit${LUAJIT_VERSION_MAJOR}.${LUAJIT_VERSION_MINOR}
+    luajit-${LUAJIT_VERSION_MAJOR}.${LUAJIT_VERSION_MINOR}
+    luajit.${LUAJIT_VERSION_MAJOR}.${LUAJIT_VERSION_MINOR}
+    lua51


Does LuaJIT actually provide a lua51 soname? That seems weird...

Good point. That is an inconsistency.

I'm using a win64 docker and MinGW's LuaJIT has same library names as standard Lua 5.1. In unix that's not true, names would be something like luajit51, luajit5.1...

Hm, if that's the standard soname that's getting shipped for the package in MSYS2, then I guess it should stay as it is.

Well, I've added luajit51 variants in order to be able to build using unix systems. I don't know if I should left the luajit${LUAJIT_VERSION_MAJOR}${LUAJIT_VERSION_MINOR} variants into the list. I've found LuaJIT installations where the library was named something like libluajit2.1.so or something like that, but I supose those name conventions are customized. Should I remove ${LUAJIT_VERSION_MAJOR}${LUAJIT_VERSION_MINOR} variants from the list leaving only luajit51 and lua51 ones, or it's alrhigt like it is now? Currently possibly library names list has 12 items...

ahigerd

I think I'm basically satisfied?

endrift requested changes Dec 22, 2024

View reviewed changes

ahigerd reviewed Dec 23, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

ahigerd reviewed Dec 23, 2024

View reviewed changes

src/platform/cmake/FindLuaJIT.cmake Outdated Show resolved Hide resolved

ahigerd reviewed Dec 23, 2024

View reviewed changes

src/platform/cmake/FindLuaJIT.cmake Outdated Show resolved Hide resolved

ahigerd reviewed Dec 23, 2024

View reviewed changes

src/script/engines/lua.c Outdated Show resolved Hide resolved

nuive force-pushed the master branch 2 times, most recently from d48a3d9 to 964f505 Compare December 28, 2024 20:00

ahigerd reviewed Dec 29, 2024

View reviewed changes

ahigerd approved these changes Dec 30, 2024

View reviewed changes

Scripting: Fix Lua 5.1/LuaJIT and Lua 5.2 problems

c552a96

nuive force-pushed the master branch from 964f505 to c552a96 Compare December 30, 2024 11:59

nuive mentioned this pull request Jan 2, 2025

Scripting: Some issues building with Lua 5.1 #2967

Open

endrift linked an issue Jan 3, 2025 that may be closed by this pull request

Scripting: Some issues building with Lua 5.1 #2967

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scripting: Fix Lua 5.1/LuaJIT and Lua 5.2 problems #3376

Scripting: Fix Lua 5.1/LuaJIT and Lua 5.2 problems #3376

nuive commented Dec 16, 2024

endrift left a comment

endrift Dec 22, 2024

ahigerd Dec 23, 2024

ahigerd commented Dec 23, 2024

nuive commented Dec 23, 2024

ahigerd commented Dec 23, 2024

nuive commented Dec 24, 2024

ahigerd commented Dec 24, 2024

nuive commented Dec 28, 2024

ahigerd Dec 29, 2024

ahigerd Dec 29, 2024

nuive Dec 30, 2024

ahigerd Dec 30, 2024

nuive Dec 31, 2024

ahigerd left a comment

Scripting: Fix Lua 5.1/LuaJIT and Lua 5.2 problems #3376

Are you sure you want to change the base?

Scripting: Fix Lua 5.1/LuaJIT and Lua 5.2 problems #3376

Conversation

nuive commented Dec 16, 2024

endrift left a comment

Choose a reason for hiding this comment

endrift Dec 22, 2024

Choose a reason for hiding this comment

ahigerd Dec 23, 2024

Choose a reason for hiding this comment

ahigerd commented Dec 23, 2024

nuive commented Dec 23, 2024

ahigerd commented Dec 23, 2024

nuive commented Dec 24, 2024

ahigerd commented Dec 24, 2024

nuive commented Dec 28, 2024

ahigerd Dec 29, 2024

Choose a reason for hiding this comment

ahigerd Dec 29, 2024

Choose a reason for hiding this comment

nuive Dec 30, 2024

Choose a reason for hiding this comment

ahigerd Dec 30, 2024

Choose a reason for hiding this comment

nuive Dec 31, 2024

Choose a reason for hiding this comment

ahigerd left a comment

Choose a reason for hiding this comment