[webkit-reviews] review denied: [Bug 127071] CStack Branch: X86-32 Fix LLInt : [Attachment 221432] Updated patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Michael Saboff <msaboff at apple.com>'s
request for review:
Bug 127071: CStack Branch: X86-32 Fix LLInt
https://bugs.webkit.org/show_bug.cgi?id=127071

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

------- Additional Comments from Mark Lam <mark.lam at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=221432&action=review


> Source/JavaScriptCore/assembler/MaxFrameExtentForSlowPathCall.h:50
>  // 6 args on stack (24 bytes) + 8 bytes to align the stack.

This comment is out of date.

> Source/JavaScriptCore/interpreter/ProtoCallFrame.cpp:49
> -    paddedArgsCount = WTF::roundUpToMultipleOf(stackAlignmentRegisters(),
paddedArgsCount);
> -    this->setPaddedArgsCount(paddedArgsCount);
> +    paddedArgsCount = roundArgumentCountToAlignFrame(paddedArgsCount);
> +    this->setPaddedArgCount(paddedArgsCount);

I find roundArgumentCountToAlignFrame() to be unclear as to what it’s trying to
do here.  I’ll upload a patch shortly with a different way to compute this
which I think is more clear about the intent.

> Source/JavaScriptCore/llint/LLIntEntrypoint.cpp:122
> -    size_t registerCount = codeBlock->m_numCalleeRegisters +
maxFrameExtentForSlowPathCallInRegisters;
> +    size_t registerCount = codeBlock->m_numCalleeRegisters;
>      ASSERT(registerCount ==
WTF::roundUpToMultipleOf(stackAlignmentRegisters(), registerCount));
> +    registerCount += maxFrameExtentForSlowPathCallInRegisters;

This weakens the assertion that was previously there.  I think we can do
better.  I’ll upload a patch shortly for my proposal on how.

> Source/JavaScriptCore/runtime/StackAlignment.h:48
> +// Align argument count taking into account the CallFrameHeaderSize may be
unaligned
> +inline unsigned roundArgumentCountToAlignFrame(unsigned argumentCount)
> +{
> +    unsigned callFrameHeaderSizeBias = JSStack::CallFrameHeaderSize % 2;
> +    return WTF::roundUpToMultipleOf(stackAlignmentRegisters(), argumentCount
+ callFrameHeaderSizeBias) - callFrameHeaderSizeBias;
> +}

This function is confusing to me.  2 main issues:
1. Why % 2?  Why not % stackAlignmentRegisters()?  I think this logic will
break if stackAlignmentRegisters() is not 2.
2. We’re not clear about the end goal of this alignment exercise?  Hence, it is
hard to tell whether we’re doing it right.

I’ll propose an alternative in a patch I’ll upload shortly.