New issue
Advanced search Search tips

Issue 915045 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 19
Cc:
Components:
EstimatedDays: ----
NextAction: 2019-04-15
OS: Windows
Pri: 3
Type: Bug

Blocking:
issue 915046



Sign in to add a comment

v8 is broken by new VC++ and new SDK toolchain

Project Member Reported by brucedaw...@chromium.org, Dec 14

Issue description

OS: 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
 
Labels: OS-Windows
Components: Infra>Client>V8
Blocking: 915046
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]

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?

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).
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.

Cc: mvstan...@chromium.org rmcilroy@chromium.org
Components: -Infra>Client>V8 Blink>JavaScript
CC'ing some V8 folks that might be able to route this to the right V8 owners if needed.
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.

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/

Project Member

Comment 11 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
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.

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.
NextAction: 2019-04-15
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