[Webkit-unassigned] [Bug 125083] Fix !ENABLE(JAVASCRIPT_DEBUGGER) build

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=125083


Mark Lam <mark.lam at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #218288|review?                     |review-
               Flag|                            |




--- Comment #6 from Mark Lam <mark.lam at apple.com>  2013-12-03 10:40:56 PST ---
(From update of attachment 218288)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.