Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[wasm2c] MSVC miscompiles for certain fp constants #2388

Open
InflexCZE opened this issue Feb 10, 2024 · 6 comments · May be fixed by #2389
Open

[wasm2c] MSVC miscompiles for certain fp constants #2388

InflexCZE opened this issue Feb 10, 2024 · 6 comments · May be fixed by #2389

Comments

@InflexCZE
Copy link

Given WASM

0000024: 44                                        ; f64.const
0000025: 0000 0000 0000 e0c1                       ; f64 literal

(module
  (func (export "addTwo") (result f64)
    f64.const -0x1p+31
  )
)

and wasm2c conversion output

//wasm2c test.wasm
f64 w2c__addTwo_0(w2c_* instance) {
  FUNC_PROLOGUE;
  f64 var_d0;
  var_d0 = -2147483648;
  FUNC_EPILOGUE;
  return var_d0;
}

there is slight discrepancy in output of different compilers.
Clang & GCC correctly output -2147483648.000000
MSVC throws warning (error with /sdl) and outputs 2147483648.000000 (notice absence of negative mark)
https://godbolt.org/z/MrjMzdeKE
image

While the conversion output is likely correct according to C spec (at least Clang and GCC are not screaming with -Wall), it's very inconvenient for Windows users to do these fixups manually.

Proposal is to use alternative notation

  var_d0 = -2147483648.0; //Notice additional decimal

which makes all tested compilers happy and correct.
https://godbolt.org/z/5sE4adbYd
image
@InflexCZE InflexCZE changed the title [wasm2c] MSVC misscompiles for certain fp constants [wasm2c] MSVC miscompiles for certain fp constants Feb 10, 2024
@keithw keithw linked a pull request Feb 10, 2024 that will close this issue
Open
@keithw
Copy link
Member

keithw commented Feb 10, 2024

Thanks for reporting this! Can you please test #2389 and see if it fixes the issue for you?

There should probably be a test for this situation, ideally eventually upstream in the spec tests.

I'm also nervous about MSVC's handling of wasm2c output for conversion instructions given https://github.com/WebAssembly/wabt/blob/main/src/c-writer.cc#L4851 and https://github.com/WebAssembly/wabt/blob/main/src/config.cc#L76 -- we should make sure this is also tested.

@rossberg rossberg mentioned this issue Feb 11, 2024
Merged
@rossberg
Copy link
Member

There should probably be a test for this situation, ideally eventually upstream in the spec tests.

@keithw, PTAL at this spec PR.

@InflexCZE
Copy link
Author

#2389 looks good, no unexpected overhead either
https://godbolt.org/z/xf4Wfa4Ev

  1. Might be good to export with //decimal value comment at the end of the line, for users' convenience
  2. Will prolly need byte re-order for little-endian

@SoniEx2
Copy link
Contributor

SoniEx2 commented Feb 11, 2024

it works fine on both endian

@keithw
Copy link
Member

keithw commented Feb 11, 2024

#2389 looks good, no unexpected overhead either https://godbolt.org/z/xf4Wfa4Ev

1. Might be good to export with `//decimal value` comment at the end of the line, for users' convenience

Thanks, good idea. Done in #2389.

This was referenced May 14, 2024
Closed
Merged
@SoniEx2
Copy link
Contributor

SoniEx2 commented Sep 16, 2024

is this still an issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

c-writer.cc: represent float consts as integer literals WebAssembly/wabt
4 participants
@keithw @SoniEx2 @rossberg @InflexCZE