Fix tests to reflect new NaN semantics #414

rfk · 2017-01-28T11:00:52Z

Here's an initial attempt at #286, let me know if it looks like it's on the right track. AFAICT it works as-is, but I still plan to:

Think more about the naming here. Since asserting a canonical nan is the more common case, I wonder if assert_return_nan() should retain its existing semantics, and e.g. assert_return_nondet_nan could be added that allows any quiet NaN.
Add some additional tests such as those suggested in Implement the new NaN semantics #286 (comment)

rossberg · 2017-01-30T12:28:51Z

interpreter/host/js.ml

@@ -129,6 +129,14 @@ let harness =
  "    throw new Error(\"Wasm return value NaN expected, got \" + actual);\n" ^
  "  };\n" ^
  "}\n" ^
+  "function assert_return_canonical_nan(action) {\n" ^


Nit: Insert one empty line before this

rossberg · 2017-01-30T12:34:17Z

interpreter/host/run.ml

@@ -365,6 +365,18 @@ let run_assertion ass =
    trace ("Asserting return...");
    let got_vs = run_action act in
    let is_nan =
+      (* Accept any NaN that's one everywhere pos_nan is one *)
+      match got_vs with
+      | [Values.F32 got_f32] -> (Int32.logand (F32.to_bits got_f32) (F32.to_bits F32.pos_nan)) = (F32.to_bits F32.pos_nan)


Nit: break line at 80 characters. also, redundant parens around the args of =

rossberg · 2017-01-30T12:36:37Z

interpreter/host/run.ml

+    trace ("Asserting return...");
+    let got_vs = run_action act in
+    let is_nan =
+      (* Accept only the two canonical NaNs *)


Nit: instead of this comment, rename the variable to is_canoncial_nan

rossberg · 2017-01-30T12:47:08Z

interpreter/host/run.ml

+      (* Accept any NaN that's one everywhere pos_nan is one *)
+      match got_vs with
+      | [Values.F32 got_f32] -> (Int32.logand (F32.to_bits got_f32) (F32.to_bits F32.pos_nan)) = (F32.to_bits F32.pos_nan)
+      | [Values.F64 got_f64] -> (Int64.logand (F64.to_bits got_f64) (F64.to_bits F64.pos_nan)) = (F64.to_bits F64.pos_nan)


rossberg · 2017-01-30T12:52:08Z

interpreter/host/js.ml

-    [ GetLocal (var i) @@ at;
-      GetLocal (var i) @@ at;
-      Compare (eq_of t) @@ at;
+  let test t =


The previous version was intended to work correctly for multiple return values. I believe that dropping the auxiliary locals will consume them in the wrong order, inconsistent with ts. (Though instead of using locals you can probably reverse ts.)

rossberg · 2017-01-30T12:53:27Z

interpreter/host/js.ml

+      BrIf (0l @@ at) @@ at ]
+  in [], List.flatten (List.map test ts)
+
+let assert_return_canonical_nan ts at =


The only difference between the two functions is the initial mask AFAICS. How about making that a parameter?

rossberg

Interpreter changes look pretty good already. @sunfishcode and @gahaas may want to have a look at the test changes.

gahaas · 2017-01-30T14:16:39Z

I like it.

sunfishcode

The test changes look right to me.

sunfishcode · 2017-01-30T20:27:10Z

interpreter/README.md

@@ -276,6 +276,7 @@ action:
 assertion:
  ( assert_return <action> <expr>* )         ;; assert action has expected results
  ( assert_return_nan <action> )             ;; assert action results in NaN
+  ( assert_return_canonical_nan <action> )   ;; assert action results in NaN in canonical form


Perhaps change "NaN in canonical form" to "NaN in a canonical form" to emphasize that there are multiple canonical forms.

sunfishcode · 2017-01-30T20:33:31Z

interpreter/host/js.ml

+  "function assert_return_canonical_nan(action) {\n" ^
+  "  let actual = action();\n" ^
+  "  // Note that JS can't reliably distinguish different NaN values,\n" ^
+  "  // so there's no good way to test that it's a canonical NaN.\n" ^


This comment would now also be relevant in the assert_return_nan function above, since JS can't test that the most-significant bit of the fraction field is 1 either.

rossberg · 2017-01-31T07:10:19Z

Regarding naming, I tend to agree that assert_return_nan should retain its original meaning (provided we have any use for it), and the variant from this change should be named assert_return_quiet_nan for clarity.

sunfishcode · 2017-01-31T19:08:40Z

The current WebAssembly documentation currently avoids using the terms "quiet" or "signaling", because WebAssembly's "signaling" NaNs don't signal anything, and are only distinguishable from "quiet" NaNs in the semantics by their bit pattern. How about assert_return_arithmetic_nan?

We indeed don't have a use for an assert that accepts any NaN under the current semantics.

rfk · 2017-01-31T21:25:38Z

Thanks for the feedback all, I hope to find some more time to iterate this in the next couple of days.

rossberg · 2017-02-01T14:41:17Z

@sunfishcode, but 'quiet NaN' is the IEEE terminology for the values it is checking for. Does it get clearer if we invent our own name to mean the same thing?

sunfishcode · 2017-02-01T19:33:23Z

The current NaN rules are designed to accomodate signaling NaNs that actually signal, but I can't predict when this will be exercised. Until then, "WebAssembly doesn't care about signaling NaNs" is tempting as a sufficient and unambiguous mental model for most people, since there are some widespread misconceptions about what signaling NaN actually means.

Also, the use of "arithmetic" in this very specific sense is not my invention.

However, I don't feel strongly about it. My main interest here is in being consistent. If we're going to talk about "quiet" NaNs in the spec/design repos, we should probably define what it means somewhere and use it consistently.

rossberg · 2017-02-02T12:36:26Z

We might want to wait for the resolution of WebAssembly/design#976. Even the naming question may become moot then.

binji · 2017-02-24T00:04:18Z

Sounds like WebAssembly/design#976 was resolved, can this move forward?

rossberg · 2017-02-24T08:23:49Z

@binji, sgtm. Still needs comments to be addressed.

rfk · 2017-02-26T23:58:38Z

I'll try to revisit the PR sometime this week.

rfk · 2017-03-05T09:55:30Z

It looks like #417 has introduced the "arithmetic_nan" terminology, so I'm going to go ahead with:

assert_return_nan -> returns positive or negative canonical nan
assert_return_arithmetic_nan -> returns any acceptable nan bitpattern

rfk · 2017-03-05T10:54:37Z

OK, I've updated this and added some more tests for the behaviour of min/max.

rossberg · 2017-03-06T08:35:48Z

It looks like #417 <#417> has introduced the "arithmetic_nan" terminology, so I'm going to go ahead with: - assert_return_nan -> returns positive or negative canonical nan - assert_return_arithmetic_nan -> returns any acceptable nan bitpattern

Shouldn't it be the other way round?

rfk · 2017-03-07T03:33:22Z

Shouldn't it be the other way round?

It's entirely possible that I mis-understood the terminology here - @sunfishcode can you clarify?

sunfishcode · 2017-03-07T08:42:14Z

For assert_return_arithmetic_nan, saying it accepts "any acceptable nan bitpattern" is a little unclear, but the code in the PR looks right.

For assert_return_nan, this was named assert_return_canonical_nan in the earlier patch, and I think we wanted to keep that name. That would leave us with nothing named assert_return_nan, but I think that's what we want, given how the language now works.

rfk · 2017-03-07T18:59:01Z

this was named assert_return_canonical_nan in the earlier patch, and I think we wanted to keep that name

OK, sounds good, I'll update to match.

rfk · 2017-03-19T01:47:58Z

Updated based on naming feedback above.

sunfishcode

Except for a few minor fixes that I just noticed, this PR now looks good to me.

sunfishcode · 2017-03-21T06:25:26Z

test/core/float_exprs.wast

 (assert_return (invoke "f64.no_fold_gt_select" (f64.const nan) (f64.const 0.0)) (f64.const 0.0))
 (assert_return (invoke "f64.no_fold_gt_select" (f64.const 0.0) (f64.const -0.0)) (f64.const -0.0))
 (assert_return (invoke "f64.no_fold_gt_select" (f64.const -0.0) (f64.const 0.0)) (f64.const 0.0))
-(assert_return (invoke "f64.no_fold_ge_select" (f64.const 0.0) (f64.const nan)) (f64.const nan))
+(assert_return_canonical_nan (invoke "f64.no_fold_ge_select" (f64.const 0.0) (f64.const nan)))


Sorry for not noticing this earlier, but all these tests involving select can actually use plain assert_return to test for a specific bit-pattern, because select is defined to return one of its operands verbatim.

sunfishcode · 2017-03-21T06:27:19Z

test/core/float_exprs.wast

 (assert_return (invoke "f64.no_fold_gt_if" (f64.const nan) (f64.const 0.0)) (f64.const 0.0))
 (assert_return (invoke "f64.no_fold_gt_if" (f64.const 0.0) (f64.const -0.0)) (f64.const -0.0))
 (assert_return (invoke "f64.no_fold_gt_if" (f64.const -0.0) (f64.const 0.0)) (f64.const 0.0))
-(assert_return (invoke "f64.no_fold_ge_if" (f64.const 0.0) (f64.const nan)) (f64.const nan))
+(assert_return_canonical_nan (invoke "f64.no_fold_ge_if" (f64.const 0.0) (f64.const nan)))


Similarly, these tests involving if can use plain assert_return because if returns its value verbatim.

sunfishcode · 2017-03-21T06:27:45Z

test/core/float_exprs.wast

 (assert_return (invoke "f64.no_fold_gt_select_to_abs" (f64.const 0.0)) (f64.const -0.0))
 (assert_return (invoke "f64.no_fold_gt_select_to_abs" (f64.const -0.0)) (f64.const 0.0))
 (assert_return (invoke "f64.no_fold_ge_select_to_abs" (f64.const nan:0x4000000000000)) (f64.const -nan:0x4000000000000))
-(assert_return (invoke "f64.no_fold_ge_select_to_abs" (f64.const -nan)) (f64.const nan))
+(assert_return_canonical_nan (invoke "f64.no_fold_ge_select_to_abs" (f64.const -nan)))


Same as above for these select tests.

sunfishcode · 2017-03-21T06:27:56Z

test/core/float_exprs.wast

 (assert_return (invoke "f64.no_fold_gt_if_to_abs" (f64.const 0.0)) (f64.const -0.0))
 (assert_return (invoke "f64.no_fold_gt_if_to_abs" (f64.const -0.0)) (f64.const 0.0))
 (assert_return (invoke "f64.no_fold_ge_if_to_abs" (f64.const nan:0x4000000000000)) (f64.const -nan:0x4000000000000))
-(assert_return (invoke "f64.no_fold_ge_if_to_abs" (f64.const -nan)) (f64.const nan))
+(assert_return_canonical_nan (invoke "f64.no_fold_ge_if_to_abs" (f64.const -nan)))


Same as above for these if tests.

rfk · 2017-03-21T06:46:57Z

Thanks @sunfishcode, I've reverted the testcases you identified.

sunfishcode · 2017-03-22T16:25:00Z

Looks good to me, and the main changes have general approval above. Thanks for working on this @rfk!

sunfishcode · 2017-03-22T16:30:08Z

@rfk Have you joined the WebAssembly W3C Community Group? https://www.w3.org/community/webassembly/

rfk · 2017-03-23T05:42:09Z

Have you joined the WebAssembly W3C Community Group?

Hmm, I thought I had, but I don't see myself in the list of participants there. I'll try again now...

rfk · 2017-03-23T21:54:07Z

OK, I'm now successfully showing up in https://www.w3.org/community/webassembly/participants

sunfishcode · 2017-03-23T21:56:40Z

Thanks!

https://bugs.webkit.org/show_bug.cgi?id=173287 <rdar://problem/32725975> Reviewed by Saam Barati. Import spec tests as of 31c641cc15f2aedbec2fa45a5185f68416df578b, with a few modifications so things work. Fix a bunch of bugs found through this process, and punt a few tests (which I marked as blocked by this bug). Fixes: Fix load / store alignment: r216908 erroneously implemented it as bit alignment instead of byte alignment. It was also missing memory-alignment.js despite it being in the ChangeLog, so add it too. This allows spec-test/align.wast.js to pass. Tables can be imported or in a section. There can be only one, but sections can be empty. An Elements section can exist if there's no Table, as long as it is also empty. Memories can be imported or in a section. There can be only one, but sections can be empty. A Data section can exist if there's no Memory, as long as it is also empty. Prototypes: stringify without .prototype. in the string. WebAssembly.Table.prototype.grow was plain wrong: it takes a delta parameter, not a final size, and throws a RangeError on failure, not a TypeError. Fix compile / instantiate so the reject the promise if given an argument of the wrong type (instead of failing instantly). Fix async on neuter test. Element section shouldn't affect any Table if any of the elements are out of bounds. We need to process it in two passes. Segment section shouldn't affect any Data if any of the segments are out of bounds. We need to process it in two passes. Empty data segments are valid, but only when there is no memory. Their index still gets validated, and has to be zero. Punts: Error messages with context, the test seems overly restrictive but this is minor. compile/instantiate/validate property descriptors. UTF-8 bugs. Temporarily disable NaN tests. We need to go back and implement the following semantics: WebAssembly/spec#414 This doesn't matter as much as getting all the other tests passing. Worth noting for NaNs: f64.no_fold_mul_one (also a NaN test) as well as no_fold_promote_demote (an interesting corner case which we get wrong). mul by one is (assert_return (invoke \"f64.no_fold_mul_one\" (i64.const 0x7ff4000000000000)) (i64.const 0x7ff8000000000000)) which means converting sNaN to qNaN, and promote/demote is (assert_return (invoke \"no_fold_promote_demote\" (i32.const 0x7fa00000)) (i32.const 0x7fc00000)) which is the same. I'm not sure why they're not allowed. JSTests: * wasm.yaml: * wasm/function-tests/i32-load8-s.js: * wasm/function-tests/memory-access-past-4gib.js: (const.op.of.WASM.opcodes): * wasm/function-tests/memory-alignment.js: Added. (const.op.of.WASM.opcodes): * wasm/function-tests/memory-section-and-import.js: * wasm/js-api/Module-compile.js: (async.testPromiseAPI): * wasm/js-api/dont-mmap-zero-byte-memory.js: (testMems): * wasm/js-api/element.js: (assert.throws.new.WebAssembly.Module.builder.WebAssembly): * wasm/js-api/neutered-inputs.js: (const.testFunction): Deleted. (const.testConstructor): Deleted. * wasm/js-api/table.js: (assert.throws.new.WebAssembly.Module.builder.WebAssembly): (new.WebAssembly.Module): (assert.throws): (assertBadTableImport): (assert.throws.WebAssembly.Table.prototype.grow): (assertBadTableInstance): Deleted. * wasm/js-api/test_Data.js: (DataSectionWithoutMemory): * wasm/spec-harness/index.js: (module): (uniqueTest): Deleted. (assert_invalid): Deleted. (assert_soft_invalid): Deleted. (register): Deleted. (call): Deleted. (get instance): Deleted. (exports): Deleted. (run): Deleted. (assert_unlinkable): Deleted. (assert_uninstantiable): Deleted. (assert_trap): Deleted. (try.f): Deleted. (catch): Deleted. (assert_exhaustion): Deleted. (assert_return): Deleted. (assert_return_nan): Deleted. * wasm/spec-harness/testharness.css: Removed. * wasm/spec-harness/testharness.js: Removed. * wasm/spec-harness/testharnessreport.js: Removed. * wasm/spec-harness/wasm-constants.js: (assertTraps): (assertWasmThrows): * wasm/spec-harness/wasm-module-builder.js: (Binary.prototype.emit_section): (Binary): (WasmFunctionBuilder.prototype.addBody): (WasmFunctionBuilder.prototype.end): (WasmFunctionBuilder): (WasmModuleBuilder.prototype.stringToBytes): (WasmModuleBuilder.prototype.addCustomSection): (WasmModuleBuilder.prototype.addFunctionTableInit): (WasmModuleBuilder.prototype.appendToTable): (WasmModuleBuilder.prototype.toArray): (WasmModuleBuilder.prototype.toBuffer): (WasmModuleBuilder.prototype.instantiate): (WasmModuleBuilder): * wasm/spec-tests/address.wast.js: * wasm/spec-tests/align.wast.js: Added. * wasm/spec-tests/binary.wast.js: * wasm/spec-tests/block.wast.js: * wasm/spec-tests/br.wast.js: * wasm/spec-tests/br_if.wast.js: * wasm/spec-tests/br_table.wast.js: * wasm/spec-tests/call.wast.js: * wasm/spec-tests/call_indirect.wast.js: * wasm/spec-tests/comments.wast.js: * wasm/spec-tests/const.wast.js: Added. * wasm/spec-tests/conversions.wast.js: Added. * wasm/spec-tests/custom_section.wast.js: * wasm/spec-tests/exports.wast.js: * wasm/spec-tests/f32.wast.js: Added. * wasm/spec-tests/f64.wast.js: Added. * wasm/spec-tests/fac.wast.js: * wasm/spec-tests/float_exprs.wast.js: Added. * wasm/spec-tests/float_misc.wast.js: Added. * wasm/spec-tests/func.wast.js: * wasm/spec-tests/globals.wast.js: * wasm/spec-tests/if.wast.js: * wasm/spec-tests/imports.wast.js: * wasm/spec-tests/inline-module.wast.js: Added. * wasm/spec-tests/jsapi.js: (testJSAPI.test): (testJSAPI): * wasm/spec-tests/labels.wast.js: * wasm/spec-tests/loop.wast.js: * wasm/spec-tests/memory.wast.js: * wasm/spec-tests/memory_trap.wast.js: Added. * wasm/spec-tests/names.wast.js: * wasm/spec-tests/nop.wast.js: * wasm/spec-tests/return.wast.js: * wasm/spec-tests/stack.wast.js: * wasm/spec-tests/token.wast.js: Added. * wasm/spec-tests/type.wast.js: Added. * wasm/spec-tests/typecheck.wast.js: * wasm/spec-tests/unreachable.wast.js: * wasm/spec-tests/unreached-invalid.wast.js: * wasm/spec-tests/unwind.wast.js: * wasm/spec-tests/utf8-custom-section-id.wast.js: Added. * wasm/spec-tests/utf8-import-field.wast.js: Added. * wasm/spec-tests/utf8-import-module.wast.js: Added. Source/JavaScriptCore: * wasm/WasmB3IRGenerator.cpp: * wasm/WasmFunctionParser.h: * wasm/WasmModuleParser.cpp: * wasm/WasmModuleParser.h: * wasm/WasmParser.h: (JSC::Wasm::Parser<SuccessType>::consumeUTF8String): * wasm/generateWasm.py: (memoryLog2Alignment): * wasm/js/JSWebAssemblyTable.cpp: (JSC::JSWebAssemblyTable::grow): * wasm/js/JSWebAssemblyTable.h: * wasm/js/WebAssemblyCompileErrorPrototype.cpp: * wasm/js/WebAssemblyInstancePrototype.cpp: * wasm/js/WebAssemblyLinkErrorPrototype.cpp: * wasm/js/WebAssemblyMemoryPrototype.cpp: * wasm/js/WebAssemblyModulePrototype.cpp: * wasm/js/WebAssemblyModuleRecord.cpp: (JSC::WebAssemblyModuleRecord::evaluate): * wasm/js/WebAssemblyPrototype.cpp: (JSC::webAssemblyCompileFunc): (JSC::resolve): (JSC::instantiate): (JSC::compileAndInstantiate): (JSC::webAssemblyInstantiateFunc): * wasm/js/WebAssemblyRuntimeErrorPrototype.cpp: * wasm/js/WebAssemblyTablePrototype.cpp: (JSC::webAssemblyTableProtoFuncGrow): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@218216 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Fix various places in the spec text and interpreter where binary encodings were inconsistent with those we decided on in WebAssembly#372.

rossberg reviewed Jan 30, 2017

View reviewed changes

sunfishcode reviewed Jan 30, 2017

View reviewed changes

sunfishcode modified the milestone: MVP Jan 31, 2017

Fix tests to reflect new NaN semantics

d5dc06b

sunfishcode approved these changes Mar 21, 2017

View reviewed changes

Test that select and if return specific NaN bitpatterns.

c087d6c

rossberg force-pushed the master branch 3 times, most recently from 4284e11 to e92b0a9 Compare March 22, 2017 20:19

sunfishcode merged commit 634f0d9 into WebAssembly:master Mar 23, 2017

Cellule mentioned this pull request Mar 25, 2017

.wast assert_return_canonical_nan & assert_return_arithmetic_nan WebAssembly/wabt#370

Closed

This was referenced Mar 29, 2017

Implement the new NaN semantics #286

Closed

Several more tests. #452

Merged

dhil pushed a commit to dhil/webassembly-spec that referenced this pull request Oct 3, 2023

Fix encoding inconsistencies (WebAssembly#414)

95ae953

Fix various places in the spec text and interpreter where binary encodings were inconsistent with those we decided on in WebAssembly#372.

Fix tests to reflect new NaN semantics #414

Fix tests to reflect new NaN semantics #414

Conversation

rfk commented Jan 28, 2017

Choose a reason for hiding this comment

rossberg Jan 30, 2017 • edited Loading

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

rossberg left a comment

Choose a reason for hiding this comment

gahaas commented Jan 30, 2017

sunfishcode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossberg commented Jan 31, 2017

sunfishcode commented Jan 31, 2017

rfk commented Jan 31, 2017

rossberg commented Feb 1, 2017

sunfishcode commented Feb 1, 2017

rossberg commented Feb 2, 2017

binji commented Feb 24, 2017

rossberg commented Feb 24, 2017

rfk commented Feb 26, 2017

rfk commented Mar 5, 2017

rfk commented Mar 5, 2017

rossberg commented Mar 6, 2017 via email

rfk commented Mar 7, 2017

sunfishcode commented Mar 7, 2017

rfk commented Mar 7, 2017

rfk commented Mar 19, 2017

sunfishcode left a 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

rfk commented Mar 21, 2017

sunfishcode commented Mar 22, 2017

sunfishcode commented Mar 22, 2017

rfk commented Mar 23, 2017

rfk commented Mar 23, 2017

sunfishcode commented Mar 23, 2017

rossberg Jan 30, 2017 •

edited

Loading