[webkit-reviews] review denied: [Bug 119860] Crash during exception unwinding : [Attachment 208964] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 119860: Crash during exception unwinding
https://bugs.webkit.org/show_bug.cgi?id=119860

Attachment 208964: Patch
https://bugs.webkit.org/attachment.cgi?id=208964&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
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.)