Bug 125083 – Fix !ENABLE(JAVASCRIPT_DEBUGGER) build

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Peter Molnar]

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.

[Peter Molnar]

Created attachment 218188 [details]
Proposed patch

[WebKit Commit Bot]

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.

[Peter Molnar]

Created attachment 218192 [details]
Proposed patch - fixed style

[Mark Lam]

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:

#if ENABLE(JAVASCRIPT_DEBUGGER)

class JS_EXPORT_PRIVATE Debugger {
    ...
};

#else // ENABLE(JAVASCRIPT_DEBUGGER)

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

#endif // ENABLE(JAVASCRIPT_DEBUGGER)

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?

[Peter Molnar]

Created attachment 218288 [details]
Updated patch

[Mark Lam]

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.

[Peter Molnar]

Created attachment 218387 [details]
Patch

Updated the patch.

[Mark Lam]

Comment on attachment 218387 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 218387 [details]
Patch

Clearing flags on attachment: 218387

Committed r160082: <http://trac.webkit.org/changeset/160082>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 218387 [details]
Patch

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.

[Mark Lam]

(In reply to comment #11)
> Lots of unneeded semicolons here.

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