1246403 - [wasm] Hit MOZ_CRASH(Float32 shouldn't be returned from a FFI) at js/src/asmjs/WasmStubs.cpp:411

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The attached binary WebAssembly testcase crashes on mozilla-central revision 1dbe350b57b1+ (build with --enable-gczeal --enable-optimize --enable-debug --enable-address-sanitizer --without-intl-api --enable-posix-nspr-emulation --disable-jemalloc --disable-tests --enable-debug, run with ). To reproduce, you can run the following code in the JS shell:

var data = os.file.readFile(file, 'binary');
wasmEval(data.buffer);


Backtrace:

==4474==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000024ffd7f bp 0x7fffebd27d50 sp 0x7fffebd26740 T0)
    #0 0x24ffd7e in GenerateInterpExitStub(js::wasm::ModuleGenerator&, unsigned int, js::jit::Label*, js::wasm::ProfilingOffsets*) js/src/asmjs/WasmStubs.cpp:409:9
    #1 0x24ffd7e in js::wasm::GenerateStubs(js::wasm::ModuleGenerator&, bool) js/src/asmjs/WasmStubs.cpp:1089
    #2 0x2445820 in js::wasm::ModuleGenerator::finish(mozilla::Vector<js::wasm::CacheableChars, 0ul, js::SystemAllocPolicy>&&, mozilla::UniquePtr<js::wasm::ModuleData, JS::DeletePolicy<js::wasm::ModuleData> >*, mozilla::UniquePtr<js::wasm::StaticLinkData, JS::DeletePolicy<js::wasm::StaticLinkData> >*, mozilla::UniquePtr<js::wasm::ExportMap, JS::DeletePolicy<js::wasm::ExportMap> >*, mozilla::Vector<js::wasm::SlowFunction, 0ul, js::TempAllocPolicy>*) js/src/asmjs/WasmGenerator.cpp:660:10
    #3 0x241d8e4 in DecodeModule(JSContext*, mozilla::UniquePtr<char [], JS::FreePolicy>, unsigned char const*, unsigned int, mozilla::Vector<ImportName, 0ul, js::SystemAllocPolicy>*, mozilla::UniquePtr<js::wasm::ExportMap, JS::DeletePolicy<js::wasm::ExportMap> >*, JS::MutableHandle<js::ArrayBufferObject*>, JS::MutableHandle<js::WasmModuleObject*>) js/src/asmjs/Wasm.cpp:1032:10
    #4 0x2412690 in WasmEval(JSContext*, unsigned int, JS::Value*) js/src/asmjs/Wasm.cpp:1171:10
    #5 0x1a5cad7 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:235:15
[...]
    #16 0x53b760 in Shell(JSContext*, js::cli::OptionParser*, char**) js/src/shell/js.cpp:6689
    #17 0x53b760 in main js/src/shell/js.cpp:7056
    #18 0x7fc2e771aec4  (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)
    #19 0x489c10 in _start (js/src/debug64afl/js/src/shell/js+0x489c10)

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: forbid-f32.patch

The module imports a function that returns a f32. In ye olde asm.js times, one could not do so, for a reason I can't remember. Luke, will wasm be able to import functions that return a float32? I don't see anything in the spec that would forbid this, but just in case, here's a patch that does the forbidding.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

See previous comment.

Comment 4

[Luke Wagner [:luke]]

wasm should indeed allow importing/exporting f32.  (Only i64 will be disallowed in JS exports until JS has a full-fidelity int64.)  If you could write a patch to add f32 import support, that'd be great.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: return-float32-from-ffi.patch

Here we go. We can't be very smart here, as the JS engine isn't used to work with Float32 (JS)Values, so this patch does the dumb thing: apply what's done for double and then convert back into a float32.

This means that even if an ion-compiled function can return a float32, we'll have a ToFloat32(ToDouble(x)) instructions series somewhere in the code, but as long as Ion can't inline asm.js code, we can't do any better, I think.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8716992 [details] [diff] [review]
return-float32-from-ffi.patch

Review of attachment 8716992 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: js/src/asmjs/WasmStubs.cpp
@@ +408,5 @@
>        case ExprType::I64:
>          MOZ_CRASH("no int64 in asm.js");
>        case ExprType::F32:
> +        MOZ_ASSERT(!mg.isAsmJS(), "import can't return float32 in asm.js");
> +        masm.call(SymbolicAddress::InvokeImport_F64);

Does it make sense (for symmetry, if nothing else) to add a new InvokeImport_F32?

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the review!

(In reply to Luke Wagner [:luke] from comment #6)
> Comment on attachment 8716992 [details] [diff] [review]
> return-float32-from-ffi.patch
> 
> Review of attachment 8716992 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> ::: js/src/asmjs/WasmStubs.cpp
> @@ +408,5 @@
> >        case ExprType::I64:
> >          MOZ_CRASH("no int64 in asm.js");
> >        case ExprType::F32:
> > +        MOZ_ASSERT(!mg.isAsmJS(), "import can't return float32 in asm.js");
> > +        masm.call(SymbolicAddress::InvokeImport_F64);
> 
> Does it make sense (for symmetry, if nothing else) to add a new
> InvokeImport_F32?

That's how I've started, but really InvokeImport_F32 was really just calling InvokeImport_F64 and converting the return value stored in argv[0]. The differences between the twos stands in one line: the masm.convertDoubleToFloat32, which is explicited if we have InvokeImport_F32, but that's it, everything else is the same. I can add it if you are strongly wanting it, though.

Comment 8

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
Well it's really just the symmetry.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e661e47254a

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

I've tried doing the symmetric way: InvokeImport_F32 would call InvokeImport_F64 first, then do:

argv[0] = Float32Value(float(argv[0].toDouble()));

But then, when we're back in assembly, argv[0] still is a Value, hence still a Double! As a matter of fact, we still need the masm.convertDoubleToFloat32. It seems a bit sad to have to perform the double->float32 conversion twice just for the sake of symmetry, so I've kept it the way it was in the original patch.

Comment 11

[Luke Wagner [:luke]]

Makes sense

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e661e47254a