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.
Created attachment 218188 [details] Proposed patch
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.
Created attachment 218192 [details] Proposed patch - fixed style
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?
Created attachment 218288 [details] Updated patch
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.
Created attachment 218387 [details] Patch Updated the patch.
Comment on attachment 218387 [details] Patch r=me
Comment on attachment 218387 [details] Patch Clearing flags on attachment: 218387 Committed r160082: <http://trac.webkit.org/changeset/160082>
All reviewed patches have been landed. Closing bug.
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.
(In reply to comment #11) > Lots of unneeded semicolons here. Thanks for catching that. Semicolons removed in r160115: <http://trac.webkit.org/r160115>.