Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2013-12-02 10:48 PST by Peter Molnar
Modified: 2013-12-04 12:38 PST (History)
4 users (show)

See Also:

Proposed patch (12.72 KB, patch)
2013-12-02 10:52 PST, Peter Molnar
no flags Details | Formatted Diff | Diff
Proposed patch - fixed style (12.72 KB, patch)
2013-12-02 10:56 PST, Peter Molnar
mark.lam: review-
Details | Formatted Diff | Diff
Updated patch (3.43 KB, patch)
2013-12-03 05:22 PST, Peter Molnar
mark.lam: review-
Details | Formatted Diff | Diff
Patch (6.52 KB, patch)
2013-12-04 00:56 PST, Peter Molnar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Molnar 2013-12-02 10:48:53 PST
Build fix after http://trac.webkit.org/changeset/158937 which broke the JSC build if JavaScript Debugger is disabled.
The proposed patch fixes the JSC build and fixes WebCore build too if ENABLE(INSPECTOR) is also turned off, because inspector always needs the JavaScript debugger.
Comment 1 Peter Molnar 2013-12-02 10:52:27 PST
Created attachment 218188 [details]
Proposed patch
Comment 2 WebKit Commit Bot 2013-12-02 10:54:33 PST
Attachment 218188 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp', u'Source/JavaScriptCore/debugger/Debugger.cpp', u'Source/JavaScriptCore/debugger/Debugger.h', u'Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp', u'Source/JavaScriptCore/debugger/DebuggerCallFrame.h', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/jit/JITOpcodes.cpp', u'Source/JavaScriptCore/llint/LLIntOfflineAsmConfig.h', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h']" exit_code: 1
Source/JavaScriptCore/debugger/DebuggerCallFrame.h:89:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 12 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Peter Molnar 2013-12-02 10:56:02 PST
Created attachment 218192 [details]
Proposed patch - fixed style
Comment 4 Mark Lam 2013-12-02 11:30:36 PST
Comment on attachment 218192 [details]
Proposed patch - fixed style

We can avoid adding all these #if ENABLE(JAVASCRIPT_DEBUGGER) if we just add a stub Debugger class in Debugger.h.  Something like this:


class JS_EXPORT_PRIVATE Debugger {


class Debugger {
    Debugger(bool = false) { }
    ... // More stubbed functions as needed.


If done right, the performance and memory usage difference should be negligible (if at all), and the code would be more readable due to not having #ifs scattered in.

Can you please try that approach and submit another patch?
Comment 5 Peter Molnar 2013-12-03 05:22:58 PST
Created attachment 218288 [details]
Updated patch
Comment 6 Mark Lam 2013-12-03 10:40:56 PST
Comment on attachment 218288 [details]
Updated patch

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

> Source/JavaScriptCore/debugger/Debugger.h:201
> +    bool m_needsOpDebugCallbacks;
> +    bool needsOpDebugCallbacks() const { return m_needsOpDebugCallbacks; }
> +    static ptrdiff_t needsOpDebugCallbacksOffset() { return 0; }

Good job, but need a little more work.  Here are some issues (due to correctness):
1. m_needsOpDebugCallbacks is uninitialized.  It should be initialized to false.
2. needsOpDebugCallbacksOffset() should return OBJECT_OFFSETOF(Debugger, m_needsOpDebugCallbacks) instead of assuming and hardcoding it to be 0.

In practice, these issues will probably not manifest any problems because the code paths which uses these should never be executed for !ENABLE(JAVASCRIPT_DEBUGGER) builds.  That said, it is still not good to leave the code in this state. 

That said, this is how I would like you to update this patch instead:

1. Remove m_needsOpDebugCallbacks and needsOpDebugCallbacksOffset().
2. Change needsOpDebugCallbacks() to always return false.
3. Add back the "if JAVASCRIPT_DEBUGGER” in LowLevelInterpreter.asm along with the changes in llint/LLIntOfflineAsmConfig.h.
4. Add back the "#elif ENABLE(JAVASCRIPT_DEBUGGER)” in JITOpcode.cpp and JITOpcode32_64.cpp.

I understand that m_needsOpDebugCallbacks was needed because the LLINT’s LowLevelInterpreter.asm makes a reference to it (and there wasn’t a way to stub that out).  This is one case where the use of this stub Debugger class proved to be inadequate as the sole mechanism to fix this !ENABLE(JAVASCRIPT_DEBUGGER) build issue.  So, it is ok to use the #if there to deal with it.  And might as well do the same for JITOpcode.cpp and JITOpcode32_64.cpp which is analogous to the LowLevelInterpreter.asm code.  I think the code is more clear this way (than with using the dummy m_needsOpDebugCallbacks).

Can you please update the patch with these changes?  Thanks.
Comment 7 Peter Molnar 2013-12-04 00:56:53 PST
Created attachment 218387 [details]

Updated the patch.
Comment 8 Mark Lam 2013-12-04 01:03:51 PST
Comment on attachment 218387 [details]

Comment 9 WebKit Commit Bot 2013-12-04 01:55:02 PST
Comment on attachment 218387 [details]

Clearing flags on attachment: 218387

Committed r160082: <http://trac.webkit.org/changeset/160082>
Comment 10 WebKit Commit Bot 2013-12-04 01:55:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Darin Adler 2013-12-04 11:53:45 PST
Comment on attachment 218387 [details]

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

> Source/JavaScriptCore/debugger/Debugger.h:209
> +    void detach(JSGlobalObject*) { };
> +    void sourceParsed(ExecState*, SourceProvider*, int, const WTF::String&) { };
> +    void exception(CallFrame*, JSValue, bool) { };
> +    void atStatement(CallFrame*) { };
> +    void callEvent(CallFrame*) { };
> +    void returnEvent(CallFrame*) { };
> +    void willExecuteProgram(CallFrame*) { };
> +    void didExecuteProgram(CallFrame*) { };
> +    void didReachBreakpoint(CallFrame*) { };

Lots of unneeded semicolons here.
Comment 12 Mark Lam 2013-12-04 12:38:38 PST
(In reply to comment #11)
> Lots of unneeded semicolons here.

Thanks for catching that.  Semicolons removed in r160115: <http://trac.webkit.org/r160115>.