Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistency with lua_ref #247

Closed
codecat opened this issue Nov 28, 2021 · 12 comments
Closed

Inconsistency with lua_ref #247

codecat opened this issue Nov 28, 2021 · 12 comments
Labels
bug Something isn't working

Comments

@codecat
Copy link

codecat commented Nov 28, 2021

Given the following program with regular Lua 5.1:

int main()
{
        lua_State* L = luaL_newstate();

        const char* source = "return \"test\"";
        size_t sourceSize = strlen(source);

        luaL_loadbuffer(L, source, sourceSize, "test");
        lua_call(L, 0, 1);

        printf("type = %d\n", lua_type(L, -1));

        int r = luaL_ref(L, LUA_REGISTRYINDEX);
        lua_rawgeti(L, LUA_REGISTRYINDEX, r);

        printf("type = %d\n", lua_type(L, -1));

        lua_close(L);
        return 0;
}

The output is, as one would expect:

type = 4
type = 4

When the following similar program is run on Luau:

int main()
{
	lua_State* L = luaL_newstate();

	const char* source = "return \"test\"";
	size_t sourceSize = strlen(source);

	size_t bytecodeSize;
	char* bytecode = luau_compile(source, sourceSize, nullptr, &bytecodeSize);
	luau_load(L, "test", bytecode, bytecodeSize, 0);
	lua_call(L, 0, 1);

	printf("type = %d\n", lua_type(L, -1));

	int r = lua_ref(L, LUA_REGISTRYINDEX);
	lua_rawgeti(L, LUA_REGISTRYINDEX, r);

	printf("type = %d\n", lua_type(L, -1));

	lua_close(L);
	return 0;
}

The types are suddenly different:

type = 5
type = 6

With 5 being LUA_TSTRING and 6 being LUA_TTABLE.

Perhaps I'm missing something in the difference between lua_ref and luaL_ref, but this seems wrong to me on first sight.

@codecat codecat added the bug Something isn't working label Nov 28, 2021
@zeux
Copy link
Collaborator

zeux commented Nov 28, 2021

lua_ref in Luau uses the standard convention where the passed argument is the stack index. lua_ref(L, -1) is what you probably want; lua_ref(L, LUA_REGISTRYINDEX) saves a reference to the registry to the registry. In Lua 5.1 I believe luaL_ref is hardwired to work with the top element on the stack, hence the difference that you observe.

@zeux
Copy link
Collaborator

zeux commented Nov 28, 2021

P.S. Since passing LUA_REGISTRYINDEX to lua_ref is likely an error in every case perhaps an api_check is in order to prevent misuse / avoid this mistake.

@codecat
Copy link
Author

codecat commented Nov 28, 2021

Ah yes - I was following this page. Perhaps it would be good to list this difference in the documentation somewhere. Unless it's already there and I missed it.

@kunitoki
Copy link
Contributor

I encountered a similar problem while working on the Luau port of LuaBridge3 (kunitoki/LuaBridge3#12) and i had to basically write some wrapper calls for luaL_ref and luaL_unref (https://github.com/kunitoki/LuaBridge3/pull/12/files#diff-62eaf77f04843ba30bd2cd74ea720cb4fa899e0d6ab5df08db16280f09e31cb4R18-R72) in order to make it work.

in any case i still get issues from unit tests where from time to time when calling lua_rawgeti(L, LUA_REGISTRYINDEX, ref); with a ref returned from lua_ref:

void lua_rawgeti(lua_State* L, int idx, int n)
{
    luaC_checkthreadsleep(L);
    StkId t = index2adr(L, idx);
    api_check(L, ttistable(t));
    setobj2s(L, L->top, luaH_getnum(hvalue(t), n));
    api_incr_top(L); // <<<<<<<<< Thread 1: EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0)
    return;
}

@zeux
Copy link
Collaborator

zeux commented Nov 29, 2021

This indicates that you are exceeding the minimum stack space; if you work with errored or yielded threads, they don't provide a stack space guarantee and as such you need to call lua_checkstack before working with the thread.

@kunitoki
Copy link
Contributor

It's strange as LuaJIT automatically grows the stack as needed (to LUA_MAXSTACK). Isn't Luau doing the same ?

@zeux
Copy link
Collaborator

zeux commented Nov 29, 2021

The stack space guarantee that Luau provides is only there when the C function is called from the VM, I'm assuming you're working with threads externally, e.g. the rawgeti call above is not done as part of a C function that is executed from scripts?

@zeux
Copy link
Collaborator

zeux commented Nov 29, 2021

Either way, stack space guarantees are separate from lua_ref - feel free to create a discussion thread for this with more information on the specific situation! It's possible that Luau is stricter on stack space than Lua 5.1 but it's also possible that the code that worked on Lua 5.1 works by accident.

This issue will be fixed by adding an assertion to lua_ref later this week to prevent misuse. I'll also create an issue for documentation on the C API which we simply lack atm, which would be a good place to note differences from Lua.

@kunitoki
Copy link
Contributor

kunitoki commented Nov 30, 2021

It seems that lua_ref is not popping the object from the stack (unline luaL_ref). is that right ?

@zeux
Copy link
Collaborator

zeux commented Nov 30, 2021

Yeah that's intentional; lua_ref doesn't change the stack in any way, it simply pins the object and returns the reference - same for unref.

@zeux
Copy link
Collaborator

zeux commented Nov 30, 2021

The correct implementation for luaL_ref in Luau looks something like this I think:

int luaL_ref(lua_State* L, int t)
{
    assert(t == LUA_REGISTRYINDEX);
    int r = lua_ref(L, -1);
    lua_pop(L, 1);
    return r;
}

zeux added a commit that referenced this issue Dec 3, 2021
- Fix some cases where type checking would overflow the native stack
- Improve autocomplete behavior when assigning a partially written function call (not currently exposed through command line tools)
- Improve autocomplete type inference feedback for some expressions where previously the type would not be known
- Improve quantification performance during type checking for large types
- Improve type checking for table literals when the expected type of the table is known because of a type annotation
- Fix type checking errors in cases where required module has errors in the resulting type
- Fix debug line information for multi-line chained call sequences (Add function name information for "attempt to call a nil value" #255)
- lua_newuserdata now takes 2 arguments to match Lua/LuaJIT APIs better; lua_newuserdatatagged should be used if the third argument was non-0.
- lua_ref can no longer be used with LUA_REGISTRYINDEX to prevent mistakes when migrating Lua FFI (Inconsistency with lua_ref #247)
- Fix assertions and possible crashes when executing script code indirectly via metatable dispatch from lua_equal/lua_lessthan/lua_getfield/etc. (Hitting a crash in an assert after lua_equal is called. #259)
- Fix flamegraph scripts to run under Python 2
@zeux
Copy link
Collaborator

zeux commented Dec 3, 2021

lua_ref now has an assertion to prevent misuse; subsequent improvements to documentation are tracked in #251

@zeux zeux closed this as completed Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

3 participants