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

Wrong function gets called when registering table with lambdas #608

Closed
knorrest opened this issue Mar 12, 2018 · 12 comments
Closed

Wrong function gets called when registering table with lambdas #608

knorrest opened this issue Mar 12, 2018 · 12 comments

Comments

@knorrest
Copy link

knorrest commented Mar 12, 2018

The code below when compiled with gcc 7.3.0 produces the problem (MSVC works as it should)
The resulting output is then really strange:

log_fatal: Some log entry...
log_fatal: Some log entry...
log_fatal: Some log entry...
log_fatal: Some log entry...
log_fatal: Some log entry...
log_fatal: Some log entry...

so apparently the registration of methods fails somehow (or am I doing something really stupid..)

`TEST(LibraryTests, CallLogMethods)
{
auto L = luaL_newstate();
sol::state_view state(L);

sol::table table(state, sol::create);
table.set(
	"log_fatal", [](const std::string& s) {std::cout << "log_fatal: " << s << std::endl; },
	"log_error", [](const std::string& s) {std::cout << "log_error: " << s << std::endl; },
	"log_warn", [](const std::string& s) {std::cout << "log_warn: " << s << std::endl; },
	"log_info", [](const std::string& s) {std::cout << "log_info: " << s << std::endl; },
	"log_debug", [](const std::string& s) {std::cout << "log_debug: " << s << std::endl; },
	"log_trace", [](const std::string& s) {std::cout << "log_trace: " << s << std::endl; }
);
state["rt"] = table;

state.safe_script(R"(
	rt.log_fatal("Some log entry...")
	rt.log_error("Some log entry...")
	rt.log_warn("Some log entry...")
	rt.log_info("Some log entry...")
	rt.log_debug("Some log entry...")
	rt.log_trace("Some log entry...")
)");

}`

@ThePhD
Copy link
Owner

ThePhD commented Mar 12, 2018

This is the second 7.3.0 bug report I've had where things work bizarre. Was gcc 7.3.0 recently released or something...?

Either way, my GCC harness is absolutely busted right now so I can't repro anything specifically gcc related. I would try to see if you can use individual .table.set calls, or just do table["log_X"] = { lambda here... };. Maybe gcc will behave better than.

@ThePhD ThePhD self-assigned this Mar 12, 2018
@ThePhD ThePhD added this to the Bugs milestone Mar 12, 2018
@ThePhD
Copy link
Owner

ThePhD commented Mar 12, 2018

Finally got to investigate. Looks like a compiler bug...?

@ThePhD
Copy link
Owner

ThePhD commented Mar 12, 2018

	auto L = luaL_newstate();
	sol::state_view state(L);

	sol::table table(state, sol::create);
	table.set(
		"log_fatal", [](const std::string& s) {std::cout << "log_fatal: " << s << std::endl; },
		"log_error", [](std::string s) {std::cout << "log_error: " << s << std::endl; },
		"log_warn", [](const std::string& s) {std::cout << "log_warn: " << s << std::endl; },
		"log_info", [](const std::string& s) {std::cout << "log_info: " << s << std::endl; },
		"log_debug", [](const std::string& s) {std::cout << "log_debug: " << s << std::endl; },
		"log_trace", [](const char* s) {std::cout << "log_trace: " << s << std::endl; }
	);
	state["rt"] = table;

	state.safe_script(R"(
	rt.log_fatal("Some log entry...")
	rt.log_error("Some log entry...")
	rt.log_warn("Some log entry...")
	rt.log_info("Some log entry...")
	rt.log_debug("Some log entry...")
	rt.log_trace("Some log entry...")
)");

With different signatures, the functions start to differentiate properly. It's time to investigate GCC and the standard...

My solution for you is to avoid the lambdas if possible.

@knorrest
Copy link
Author

I noticed that too, and it really feels like a compiler bug. And sure, I can avoid lambdas, but I have loads of them in some places... Still, I really appreciate your quick replies!
I had an even stranger behaviour when I had a counter

auto dbg_counter = 0;
...
"log_debug", [](const std::string& s) {std::cout << "log_debug: " << s << std::endl; ++dbg_counter},
...
state.safe_script(R"(
rt.log_debug("Some log entry...")
)");
ASSERT_EQ(dbg_counter, 1)

The assert passed, but log_fatal was printed...

@ThePhD
Copy link
Owner

ThePhD commented Mar 13, 2018

I called the lambdas directly and they all print out the correct string and perform the correct action. This... bothers me, greatly.

@ThePhD
Copy link
Owner

ThePhD commented Mar 13, 2018

What version of Lua are you using, perchance?

@knorrest
Copy link
Author

I use the latest release - 5.3.4.

@ThePhD
Copy link
Owner

ThePhD commented Mar 14, 2018

Long search later, found the issue.

You can fix this by doing .set_function, which will handle this case properly. If you use set, we have to assume it's a class, and thusly this problem can pop up. I'll add a note to the docs about registering things with set using multiple lambdas.

