when lowering AssertNotEmpty, create the value before creating the patchpoint
authorrmorisset@apple.com <rmorisset@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Feb 2019 01:36:28 +0000 (01:36 +0000)
committerrmorisset@apple.com <rmorisset@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Feb 2019 01:36:28 +0000 (01:36 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194231

Reviewed by Saam Barati.

JSTests:

This test is painfully fragile: it tries to test that AssertNotEmpty on a constant produces valid B3 IR.
The problem is that AssertNotEmpty is only created by DFGConstantFolding when it can simplify a CheckStructure, and constant folding is a bit capricious (https://bugs.webkit.org/show_bug.cgi?id=133947)
So even tiny changes to this test can change the path code taken.

* stress/assert-not-empty.js: Added.
(foo):

Source/JavaScriptCore:

This is a very simple change: we should never generate B3 IR where an instruction depends on a value that comes later in the instruction stream.
AssertNotEmpty was generating some such IR, it probably slipped through until now because it is a rather rare and tricky instruction to generate.

* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileAssertNotEmpty):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@240959 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/assert-not-empty.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

index 8d03fbc..db11322 100644 (file)
@@ -1,3 +1,17 @@
+2019-02-04  Robin Morisset  <rmorisset@apple.com>
+
+        when lowering AssertNotEmpty, create the value before creating the patchpoint
+        https://bugs.webkit.org/show_bug.cgi?id=194231
+
+        Reviewed by Saam Barati.
+
+        This test is painfully fragile: it tries to test that AssertNotEmpty on a constant produces valid B3 IR.
+        The problem is that AssertNotEmpty is only created by DFGConstantFolding when it can simplify a CheckStructure, and constant folding is a bit capricious (https://bugs.webkit.org/show_bug.cgi?id=133947)
+        So even tiny changes to this test can change the path code taken.
+
+        * stress/assert-not-empty.js: Added.
+        (foo):
+
 2019-02-01  Mark Lam  <mark.lam@apple.com>
 
         Remove invalid assertion in DFG's compileDoubleRep().
diff --git a/JSTests/stress/assert-not-empty.js b/JSTests/stress/assert-not-empty.js
new file mode 100644 (file)
index 0000000..bafc229
--- /dev/null
@@ -0,0 +1,12 @@
+//@ runDefault("--useObjectAllocationSinking=0")
+
+function foo(o) {
+  if (!o) {
+    +eval;
+  }
+  o.x;
+};
+let i=0;
+for (;i<100000;++i) {
+  foo(Object);
+}
index 5360edd..4d70226 100644 (file)
@@ -1,3 +1,16 @@
+2019-02-04  Robin Morisset  <rmorisset@apple.com>
+
+        when lowering AssertNotEmpty, create the value before creating the patchpoint
+        https://bugs.webkit.org/show_bug.cgi?id=194231
+
+        Reviewed by Saam Barati.
+
+        This is a very simple change: we should never generate B3 IR where an instruction depends on a value that comes later in the instruction stream.
+        AssertNotEmpty was generating some such IR, it probably slipped through until now because it is a rather rare and tricky instruction to generate.
+
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileAssertNotEmpty):
+
 2019-02-04  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [JSC] ExecutableToCodeBlockEdge should be smaller
index 4f3a5a0..f4764b7 100644 (file)
@@ -3111,8 +3111,9 @@ private:
         if (!validationEnabled())
             return;
 
+        LValue val = lowJSValue(m_node->child1());
         PatchpointValue* patchpoint = m_out.patchpoint(Void);
-        patchpoint->appendSomeRegister(lowJSValue(m_node->child1()));
+        patchpoint->appendSomeRegister(val);
         patchpoint->setGenerator(
             [=] (CCallHelpers& jit, const StackmapGenerationParams& params) {
                 AllowMacroScratchRegisterUsage allowScratch(jit);