Changeset 285788 in webkit

Timestamp:

Nov 14, 2021, 12:45:44 AM (5 years ago)

Author:

commit-queue@webkit.org

Message:

Prevent fused multiply add during ParseInt
​https://bugs.webkit.org/show_bug.cgi?id=232951

Patch by Mikhail R. Gadelha <Mikhail R. Gadelha> on 2021-11-14
Reviewed by Yusuke Suzuki.

When parsing the string in parseInt, gcc can wrongfully generate
a fused multiply-add instruction, causing the conversion to be wrong
for some high values. An add followed by a multiply gives the correct
result and it is the code generated most of the times.

This patch adds a volatile qualifier to the number variable, so the
compiler doesn't try to optimize it, and enables a failing test on
mips.

Alternative solutions that I tried but gcc seems to ignore: #pragma
STDC FP_CONTRACT OFF, compiling with -ffp-contract=off, and setting function
attributes attribute((optimize("fp-contract=off"))) and
attribute((optimize("-ffp-contract=off"))), so volative seems to be
a good compromise.

The issue was found when cross compiling to mips with gcc 8.4.0 and
options -ffp-contract=off -mmadd4.

JSTests:

• ChakraCore.yaml:

Source/JavaScriptCore:

• runtime/ParseInt.h:

(JSC::parseInt):

Location:

trunk

Files:

• JSTests/ChakraCore.yaml (modified) (1 diff)
• JSTests/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/runtime/ParseInt.h (modified) (1 diff)

trunk/JSTests/ChakraCore.yaml

@@ -697 +697 @@
 - path: ChakraCore/test/GlobalFunctions/ParseInt1.js
   # Currently fails on the Loongson 3A4000 (in 32-bits mode).
-  cmd: |
-    if $architecture == "mips"
-      skip
-    else
-      runChakra :baseline, "NoException", "ParseInt1.baseline", []
-    end
+  cmd: runChakra :baseline, "NoException", "ParseInt1.baseline", []
 - path: ChakraCore/test/GlobalFunctions/toString.js
   cmd: runChakra :baseline, "NoException", "toString.baseline", []

trunk/JSTests/ChangeLog

@@ +1 @@
+2021-11-14  Mikhail R. Gadelha  <mikhail@igalia.com>
+
+        Prevent fused multiply add during ParseInt
+        https://bugs.webkit.org/show_bug.cgi?id=232951
+
+        Reviewed by Yusuke Suzuki.
+
+        When parsing the string in parseInt, gcc can wrongfully generate
+        a fused multiply-add instruction, causing the conversion to be wrong
+        for some high values. An add followed by a multiply gives the correct
+        result and it is the code generated most of the times.
+
+        This patch adds a volatile qualifier to the number variable, so the
+        compiler doesn't try to optimize it, and enables a failing test on
+        mips.
+
+        Alternative solutions that I tried but gcc seems to ignore: #pragma
+        STDC FP_CONTRACT OFF, compiling with -ffp-contract=off, and setting function
+        attributes __attribute__((optimize("fp-contract=off"))) and
+        __attribute__((optimize("-ffp-contract=off"))), so volative seems to be
+        a good compromise.
+
+        The issue was found when cross compiling to mips with gcc 8.4.0 and
+        options -ffp-contract=off -mmadd4.
+
+        * ChakraCore.yaml:
+
 2021-11-12  Joseph Griego  <jgriego@igalia.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2021-11-14  Mikhail R. Gadelha  <mikhail@igalia.com>
+
+        Prevent fused multiply add during ParseInt
+        https://bugs.webkit.org/show_bug.cgi?id=232951
+
+        Reviewed by Yusuke Suzuki.
+
+        When parsing the string in parseInt, gcc can wrongfully generate
+        a fused multiply-add instruction, causing the conversion to be wrong
+        for some high values. An add followed by a multiply gives the correct
+        result and it is the code generated most of the times.
+
+        This patch adds a volatile qualifier to the number variable, so the
+        compiler doesn't try to optimize it, and enables a failing test on
+        mips.
+
+        Alternative solutions that I tried but gcc seems to ignore: #pragma
+        STDC FP_CONTRACT OFF, compiling with -ffp-contract=off, and setting function
+        attributes __attribute__((optimize("fp-contract=off"))) and
+        __attribute__((optimize("-ffp-contract=off"))), so volative seems to be
+        a good compromise.
+
+        The issue was found when cross compiling to mips with gcc 8.4.0 and
+        options -ffp-contract=off -mmadd4.
+
+        * runtime/ParseInt.h:
+        (JSC::parseInt):
+
 2021-11-12  Darin Adler  <darin@apple.com>
 

trunk/Source/JavaScriptCore/runtime/ParseInt.h

@@ -163 +163 @@
     int firstDigitPosition = p;
     bool sawDigit = false;
+#if COMPILER(GCC)
+    // Due to a bug found in GCC v8.4.0, a wrong fused multiply-add optimization can be inserted when calculating the final number,
+    // in number *= radix; number += digit;, so add volatile to prevent optimizations.
+    // GCC v8.4.0 also seems to ignore #pragma STDC FP_CONTRACT OFF, compiling with -ffp-contract=off, and setting function attributes
+    // __attribute__((optimize("fp-contract=off"))) and __attribute__((optimize("-ffp-contract=off"))).
+    volatile double number = 0;
+#else
     double number = 0;
+#endif
     while (p < length) {
         int digit = parseDigit(data[p], radix);