@knorrest
Copy link
Author

Ok, I've fixed it by using set_function. I don't fully understand the problem though - was this purely a sol thing and not GCC related?

@ThePhD
Copy link
Owner

ThePhD commented Mar 14, 2018

It's purely a sol2 thing. Sort of.

We rely on unique type names in order to serialize usertypes. When you use .set, we assume that things you pass -- even lambdas -- are meant as regular usertypes if they don't match the string/integer/float primitive or match being a table/container.

In MSVC, lambdas are given a unique name. And those unique names are accessible through __FUNCDSIG__, the VC++-specific macro for getting the function name of a signature. We do the same with GCC's __PRETTY_FUNCTION__. This works well, except in gcc in the case of their lambdas. They serialize lambda names in __PRETTY_FUNCTION__ as "lambda(arg_types...)". This means that different lambdas with the same arguments look identical to the implementation.

"Well, hold on, why don't you just use RTTI and typeid(X) to save you here?" The short answer is that we can't, a lot of people turn off RTTI and have complained about RTTI being on. The long answer is that it still doesn't actually solve our problem. When you want to get the metatable that controls things like. e.g., call-operator overloading in Lua, you need a string. There's 3-4 problems with typeid() and its friends type_index/type_info in C++: type_name() is not guaranteed to be unique to a type, hash_code() is (obviously) not unique, and the address of type_info objects return by typeid are not even guaranteed to be unique in the same translation unit, let alone across linked-in DLLs and what not.

Finally, it is impossible to instantiate a template in 1 DLL, and then another DLL (or exec), and have it point to the same type_info information. And even if we compare the type_info structures themselves, there's no guarantee it will compare equal...

... Welcome to no-static-reflection hell, I guess...?

@ThePhD
Copy link
Owner

ThePhD commented Mar 14, 2018

I added a note to the docs that not only points to this issue but also explains that using set_function gets around this limitation. Buyer Beware, I guess.

Sorry for the trouble, there's not much else I can do to make this work out without forcing RTTI on everyone.

@ThePhD ThePhD closed this as completed Mar 14, 2018
@knorrest
Copy link
Author

No problem, thanks for the explanation!

cuavas added a commit to mamedev/mame that referenced this issue Mar 15, 2022
Made the debugger memory view not depend on isprint which is affected by
the global locale.  Assume the OSD will display as ISO-8869-1 and
replace problematic printable characters.

Started changing Lua function bindings to use set_function to avoid
potential issues related to ThePhD/sol2#608, and worked out what was
causing problems with symbol table read_memory/write_memory.  (They
aren't really essential - you can do the same thing with the address
space object itself, but they're easier to parameterise.)
cuavas added a commit to mamedev/mame that referenced this issue Mar 23, 2022
Made auto-boot script errors and plugin bootstrap errors fatal.

Run auto-boot scripts in a sandbox.  Globals can be accessed, but not
set.  The sandbox is cleared on hard reset, but not on soft reset.

Added (hopefully) useful to string metafunctions to device_t and
address space that show short names and tags.

Fixed issues in plugins that surface when strict type checking is
enabled, as this means numbers and nil are not automatically converted
to strings.  Plugins should be tested with debug builds to check for
this.

Made save item read_block raise an error on invalid arguments rather
than returning an empty string, and made it use luaL_buffer directly
rather than using the helper wrapper.

Changed some more function bindings to use set_function to avoid issues
related to ThePhD/sol2#608, and got rid of some unnecessary lambda
captures.
wilbertpol pushed a commit to wilbertpol/mame that referenced this issue Apr 22, 2022
Made the debugger memory view not depend on isprint which is affected by
the global locale.  Assume the OSD will display as ISO-8869-1 and
replace problematic printable characters.

Started changing Lua function bindings to use set_function to avoid
potential issues related to ThePhD/sol2#608, and worked out what was
causing problems with symbol table read_memory/write_memory.  (They
aren't really essential - you can do the same thing with the address
space object itself, but they're easier to parameterise.)
wilbertpol pushed a commit to wilbertpol/mame that referenced this issue Apr 22, 2022
Made auto-boot script errors and plugin bootstrap errors fatal.

Run auto-boot scripts in a sandbox.  Globals can be accessed, but not
set.  The sandbox is cleared on hard reset, but not on soft reset.

Added (hopefully) useful to string metafunctions to device_t and
address space that show short names and tags.

Fixed issues in plugins that surface when strict type checking is
enabled, as this means numbers and nil are not automatically converted
to strings.  Plugins should be tested with debug builds to check for
this.

Made save item read_block raise an error on invalid arguments rather
than returning an empty string, and made it use luaL_buffer directly
rather than using the helper wrapper.

Changed some more function bindings to use set_function to avoid issues
related to ThePhD/sol2#608, and got rid of some unnecessary lambda
captures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants