Bug 162809 - Emit DebugHooks uniformly with pause locations instead of having separate pause locations and op_debug emits
Summary: Emit DebugHooks uniformly with pause locations instead of having separate pau...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-30 14:20 PDT by Joseph Pecoraro
Modified: 2016-10-12 11:51 PDT (History)
9 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (69.25 KB, patch)
2016-10-11 16:51 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Bots - All Necessary Parts (71.45 KB, patch)
2016-10-11 16:57 PDT, Joseph Pecoraro
ggaren: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>