Bug 176317

Created attachment 319800 [details] WIP

Created attachment 319801 [details] WIP

Comment on attachment 319801 [details] WIP Attachment 319801 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/4443676 New failing tests: v8-v6/v8-earley-boyer.js.dfg-eager airjs-tests.yaml/stress-test.js.default v8-v6/v8-earley-boyer.js.ftl-eager-no-cjit-b3o1 stress/v8-earley-boyer-strict.js.ftl-eager stress/v8-earley-boyer-strict.js.ftl-eager-no-cjit airjs-tests.yaml/stress-test.js.no-ftl airjs-tests.yaml/stress-test.js.ftl-no-cjit stress/v8-earley-boyer-strict.js.ftl-eager-no-cjit-b3o1 v8-v6/v8-earley-boyer.js.dfg-eager-no-cjit-validate ChakraCore.yaml/ChakraCore/test/Lib/construct.js.default v8-v6/v8-earley-boyer.js.ftl-eager-no-cjit v8-v6/v8-earley-boyer.js.ftl-eager stress/v8-earley-boyer-strict.js.dfg-eager-no-cjit-validate stress/v8-earley-boyer-strict.js.dfg-eager

Comment on attachment 319801 [details] WIP Attachment 319801 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4443783 New failing tests: imported/w3c/web-platform-tests/html/dom/reflection-embedded.html

Created attachment 319808 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 319801 [details] WIP Attachment 319801 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4443810 New failing tests: imported/w3c/web-platform-tests/html/dom/reflection-embedded.html

Created attachment 319813 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5

Created attachment 319818 [details] WIP

Created attachment 319861 [details] patch

Comment on attachment 319861 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=319861&action=review r=me with some comments. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2370 > + // We rely on constant folding to CheckStructure to do the heavy lifting Typo: "We rely on constant folding to CheckStructure" => "We rely on (the?) constant folding of CheckStructure" Also, what's the reason to not do the conversion to CheckStructure here?

Comment on attachment 319861 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=319861&action=review >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2370 >> + // We rely on constant folding to CheckStructure to do the heavy lifting > > Typo: "We rely on constant folding to CheckStructure" => "We rely on (the?) constant folding of CheckStructure" > > Also, what's the reason to not do the conversion to CheckStructure here? Because AI is ran outside of constant folding.

Created attachment 319870 [details] patch for landing

Comment on attachment 319870 [details] patch for landing Clearing flags on attachment: 319870 Committed r221607: <http://trac.webkit.org/changeset/221607>

All reviewed patches have been landed. Closing bug.

Comment on attachment 319870 [details] patch for landing This feels sketchy to me. JSValue() is not a value that we should encounter during normal execution. Usually JSValue() means that you've read garbage memory that isn't always guaranteed to be JSValue(). For this reason, most of JSC is not JSValue()-safe. There have been exceptions to this general rule in the past. For example, the lazy arguments object, which no longer exists, used a strategy of treating JSValue() as a special marker value. Still, even that behavior never read JSValue() into normal program control flow. TDZ is another exception. We use JSValue() to demarcate a TDZ read. But what causes the TDZ value to enter normal program control flow? I don't think that behavior is correct, and I think it might break lots of stuff. Instead, I think TDZ needs to throw immediately upon being read.

(In reply to Geoffrey Garen from comment #15) > Comment on attachment 319870 [details] > patch for landing > > This feels sketchy to me. > > JSValue() is not a value that we should encounter during normal execution. > Usually JSValue() means that you've read garbage memory that isn't always > guaranteed to be JSValue(). For this reason, most of JSC is not > JSValue()-safe. I don't agree with this assertion. I've never taken reading JSValue() to mean the program likely read garbage memory. We use this in places where it'd be wrong if the value weren't JSValue() under certain circumstances. > > There have been exceptions to this general rule in the past. For example, > the lazy arguments object, which no longer exists, used a strategy of > treating JSValue() as a special marker value. Still, even that behavior > never read JSValue() into normal program control flow. > > TDZ is another exception. We use JSValue() to demarcate a TDZ read. But what > causes the TDZ value to enter normal program control flow? I don't think > that behavior is correct, and I think it might break lots of stuff. Instead, > I think TDZ needs to throw immediately upon being read. Because the TDZ marker is JSValue(), I don't see how we can guarantee that it can't be part of the normal program control flow. I think things become cleaner once we are OK with this. For example, bytecode locals may be JSValue() because they're TDZ. We can't emit a TDZ check before we do any mov of a bytecode local. I agree that almost all of our bytecode operations would crash if TDZ flowed into them. It's the bytecode generator's job to ensure that this doesn't happen. It already does this by emitting TDZ checks. The DFG must respect this property of bytecode. Many DFG nodes would also crash if JSValue() flowed into them. However, the way we emit bytecode ensures that only a handful of DFG nodes will see JSValue(). It would be wrong for the DFG to do code motion in such a way that it moves code around or inserts code that may see the empty value but can't handle it. This is what the type check hoisting phase was doing. It was doing code motion by hoisting structure checks to right before a SetLocal. Conservatively, we must assume any given local can be JSValue() (AI and other analyses can prove something stronger as needed). From this perspective, this is the DFG's error. It hoisted code to a place that saw the empty value but couldn't handle it. For similar reasons, I didn't change other CheckStructure's that we emit elsewhere to be CheckStructureOrEmpty, such as in bytecode parser or fixup phase. We emit those CheckStructures in such a way that they're guaranteed to see the empty value.

<rdar://problem/34261630>