Workaround for floating point arguments and return values in `DynamicFunc`s. #1283

losfair · 2020-03-09T19:20:36Z

This PR makes floating point arguments and return values for DynamicFuncs work correctly in all three backends.

Previously Singlepass used integer registers for all arguments. This PR adds another thin trampoline layer just before control is transferred to the import function, so that arguments will be rearranged strictly according to the System V ABI.

The full fix would require singlepass to implement the SysV calling convention internally too: #1271 . This is just a workaround.

…ines.

…t-functions' into fix/fpcc-workaround

losfair · 2020-03-09T19:36:28Z

bors try

bors · 2020-03-09T20:02:02Z

try

Build failed

wasmerio.wasmer

nlewycky · 2020-03-09T19:56:46Z

lib/runtime-core/src/parse.rs

    let mut mcg_info_fed = false;

    loop {
        use wasmparser::ParserState;
        let state = parser.read();
+
+        // Feed signature and namespace information as early as possible.


I'm confused about this change? These would have the same behaviour if they were cases in the match *state that follows, right? If this is about code reuse, maybe create a closure?

Maybe it makes sense to use the Wasmparser types that let us be more specific about what exactly we want: https://docs.rs/wasmparser/0.51.4/wasmparser/struct.CodeSectionReader.html . They have a bunch of section-specific parsers, there aren't validating versions of these parsers though, so we'd have to do a validation pass first

Since this block borrows mcg, there would be lifetime issues if this is made into a closure. And yeah this is about code reuse. The same code was already duplicated twice for BeginFunctionBody and EndWasm, and this change will add one more duplication if done "in place".

mcg could be passed as an argument here to make closures work

lib/runtime-core/src/state.rs

nlewycky · 2020-03-09T20:02:08Z

lib/runtime-core/src/trampoline_x64.rs

@@ -246,44 +248,49 @@ impl TrampolineBufferBuilder {
        &mut self,
        target: unsafe extern "C" fn(*const CallContext, *const u64) -> u64,
        context: *const CallContext,
-        num_params: u32,
+        params: &[Type],


Any reason this shouldn't just take the full function signature? As it happens it only used to need the number of parameters, now it needs the whole list of the types, but isn't that an implementation detail?

This function does not prepend the WebAssembly-specific vm::Ctx argument and is just a general-purpose trampoline generator, therefore should not take a WebAssembly FuncSig. I added a returns argument just in case of future compatibility.

nlewycky · 2020-03-09T20:04:10Z

lib/runtime-core/src/trampoline_x64.rs

+        unsafe extern "C" fn inner(n: *const CallContext, args: *const u64) -> u64 {
+            let n = n as usize;
+            let mut result: u64 = 0;
+            for i in 0..n {


Sorry I'm confused – n was a pointer to a CallContext, so if it's at address 0x00005620d48999e8 then this loop will run 94699004729832 times?

The *const CallContext is casted from the length of the parameter list as an integer in the following call to add_callinfo_trampoline, so it's not really a pointer.

Will add a comment.

OK, but you can't just go around passing integers as pointers. The compiler makes the assumption that the pointer is sufficiently aligned to point to a CallContext, which could lead it to conclude that the bottom bits are zero. All you do here is use it as a usize which will cast it, but if you were to pass, say, a bitfield through, the compiler would actually delete the tests of the bottom bits.

I don't know about Rust, but in C and C++ the rule is that all pointers must actually point to a valid object when created (whenever an expression of pointer type is evaluated). You're guaranteed that casting a pointer to an appropriately sized int and back to the same pointer type gets you the original pointer back, but that's a different property from being able to take an int, cast it to a pointer, and put it back to an int. For a crazy example that no compiler I know of does, if there's a value in the range [0, 4) the compiler could pack it in the same storage as the int.

If this is a temporary part of the workaround that will go away with the permanent fix, let me know. Casting this way doesn't seem to go wrong in practice, much.

This is part of the test code and does not go into the compiled crate, so the worst case is one more failing test and we know something is wrong here.

Oh no, that's never the worst case. :-) The worst case is something like "not only does this test pass when it should actually fail, it causes the other tests running in other threads to incorrectly pass when they should be failing". Anyway, I don't think we need to change this right now.

lib/runtime-core/src/typed_func.rs

lib/singlepass-backend/src/codegen_x64.rs

MarkMcCaskey · 2020-03-09T21:09:11Z

lib/runtime-core/src/parse.rs

+        }
+        match *state {
+            ParserState::BeginFunctionBody { .. } | ParserState::EndWasm => {
+                if !mcg_info_fed {


If we use separate match statements, moving the if !mcg_*_fed { checks outside the match *state will probably have better performance due to better branch prediction.

I think this is a kind of simple optimization that can be left to the compiler?

Depends whether the compiler can prove *state is side-effect free. I'm sorta torn on this one, I think we should write whatever is clearest to the reader, but I'm not sure that pulling this part of the match out in advance is actually clearer. I'm happy to let you and Mark decide.

Will make it into a closure.

MarkMcCaskey · 2020-03-09T21:13:26Z

lib/runtime-core/src/parse.rs

    let mut mcg_info_fed = false;

    loop {
        use wasmparser::ParserState;
        let state = parser.read();
+
+        // Feed signature and namespace information as early as possible.


Maybe it makes sense to use the Wasmparser types that let us be more specific about what exactly we want: https://docs.rs/wasmparser/0.51.4/wasmparser/struct.CodeSectionReader.html . They have a bunch of section-specific parsers, there aren't validating versions of these parsers though, so we'd have to do a validation pass first

lib/spectests/tests/spectest.rs

lib/runtime-core-tests/tests/imports.rs

lib/runtime-core/src/trampoline_x64.rs

Co-Authored-By: nlewycky <nick@wasmer.io>

losfair · 2020-03-10T04:39:16Z

bors try

bors · 2020-03-10T05:06:46Z

try

Build succeeded

…cc-workaround

syrusakbary · 2020-03-12T04:51:56Z

bors r+

bors · 2020-03-12T05:17:51Z

Build succeeded

wasmerio.wasmer
continuous-integration/travis-ci/push

Hywan and others added 8 commits March 5, 2020 14:17

test(runtime-core) Test polymorphic host functions with more types.

4d3e8ee

runtime-core: Correctly allocate floating point registers for trampol…

a5de17f

…ines.

runtime-core: Move ArgumentRegisterAllocator into state.

e62095d

Merge remote-tracking branch 'origin/master' into fix/fpcc-workaround

3ff3554

Prettier format panic message in spectest.

79613e4

Add a translation layer before import call in singlepass.

65962f0

Merge remote-tracking branch 'Hywan/test-runtime-core-polymorphic-hos…

fc114ac

…t-functions' into fix/fpcc-workaround

Fix floating point return values.

7e2ede3

losfair requested review from Hywan, MarkMcCaskey, nlewycky and syrusakbary as code owners March 9, 2020 19:20

bors bot added a commit that referenced this pull request Mar 9, 2020

Try #1283:

58e9d4f

nlewycky reviewed Mar 9, 2020

View reviewed changes

MarkMcCaskey reviewed Mar 9, 2020

View reviewed changes

losfair and others added 4 commits March 10, 2020 12:28

Resolve review comments.

7d2d89b

Deprecate wasmer_trampoline_buffer_builder_add_callinfo_trampoline.

3e63f1a

Fix typo (1)

cfbcd88

Co-Authored-By: nlewycky <nick@wasmer.io>

Fix typo (2)

e521dfe

Co-Authored-By: nlewycky <nick@wasmer.io>

bors bot added a commit that referenced this pull request Mar 10, 2020

Try #1283:

8182cbd

losfair added 4 commits March 12, 2020 02:51

Move feed_* into closures.

adabfa0

Merge remote-tracking branch 'origin/master' into fix/fpcc-workaround

fc9f1bc

Add #1283 into changelog.

f93561d

Merge remote-tracking branch 'origin/fix/fpcc-workaround' into fix/fp…

9930d54

…cc-workaround

nlewycky approved these changes Mar 11, 2020

View reviewed changes

Merge branch 'master' into fix/fpcc-workaround

7617350

syrusakbary approved these changes Mar 12, 2020

View reviewed changes

bors bot merged commit 18168fc into master Mar 12, 2020

bors bot deleted the fix/fpcc-workaround branch March 12, 2020 05:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workaround for floating point arguments and return values in `DynamicFunc`s. #1283

Workaround for floating point arguments and return values in `DynamicFunc`s. #1283

losfair commented Mar 9, 2020

losfair commented Mar 9, 2020

bors bot commented Mar 9, 2020

nlewycky Mar 9, 2020

MarkMcCaskey Mar 9, 2020

losfair Mar 10, 2020

MarkMcCaskey Mar 10, 2020

nlewycky Mar 9, 2020

losfair Mar 10, 2020

nlewycky Mar 9, 2020

losfair Mar 10, 2020

losfair Mar 10, 2020

nlewycky Mar 11, 2020

losfair Mar 11, 2020

nlewycky Mar 11, 2020

MarkMcCaskey Mar 9, 2020

losfair Mar 10, 2020

nlewycky Mar 11, 2020 •

edited

Loading

losfair Mar 11, 2020

MarkMcCaskey Mar 9, 2020

losfair commented Mar 10, 2020

bors bot commented Mar 10, 2020

syrusakbary commented Mar 12, 2020

bors bot commented Mar 12, 2020

Workaround for floating point arguments and return values in DynamicFuncs. #1283

Workaround for floating point arguments and return values in DynamicFuncs. #1283

Conversation

losfair commented Mar 9, 2020

losfair commented Mar 9, 2020

bors bot commented Mar 9, 2020

try

Build failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlewycky Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losfair commented Mar 10, 2020

bors bot commented Mar 10, 2020

try

Build succeeded

syrusakbary commented Mar 12, 2020

bors bot commented Mar 12, 2020

Build succeeded

Workaround for floating point arguments and return values in `DynamicFunc`s. #1283

Workaround for floating point arguments and return values in `DynamicFunc`s. #1283

nlewycky Mar 11, 2020 •

edited

Loading