Bug 162809

Summary: Emit DebugHooks uniformly with pause locations instead of having separate pause locations and op_debug emits
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: JavaScriptCoreAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, ggaren, joepeck, keith_miller, mark.lam, msaboff, saam, timothy
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
[PATCH] Proposed Fix
none
[PATCH] For Bots - All Necessary Parts ggaren: review+, commit-queue: commit-queue-

Description Joseph Pecoraro 2016-09-30 14:20:40 PDT
Summary:
Emit DebugHooks uniformly with pause locations instead of having separate pause locations and op_debug emits

Currently:
- The Parser generates pause locations and sets nodes as needing a debug hook
- Bytecode generation emits op_debugs, and ensures all nodes it generates for needed a debug hook

This ensures op_debugs are generated for breakpoint locations.
However this does not ensure all breakpoint locations generate op_debugs.

We should eliminate the distinction so that op_debugs are always generated for nodes where we set breakpoint locations.
Comment 1 Joseph Pecoraro 2016-10-11 16:51:00 PDT
Created attachment 291318 [details]
[PATCH] Proposed Fix
Comment 2 Joseph Pecoraro 2016-10-11 16:56:34 PDT
Naturally this doesn't apply because of patches still up for review that I'm building off of =(. I'll put up a combined patch for the bots.
Comment 3 Joseph Pecoraro 2016-10-11 16:57:19 PDT
Created attachment 291319 [details]
[PATCH] For Bots - All Necessary Parts
Comment 4 Joseph Pecoraro 2016-10-11 17:02:57 PDT
Comment on attachment 291318 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=291318&action=review

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:101
> +    if (value != MixedTriState) {
> +        if (UNLIKELY(needsDebugHook()))
> +            generator.emitDebugHook(this);
> +    }

The reason these emitBytecodeInConditionContext don't arbitrarily emit a debug hook is because if we fall into the ExpressionNode::emitBytecodeInConditionContext path, that function will emit a debug hook. So we do this dance to avoid emitting two debug hooks. I'll clarify this in the ChangeLog somewhere.
Comment 5 Geoffrey Garen 2016-10-12 11:32:06 PDT
Comment on attachment 291319 [details]
[PATCH] For Bots - All Necessary Parts

r=me
Comment 6 WebKit Commit Bot 2016-10-12 11:33:19 PDT
Comment on attachment 291319 [details]
[PATCH] For Bots - All Necessary Parts

Rejecting attachment 291319 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 291319, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebInspectorUI/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2271766
Comment 7 Joseph Pecoraro 2016-10-12 11:51:08 PDT
<https://trac.webkit.org/changeset/207228>