Skip to content

Commit

Permalink
embind - Fix reading various bindings types from val on wasm64. (#21225)
Browse files Browse the repository at this point in the history
- The function simpleReadValueFromPointer was always reading pointers as
i32 numbers which was incorrect on wasm64.
- Decoding of std::wstring was using a right shift to read pointers. Right
  shift was truncating numbers larger than i32 and causing misreads.
- Added tests to execute readValueFromPointer on all the binding types
  to ensure these don't break on wasm64.
  • Loading branch information
brendandahl authored Feb 1, 2024
1 parent f6a7e5f commit 1fc7d9d
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 26 deletions.
35 changes: 13 additions & 22 deletions src/embind/embind.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
// -- jshint doesn't understand library syntax, so we need to specifically tell it about the symbols we define
/*global typeDependencies, flushPendingDeletes, getTypeName, getBasestPointer, throwBindingError, UnboundTypeError, embindRepr, registeredInstances, registeredTypes*/
/*global ensureOverloadTable, embind__requireFunction, awaitingDependencies, makeLegalFunctionName, embind_charCodes:true, registerType, createNamedFunction, RegisteredPointer, throwInternalError*/
/*global simpleReadValueFromPointer, floatReadValueFromPointer, integerReadValueFromPointer, enumReadValueFromPointer, replacePublicSymbol, craftInvokerFunction, tupleRegistrations*/
/*global floatReadValueFromPointer, integerReadValueFromPointer, enumReadValueFromPointer, replacePublicSymbol, craftInvokerFunction, tupleRegistrations*/
/*global finalizationRegistry, attachFinalizer, detachFinalizer, releaseClassHandle, runDestructor*/
/*global ClassHandle, makeClassHandle, structRegistrations, whenDependentTypesAreResolved, BindingError, deletionQueue, delayFunction:true, upcastPointer*/
/*global exposePublicSymbol, heap32VectorToArray, newFunc, char_0, char_9*/
Expand All @@ -41,7 +41,7 @@ var LibraryEmbind = {
// If register_type is used, emval will be registered multiple times for
// different type id's, but only a single type object is needed on the JS side
// for all of them. Store the type for reuse.
$EmValType__deps: ['_emval_decref', '$Emval', '$simpleReadValueFromPointer', '$GenericWireTypeSize'],
$EmValType__deps: ['_emval_decref', '$Emval', '$readPointer', '$GenericWireTypeSize'],
$EmValType: `{
name: 'emscripten::val',
'fromWireType': (handle) => {
Expand All @@ -51,7 +51,7 @@ var LibraryEmbind = {
},
'toWireType': (destructors, value) => Emval.toHandle(value),
'argPackAdvance': GenericWireTypeSize,
'readValueFromPointer': simpleReadValueFromPointer,
'readValueFromPointer': readPointer,
destructorFunction: null, // This type does not need a destructor
// TODO: do we need a deleteObject here? write a test where
Expand Down Expand Up @@ -494,12 +494,6 @@ var LibraryEmbind = {
});
},

$simpleReadValueFromPointer__docs: '/** @suppress {globalThis} */',
// For types whose wire types are 32-bit pointers.
$simpleReadValueFromPointer: function(pointer) {
return this['fromWireType']({{{ makeGetValue('pointer', '0', 'i32') }}});
},

$readPointer__docs: '/** @suppress {globalThis} */',
$readPointer: function(pointer) {
return this['fromWireType']({{{ makeGetValue('pointer', '0', '*') }}});
Expand Down Expand Up @@ -617,33 +611,30 @@ var LibraryEmbind = {
],
_embind_register_std_wstring: (rawType, charSize, name) => {
name = readLatin1String(name);
var decodeString, encodeString, getHeap, lengthBytesUTF, shift;
var decodeString, encodeString, readCharAt, lengthBytesUTF;
if (charSize === 2) {
decodeString = UTF16ToString;
encodeString = stringToUTF16;
lengthBytesUTF = lengthBytesUTF16;
getHeap = () => HEAPU16;
shift = 1;
readCharAt = (pointer) => {{{ makeGetValue('pointer', 0, 'u16') }}};
} else if (charSize === 4) {
decodeString = UTF32ToString;
encodeString = stringToUTF32;
lengthBytesUTF = lengthBytesUTF32;
getHeap = () => HEAPU32;
shift = 2;
readCharAt = (pointer) => {{{ makeGetValue('pointer', 0, 'u32') }}};
}
registerType(rawType, {
name,
'fromWireType': (value) => {
// Code mostly taken from _embind_register_std_string fromWireType
var length = {{{ makeGetValue('value', 0, '*') }}};
var HEAP = getHeap();
var str;

var decodeStartPtr = value + {{{ POINTER_SIZE }}};
// Looping here to support possible embedded '0' bytes
for (var i = 0; i <= length; ++i) {
var currentBytePtr = value + {{{ POINTER_SIZE }}} + i * charSize;
if (i == length || HEAP[currentBytePtr >> shift] == 0) {
if (i == length || readCharAt(currentBytePtr) == 0) {
var maxReadBytes = currentBytePtr - decodeStartPtr;
var stringSegment = decodeString(decodeStartPtr, maxReadBytes);
if (str === undefined) {
Expand All @@ -668,7 +659,7 @@ var LibraryEmbind = {
// assumes POINTER_SIZE alignment
var length = lengthBytesUTF(value);
var ptr = _malloc({{{ POINTER_SIZE }}} + length + charSize);
{{{ makeSetValue('ptr', '0', 'length >> shift', SIZE_TYPE) }}};
{{{ makeSetValue('ptr', '0', 'length / charSize', SIZE_TYPE) }}};

encodeString(value, ptr + {{{ POINTER_SIZE }}}, length + charSize);

Expand All @@ -678,7 +669,7 @@ var LibraryEmbind = {
return ptr;
},
'argPackAdvance': GenericWireTypeSize,
'readValueFromPointer': simpleReadValueFromPointer,
'readValueFromPointer': readPointer,
destructorFunction(ptr) {
_free(ptr);
}
Expand Down Expand Up @@ -1012,7 +1003,7 @@ var LibraryEmbind = {

_embind_finalize_value_array__deps: [
'$tupleRegistrations', '$runDestructors',
'$simpleReadValueFromPointer', '$whenDependentTypesAreResolved'],
'$readPointer', '$whenDependentTypesAreResolved'],
_embind_finalize_value_array: (rawTupleType) => {
var reg = tupleRegistrations[rawTupleType];
delete tupleRegistrations[rawTupleType];
Expand Down Expand Up @@ -1064,7 +1055,7 @@ var LibraryEmbind = {
return ptr;
},
'argPackAdvance': GenericWireTypeSize,
'readValueFromPointer': simpleReadValueFromPointer,
'readValueFromPointer': readPointer,
destructorFunction: rawDestructor,
}];
});
Expand Down Expand Up @@ -1115,7 +1106,7 @@ var LibraryEmbind = {

_embind_finalize_value_object__deps: [
'$structRegistrations', '$runDestructors',
'$simpleReadValueFromPointer', '$whenDependentTypesAreResolved'],
'$readPointer', '$whenDependentTypesAreResolved'],
_embind_finalize_value_object: (structType) => {
var reg = structRegistrations[structType];
delete structRegistrations[structType];
Expand Down Expand Up @@ -1173,7 +1164,7 @@ var LibraryEmbind = {
return ptr;
},
'argPackAdvance': GenericWireTypeSize,
'readValueFromPointer': simpleReadValueFromPointer,
'readValueFromPointer': readPointer,
destructorFunction: rawDestructor,
}];
});
Expand Down
8 changes: 4 additions & 4 deletions test/code_size/embind_val_wasm.json
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"a.html": 673,
"a.html.gz": 431,
"a.js": 7387,
"a.js.gz": 3112,
"a.js": 7325,
"a.js.gz": 3088,
"a.wasm": 11433,
"a.wasm.gz": 5725,
"total": 19493,
"total_gz": 9268
"total": 19431,
"total_gz": 9244
}
81 changes: 81 additions & 0 deletions test/embind/test_val_read_pointer.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
#include <emscripten/bind.h>
#include <emscripten.h>

using namespace emscripten;

struct ValueObject {
ValueObject(): value(43) {};
int value;
};

struct ValueArray {
ValueArray(): x(44) {};
int x;
};

struct Foo {
Foo(): value(45) {};
int value;
};

enum Enum { ONE, TWO };

EMSCRIPTEN_BINDINGS(xxx) {
value_object<ValueObject>("ValueObject")
.field("value", &ValueObject::value);

value_array<ValueArray>("ValueArray")
.element(&ValueArray::x);

class_<Foo>("Foo")
.property("value", &Foo::value);

enum_<Enum>("Enum")
.value("ONE", ONE)
.value("TWO", TWO);
}


int main() {
EM_ASM(
globalThis["passthrough"] = (arg) => {
return arg;
};
globalThis["passthroughValueObject"] = (arg) => {
return arg.value;
};
globalThis["passthroughValueArray"] = (arg) => {
return arg[0];
};
globalThis["passthroughClass"] = (arg) => {
const value = arg.value;
arg.delete();
return value;
};
globalThis["passthroughMemoryView"] = (arg) => {
return arg[2];
};
);

// These tests execute all the various readValueFromPointer functions for each
// of the different binding types.
assert(val::global().call<bool>("passthrough", true));
assert(val::global().call<int>("passthrough", 42) == 42);
assert(val::global().call<double>("passthrough", 42.2) == 42.2);
ValueObject vo;
assert(val::global().call<int>("passthroughValueObject", vo) == 43);
ValueArray va;
assert(val::global().call<int>("passthroughValueArray", va) == 44);
Foo foo;
assert(val::global().call<int>("passthroughClass", foo) == 45);
assert(val::global().call<std::string>("passthrough", val("abc")) == "abc");
std::string str = "hello";
assert(val::global().call<std::string>("passthrough", str) == "hello");
std::wstring wstr = L"abc";
assert(val::global().call<std::wstring>("passthrough", wstr) == L"abc");
static const int data[] = {0, 1, 2};
assert(val::global().call<int>("passthroughMemoryView", typed_memory_view(3, data)) == 2);
assert(val::global().call<Enum>("passthrough", ONE) == ONE);

return 0;
}
4 changes: 4 additions & 0 deletions test/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -7471,6 +7471,10 @@ def test_embind_val(self):
self.emcc_args += ['-lembind']
self.do_run_in_out_file_test('embind/test_val.cpp')

def test_embind_val_read_pointer(self):
self.emcc_args += ['-lembind']
self.do_runf('embind/test_val_read_pointer.cpp')

def test_embind_val_assignment(self):
err = self.expect_fail([EMCC, test_file('embind/test_val_assignment.cpp'), '-lembind', '-c'])
self.assertContained('candidate function not viable: expects an lvalue for object argument', err)
Expand Down

0 comments on commit 1fc7d9d

Please sign in to comment.