Force evaluation order in set_date_fields
#268
Conversation
- use `volatile` to force evaluation order in `set_date_fields` - fix evaluation error in test262/test/built-ins/Date/UTC/fp-evaluation-order.js:19: unexpected error: Test262Error: precision in MakeDate Expected SameValue(«34448384», «34447360») to be true - this error occurs on the macOS target with clang --version: Apple clang version 15.0.0 (clang-1500.1.0.2.5) Target: arm64-apple-darwin23.3.0
|
Is the |
|
This is not strict speaking a compiler bug, but ECMA insists on the order of operations when converting the date components into an actual time value, whereas the order of evaluation is not so easy to force in C. There are 2 tests in test262 that keep failing on macOS with M2 hardware and clang v15. Making one of the |
|
I believe I figured out the cause. It's not order of evaluation, not exactly. It's rounding.
The ECMA spec however defines arithmetic in terms of discrete rounding. Your fix is correct. Further evidence: when I add The only thing I'd ask of you is to update the comment with an abridged version of the above, otherwise this tidbit of institutional knowledge is bound to get forgotten again. :-) |
- use `double` arithmetic where necessary to match the spec - use `volatile` to ensure correct order of evaluation and prevent FMA code generation - reject some border cases. - avoid undefined behavior in `double` -> `int64_t` conversions - improved tests/test_builtin.js `assert` function to compare values more reliably. - added some tests in `test_date()`
|
I added the offending tests in our tests/test_builtin.js and I get failures from the CI jobs on Windows (windows-mingw32-debug [Debug and Release]). Looking into that, I noticed the test262 suite is not run on all targets, hence these failures might already be present in the current version and my code does not fix them for those compiler/target combinations. Beside the long run times, are there specific reasons to omit the test262 suite on many target systems? I saw that you disabled some numeric conversion tests on the windows targets, I shall use the same work around for these precision / evaluation order date tests, but what about other targets, why not run the test262 suite? |
|
Is there a way to configure github CI to only report failing CI jobs in their reports (both on this page and in the emails) ? |
We made 262 run in a representative subset of the targets, happy to add more if necessary. Regarding MinGW: there is indeed something going on with floating point rounding. We haven't gotten to the bottom of it yet. |
|
Did you decide on a big-endian target ? |
|
Approved the workflow. I'm traveling this week and hope to add the CI the next week. If all is well we can land this before no problem. |
- use `double` arithmetic where necessary to match the spec - use `volatile` to ensure correct order of evaluation and prevent FMA code generation - reject some border cases. - avoid undefined behavior in `double` -> `int64_t` conversions - improved tests/test_builtin.js `assert` function to compare values more reliably. - added some tests in `test_date()` - disable date precision tests on win32 and cygwin targets - remove unused variable
volatileto force evaluation order inset_date_fieldsApple clang version 15.0.0 (clang-1500.1.0.2.5)
Target: arm64-apple-darwin23.3.0