[webkit-reviews] review denied: [Bug 125083] Fix !ENABLE(JAVASCRIPT_DEBUGGER) build : [Attachment 218288] Updated patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Peter Molnar
<pmolnar.u-szeged at partner.samsung.com>'s request for review:
Bug 125083: Fix !ENABLE(JAVASCRIPT_DEBUGGER) build
https://bugs.webkit.org/show_bug.cgi?id=125083

Attachment 218288: Updated patch
https://bugs.webkit.org/attachment.cgi?id=218288&action=review

------- Additional Comments from Mark Lam <mark.lam at apple.com>
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.