emscripten-core / emscripten-core/emscripten
"[emval] Prevent creating lvalue refs from thin air (#24606)" breaks existing code
- Dominant language
- C++
- Stars
- 27.6k
- Forks
- 3.6k
- Avg merge
- 1d 1h
- Merged PRs (30d)
- 105
Description
"[emval] Prevent creating lvalue refs from thin air (#24606)" in Emscripten 4.0.11 started to break compilation of the following test code
```
#include
#include
#include
namespace {
struct Obj {
Obj() { ++count; }
Obj(Obj const&) { ++count; }
~Obj() { --count; }
static unsigned count;
};
unsigned Obj::count = 0;
struct Interface {
virtual ~Interface() {}
virtual Obj get() const = 0;
};
struct Wrapper: emscripten::wrapper {
EMSCRIPTEN_WRAPPER(Wrapper);
Obj get() const override {
Obj const & orig = call("get");
Obj copy(orig);
delete &orig;
return copy;
}
};
}
int main() {
emscripten::class_("Obj").constructor<>();
emscripten::class_("Interface")
.allow_subclass("Wrapper")
.function("get", &Interface::get, emscripten::pure_virtual());
EM_ASM(
globalThis.Impl = Module.Interface.implement({
get: () => new Module.Obj
});
);
emscripten::val::global("Impl").as(emscripten::allow_raw_pointers())->get();
assert(Obj::count == 0);
}
```
with
```
In file included from /home/sberg/github.com/emscripten-core/emscripten/test/embind/test_wrapper_return_by_value.cpp:4:
In file included from /home/sberg/github.com/emscripten-core/emscripten/cache/sysroot/include/emscripten/bind.h:26:
/home/sberg/github.com/emscripten-core/emscripten/cache/sysroot/include/emscripten/val.h:637:19: error:
static assertion failed due to requirement '!std::is_lvalue_reference::value': Cannot create a lvalue reference out of a JS value.
637 | static_assert(!std::is_lvalue_reference::value,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/sberg/github.com/emscripten-core/emscripten/cache/sysroot/include/emscripten/val.h:532:12: note:
in instantiation of function template specialization
'emscripten::val::internalCall, const (anonymous namespace)::Obj &>' requested here
532 | return internalCall, ReturnValue>(as_handle(), name, st...
| ^
/home/sberg/github.com/emscripten-core/emscripten/cache/sysroot/include/emscripten/bind.h:1277:24: note:
in instantiation of function template specialization 'emscripten::val::call' requested here
1277 | return wrapped.call(name, std::forward(args)...);
| ^
/home/sberg/github.com/emscripten-core/emscripten/test/embind/test_wrapper_return_by_value.cpp:28:24: note:
in instantiation of function template specialization 'emscripten::wrapper<(anonymous
namespace)::Interface>::call' requested here
28 | Obj const & orig = call("get");
| ^
1 error generated.
```
The reason for the `orig`/`copy` dance in `Wrapper::get` and for using `Obj const&` instead of `Obj const*` there is as per the commit message of "Embind: Fix lifecycle of UNO any and sequence values returned from JS to C++":
> When a JS function implementing a [wrapped] interface method returns [`Obj`], it could not create a new instance of that type and return it, as it would have needed to call `.delete()` on that instance, but couldn't.
> So [do the following instead]: If a[n] interface method returns [`Obj`] (i.e., a type on which `.delete()` must be called), change the JS implemen[t]ation's return type from by-value (which meant that the C++ code received a copy) to by-reference---which means that now the C++ code can access the original instance and delete it. But which also means that the JS code must always return a fresh instance now!
> (Ideally, the [...] code would use by-pointer rather than by-reference for that return type, but that caused
> ```
> emsdk/upstream/emscripten/cache/sysroot/include/emscripten/wire.h:116:19: error: static assertion failed due to requirement '!std::is_pointer::value': Implicitly binding raw pointers is illegal. Specify allow_raw_pointer>
> 116 | static_assert(!std::is_pointer::value, "Implicitly binding raw pointers is illegal. Specify allow_raw_pointer>");
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> ```
> errors with no obvious place where to put such `allow_raw_pointer` markers, so lets go with this little hack at least for now.)
Contributor guide
Assessment
This issue has not been assessed yet.