wasm2c: Fix f64 harness

[SoniEx2]

Old output:

out/test/harness/wasm2c/floating_point\floating_point-main.c:384: assertion failed: in w2c_floating__point__0__wasm_f32(&floating__point__0__wasm_instance, 0.f): expected 123.400002, got 0.
out/test/harness/wasm2c/floating_point\floating_point-main.c:387: assertion failed: in w2c_floating__point__0__wasm_f32(&floating__point__0__wasm_instance, 123.400002f): expected 0, got 123.400002.
out/test/harness/wasm2c/floating_point\floating_point-main.c:390: assertion failed: in w2c_floating__point__0__wasm_f64(&floating__point__0__wasm_instance, 0.0000000000000000L): expected 0, got 6.9218011802898607e-310.
out/test/harness/wasm2c/floating_point\floating_point-main.c:393: assertion failed: in w2c_floating__point__0__wasm_f64(&floating__point__0__wasm_instance, 123.40000000000001L): expected 123.40000000000001, got 6.9218011802898607e-310.

(Note how wrong this output is!)

[keithw]

Hmm, it looks like it was changed to %#.17gL in #1843 as part of adding Windows support; any idea why? I'm not that up on my Python format strings.

[SoniEx2]

In C, L (such as 1.5L) is for long double, but wasm2c uses double everywhere. As far as we can tell? It must've been unintentional, unless it's somehow related to 32-bit x86. But based on what we have today it is clearly wrong.

(We also note that our second person pronouns are "it instead of you".)

[SoniEx2]

For what it's worth, the relevant code in c-writer.cc is at line 1295. It does not take the L.

[sbc100]

Is this fix only needed on plaforms where long double and no the same as double?

On what platform did you discover the issue?

[SoniEx2]

we first noticed the issue on both linux-x64 and linux-s390x (via qemu).

we do strongly believe this is the correct way of doing it. passing long double to a format string expecting double is UB.

[sbc100]

we first noticed the issue on both linux-x64 and linux-s390x (via qemu).

we do strongly believe this is the correct way of doing it. passing long double to a format string expecting double is UB.

I understand, I'm just trying to figure exactly who is effected and how.

[SoniEx2]

it affects our ability to understand test failures. that's all this fix does, makes it possible for someone to look at the test output and figure out what went wrong, as opposed to guessing what went wrong.

[workingjubilee]

Hmm, it looks like it was changed to %#.17gL in #1843 as part of adding Windows support; any idea why? I'm not that up on my Python format strings.

Windows now treats long double as equivalent to double. It is possible the change was just to enforce the use of the full double-precision floating point type handling.

It should be noted that explicitly using long double can get you all of, on x86-64 platforms, with the only difference being OS:

• a double precision floating-point value (AKA binary64 AKA f64): Windows
• an extended precision x87 floating-point value (AKA f80): GNU C compiler
• a quadruple precision floating-point value (AKA binary128 AKA f128), as a soft float: Android, GNU C compiler configuration for Bionic

[SoniEx2]

We will further note that f64 has always been double, never long double, even on Windows, as per https://github.com/WebAssembly/wabt/blob/main/src/template/wasm2c.top.h .

Everything appears to point to this being a mistake, or a misunderstanding of the Windows types or something. Also, CI passes with this change, even on Windows.

[sbc100]

lgtm

Please bear with me for for one more question though. Note I don't doubt the value of this change or the likelihood that this was a mistake/oversight.

Do you know why none of the existing test outputs changed here? Do we not have existing tests that contain f64 outputs and with f64 assertion failures? i.e. why do we need a new test here, why isn't this covered by existing spec tests?

[sbc100]

lgtm % last question

[SoniEx2]

This is a test harness test. The spec tests expect assertions to, well, pass, while here we're testing what happens when an assertion doesn't pass. (The spec tests sometimes assert that a function traps, but that's different from an assertion failure.)

When passed into a module, the long double was narrowed into a double, so anything passing through wasm was a double. But due to the way the test harness macros are set up it would still pass these long double literals as long double into the printf machinery - which was expecting a double.

[sbc100]

This is a test harness test. The spec tests expect assertions to, well, pass, while here we're testing what happens when an assertion doesn't pass. (The spec tests sometimes assert that a function traps, but that's different from an assertion failure.)

Thanks, that was what I was missing. I was expecting to see some change to the output of expected failures in the spec tests .. but this change only effects unexpected failures.

lgtm

[keithw]

(updating so this will automerge)