Fix test_rounding for recent LLVM change#26975
Conversation
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
Never heard to this pragma.. lgtm if works.
|
Yeah, I never heard of it either, but at least the LLVM warning is clear: |
| e.g. Intel x64 Macs and Windows-on-ARM, downloading Java SE Development Kit | ||
| 21.0.11 from https://www.oracle.com/europe/java/technologies/downloads/#java21 | ||
| is required in order to use Emscripten's Closure Compiler integration. | ||
| - clang now warns if fp exception functions are called without appropriate |
There was a problem hiding this comment.
It's not just exception functions, I think it's anything from fenv.h (in our test it's fegetround).
There was a problem hiding this comment.
Though, re-reading the LLVM commit, they do call these "fp exception functions"?
|
Landing to unblock the roller, we can iterate on the text later if we want. |
|
Oh, this pragma fails on this target actually... I'll disable the warning then. That also makes the changlog moot (users have just one choice here). |
|
Ah, that doesn't work too, because the warning doesn't exist... I'll disable this not-very-important test to allow the roll, then fix. |
| self.do_run_in_out_file_test('math/fmodf.c') | ||
|
|
||
| def test_rounding(self): | ||
| self.skipTest('https://github.com/emscripten-core/emscripten/pull/26975') |
There was a problem hiding this comment.
You can use -Wno-unknown-warning-option for this :)
There was a problem hiding this comment.
Oh nice, thanks. Done. Maybe some day I'll get this PR right...
|
lol the LLVM commit was just reverted, so this is not needed any more (but might be if it relands...) |
|
You said we don't support this pragma on Wasm? I guess it sort of makes sense since it doesn't really work, but it seems like if we're going to have a no-op fenv.h we should have a noop pragma too. |
|
Yes, the pragma just errors in clang on not being supported in the target. I agree it's not very consistent (though enough of a corner case that I'm not terribly worried...) |
|
Yeah, according to the discussion on llvm/llvm-project#187860 the question of what to do on targets that don't support FENV_ACCESS is one of the current issues. Assuming wasm is currently recognized as that, then maybe we will fall into whatever solution they come up with (presumably just disabling the warning on those targets). Although having our "noop" fenv functions suggests that we could potentially also have a noop pragma and require it to be used. This would mean that if and when we get rounding modes in wasm and want to support them in Emscripten code that correctly uses the rounding modes we support would start working as intended. |
3 participants
As of llvm/llvm-project@5f2bedc, LLVM warns on using fe* methods without proper flags.
This does not really matter in wasm, but we still must disable the warning.