[webkit-reviews] review denied: [Bug 127902] Fix the remaining regression caused by the jsCStack branch merge on Linux platforms : [Attachment 231696] Patch

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has denied Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 127902: Fix the remaining regression caused by the jsCStack branch merge on
Linux platforms
https://bugs.webkit.org/show_bug.cgi?id=127902

Attachment 231696: Patch
https://bugs.webkit.org/attachment.cgi?id=231696&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=231696&action=review


Pretty close.  Just a few items.

> Source/WTF/wtf/StackBounds.cpp:103
> +// In the case of really big stacks we only initialize first 256MB of it.
Such sizes are achievable if the stack

256MB seems huge.  Shouldn't this be Options::maxPerThreadStackUsage?

> Source/WTF/wtf/StackBounds.cpp:112
> +#if COMPILER(GCC) && !COMPILER(CLANG)
> +#define UNOPTIMIZED __attribute__((optimize("O0")))
> +#else
> +#define UNOPTIMIZED
> +#endif

Please add a FIXME here, referencing a new webkit bug and possibly a gcc bug
number?

> Source/WTF/wtf/StackBounds.cpp:116
> +// builds by using the GCC-specific attribute. Passing a reference into this
function prevents both GCC and Clang to
> +// optimize away the first initializeStack() call in
StackBounds::initialize().

Change "to optimize" to "from optimizing"

> Source/WTF/wtf/StackBounds.cpp:123
> +	   stackInitialized = true;

Why can't this just be a return?  If it is a return, then I don't think we need
"stackInitialized" except maybe to prevent the compiler optimizing away issue
noted above.