Summary: | Crash during exception unwinding | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Oliver Hunt <oliver> | ||||||||
Component: | JavaScriptCore | Assignee: | Oliver Hunt <oliver> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ayao, commit-queue, fpizlo, ggaren, jruderman, mark.lam, oliver, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Oliver Hunt
2013-08-15 13:36:48 PDT
As far as i can make out OSR exit is overwriting the activation register with undefined, but not the arguments register Created attachment 208963 [details]
Patch
Comment on attachment 208963 [details]
Patch
Why doesn't this have changes to DFGInPlaceAbstractState, and probably elsewhere, to remove Throw/ThrowReferenceError from the list of block terminals and add Unreachable to that same list?
Created attachment 208964 [details]
Patch
Comment on attachment 208964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208964&action=review Please change the comment in DFGAbstractInterpreter.h that mentions Throw as a terminal. Please change the comment in DFGFlushLivenessAnalysis that mentions Throw as a terminal. Ditto in DFGLivenessAnalysis. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1497 > + case Unreachable: > + break; Since Throw/ThrowReferenceError do setIsValid(false) and presumably all other uses of Unreachable will also be dominated by something that always exits/terminates, shouldn't this be RELEASE_ASSERT_NOT_REACHED()? > Source/JavaScriptCore/dfg/DFGClobberize.h:114 > + case Unreachable: Since this is a control construct, shouldn't it write(SideState)? > Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp:447 > + case Unreachable: > + return false; Why isn't this in the above case block, along with the cfaBranchDirection assertion? > Source/JavaScriptCore/dfg/DFGNodeType.h:252 > macro(Throw, NodeMustGenerate) \ > macro(ThrowReferenceError, NodeMustGenerate) \ Shouldn't you be removing these from the block that has the "block terminals" comment? Wouldn't they be more appropriate in another block of node types with a comment like "these always exit but aren't terminals to allow liveness hooks after them"? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4649 > + case Unreachable: > + // We should not ever reach this code > + m_jit.breakpoint(); > + noResult(node); > + break; Shouldn't this be a RELEASE_ASSERT_NOT_REACHED() because the *compiler* shouldn't even reach this node? (Ditto in the 32-bit compiler.) Created attachment 209107 [details]
Patch
Attachment 209107 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/js/dfg-activation-register-overwritten-in-throw-expected.txt', u'LayoutTests/fast/js/dfg-activation-register-overwritten-in-throw.html', u'LayoutTests/fast/js/script-tests/dfg-activation-register-overwritten-in-throw.js', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/dfg/DFGAbstractInterpreter.h', u'Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGClobberize.h', u'Source/JavaScriptCore/dfg/DFGFixupPhase.cpp', u'Source/JavaScriptCore/dfg/DFGFlushLivenessAnalysisPhase.cpp', u'Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp', u'Source/JavaScriptCore/dfg/DFGNode.h', u'Source/JavaScriptCore/dfg/DFGNodeType.h', u'Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGSafeToExecute.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp']" exit_code: 1
Source/JavaScriptCore/dfg/DFGNodeType.h:246: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 1 in 18 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 209107 [details]
Patch
sweet
Committed r154290: <http://trac.webkit.org/changeset/154290> *** Bug 119902 has been marked as a duplicate of this bug. *** |