Issue metadata
Sign in to add a comment
|
v8 is broken by new VC++ and new SDK toolchain |
||||||||||||||||||||||
Issue descriptionOS: Windows Landing crrev.com/c/1342814 broke v8 builds - several tests failed, including these ones: InterpreterBinaryOpsSmi RunFloat64Mod RunFloat64ModP constant-folding-2 smi-negative-zero The build output can be found here: https://ci.chromium.org/p/v8/builders/luci.v8.try/v8_win64_rel_ng_triggered/b8927260687988879728
,
Dec 14
,
Dec 14
,
Dec 17
Build settings are: 'gn_args': 'dcheck_always_on = true is_component_build = false is_debug = false symbol_level = 1 target_cpu = "x64" use_goma = true' Some specific failure details include: Command: e:\b\swarm_slave\w\ir\out\Release\cctest.exe test-run-machops/RunFloat64ModP --random-seed=831298184 --nohard-abort === cctest/test-interpreter/InterpreterBinaryOpsSmi === --- stderr --- # # Fatal error in ../../test/cctest/interpreter/test-interpreter.cc, line 346 # Check failed: return_value->SameValue(*expected_value). # #FailureMessage Object: 000000000038F2E8 ==== C stack trace =============================== v8::base::debug::StackTrace::StackTrace [0x000000014175240B+27] v8::platform::DefaultPlatform::GetStackTracePrinter [0x00000001416E9AA7+55] V8_Fatal [0x00000001416E0ED5+213] v8::internal::interpreter::InterpreterTester::GetBytecodeFunction<v8::internal::Handle<v8::internal::Object>,v8::internal::Handle<v8::internal::Oddball>,v8::internal::Handle<v8::internal::Smi>,v8::internal::Handle<v8::internal::Smi>,v8::internal::Handle<v [0x000000013FC7160F+13359] CcTest::Run [0x000000013FAE11B9+201] main [0x000000013FAE1F7F+815] __scrt_common_main_seh [0x00000001417ADA58+268] (d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288) BaseThreadInitThunk [0x00000000777959CD+13] RtlUserThreadStart [0x00000000779CB891+33] out\Release\cctest.exe test-interpreter/InterpreterBinaryOpsSmi --random-seed=831298184 --stress-background-compile --nohard-abort === cctest/test-run-machops/RunFloat64ModP === --- stderr --- # # Fatal error in ../../test/cctest/compiler/test-run-machops.cc, line 4083 # Check failed: DoubleWrapper(Modulo(*i, *j)) == DoubleWrapper(bt.call(*i, *j)) (0 (0x0000000000000000) vs. -0 (0x0000000000000080)). # #FailureMessage Object: 00000000003DF788 ==== C stack trace =============================== v8::base::debug::StackTrace::StackTrace [0x000000014175240B+27] v8::platform::DefaultPlatform::GetStackTracePrinter [0x00000001416E9AA7+55] V8_Fatal [0x00000001416E0ED5+213] v8::base::MakeCheckOpString<int,short> [0x000000013FBB988C+166380] CcTest::Run [0x000000013FAE11B9+201] main [0x000000013FAE1F7F+815] __scrt_common_main_seh [0x00000001417ADA58+268] (d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288) BaseThreadInitThunk [0x00000000777959CD+13] RtlUserThreadStart [0x00000000779CB891+33]
,
Dec 18
I've looked at this test and can repro the problem: out\Release\cctest.exe test-interpreter/InterpreterBinaryOpsSmi --random-seed=831298184 --stress-background-compile --nohard-abort All I have to do is patch in the new hash to build\vs_toolchain.py, run gclient runhooks, build the release directory, and run the test. It fails as on the bots. This is curious since the build is being done with clang, so somehow one of the header files or libraries is changing the results. Disabling optimizations in the affected functions doesn't seem to change things. The failure happens when l == 3 && r == 5 && o == 10, so lhs is -17, rhs is 1, and operator is Token::Value::MOD. Perhaps fmod behaves differently now when lhs is negative?
,
Dec 18
Yep, fmod(-17,1) used to give -0 as the result and now it gives +0. I'll experiment a bit more to figure out what this means and what to do next (and I'll have to see what the spec says, if it does).
,
Dec 18
A little twitter reconnaissance: https://twitter.com/BruceDawson0xB/status/1074916689748213760 finds this bug: https://developercommunity.visualstudio.com/content/problem/375091/fmod-returns-incorrect-result-after-kb4346783.html So, the new fmod behavior is incorrect. On debug (or is it just on component?) builds this bad behavior will be OS dependent (the universal CRT is an OS component) so we may need to teach the tests to ignore this particular misbehavior of fmod. I will investigate the other failures to see if this change explains all of them.
,
Dec 18
CC'ing some V8 folks that might be able to route this to the right V8 owners if needed.
,
Dec 18
Things that would be useful to know:
Does the change in fmod affect v8, or just its tests?
Is there a wrapper function I can alter to correct this? It looks like Modulo() in v8\src\utils.h will do the trick.
I *think* the answer is to modify Modulo as shown below and then use it instead of std::fmod in BinaryOpC() and possibly a few other places:
diff --git a/src/utils.h b/src/utils.h
index a26aabcdeb..80f21c6d56 100644
--- a/src/utils.h
+++ b/src/utils.h
@@ -228,7 +228,12 @@ inline double Modulo(double x, double y) {
// dividend is a zero and divisor is nonzero finite => result equals dividend
if (!(std::isfinite(x) && (!std::isfinite(y) && !std::isnan(y))) &&
!(x == 0 && (y != 0 && std::isfinite(y)))) {
- x = fmod(x, y);
+ double result = fmod(x, y);
+ // Workaround MS bug in VS 2017 15.9, https://crbug.com/915045
+ // fmod(-17, +/-1) should equal -0.0 but now returns 0.0.
+ if (x < 0 && result == 0)
+ result = -0.0;
+ x = result;
}
return x;
#elif defined(V8_OS_AIX)
This fixes at least one failure.
,
Dec 18
It looks like fixing these five test failures is easy: Fixed by updating Modulo(): RunFloat64Mod out\Release\cctest.exe test-run-machops/RunFloat64Mod --random-seed=831298184 --noopt --nohard-abort RunFloat64ModP out\Release\cctest.exe test-run-machops/RunFloat64ModP --random-seed=831298184 --nohard-abort constant-folding-2 out\Release\d8.exe --test test\mjsunit\mjsunit.js test\mjsunit\constant-folding-2.js --random-seed=831298184 --noopt --nohard-abort --allow-natives-syntax --nostress-opt --opt smi-negative-zero out\Release\d8.exe --test test\mjsunit\mjsunit.js test\mjsunit\smi-negative-zero.js --random-seed=831298184 --stress-opt --always-opt --nohard-abort Fixed by having BinaryOpC() call the updated Modulo(): InterpreterBinaryOpsSmi: out\Release\cctest.exe test-interpreter/InterpreterBinaryOpsSmi --random-seed=831298184 --stress-background-compile --nohard-abort It's worrisome that there are probably other plays where fmod(-X*n, X) will give different results, but on the other hand, +0 versus -0 usually doesn't actually matter, so ¯\_(ツ)_/¯. This should fix these failures: https://chromium-review.googlesource.com/c/v8/v8/+/1383091/
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/4bd1215c95dd624eac56b597038f48d5ab6ee0ca commit 4bd1215c95dd624eac56b597038f48d5ab6ee0ca Author: Bruce Dawson <brucedawson@chromium.org> Date: Wed Dec 19 18:05:18 2018 Workaround VS/UCRT fmod bug Recent versions of the Windows Universal CRT changed the behavior of fmod for when the first parameter is negative. In particular, a result of negative zero became positive zero. This is rarely critical but it causes test failures and may effect some JS test suites or web pages. The fix is to modify Modulo to check for a result of 0 when the first parameter is negative and change the result to -0. That fixes four of the five test failures and the fifth one is fixed by comparing the results against Modulo instead of std::fmod. Bug: chromium:915045 Change-Id: Ia4490ec98361a37006d6c338acd33f959fa3ccea Reviewed-on: https://chromium-review.googlesource.com/c/1383091 Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Reviewed-by: Ross McIlroy <rmcilroy@chromium.org> Cr-Commit-Position: refs/heads/master@{#58377} [modify] https://crrev.com/4bd1215c95dd624eac56b597038f48d5ab6ee0ca/src/utils.h [modify] https://crrev.com/4bd1215c95dd624eac56b597038f48d5ab6ee0ca/test/cctest/interpreter/test-interpreter.cc
,
Dec 19
I believe that all of the v8 failures are fixed now. I ran try jobs with the new toolchain (I think, but I can't confirm from the try jobs' outputs) and they succeeded: https://chromium-review.googlesource.com/c/v8/v8/+/1384792 Any concerns? I'm going to mark this as fixed based on the try jobs but any confirmation of whether the try jobs worked as intended would be great. The toolchain change will probably reland after the holidays, but I'd like to confirm that the road blocks are addressed now.
,
Dec 19
Go ahead and reland after the holidays (just speaking for this road block). Due to vacation, there will be proper monitoring of the V8 deps rolls first after Jan 6.
,
Dec 28
Adding a NextAction date of a time when I think a new Windows SDK should have shipped. At that point I will try building with the new SDK to see if the Microsoft bug in fmod has been fixed, and then consider a plan to remove the hack. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by brucedaw...@chromium.org
, Dec 14