[webkit-reviews] review granted: [Bug 175449] Implement 32-bit MacroAssembler::probe support for Windows. : [Attachment 318405] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Per Arne Vollan <pvollan at apple.com>'s
request for review:
Bug 175449: Implement 32-bit MacroAssembler::probe support for Windows.
https://bugs.webkit.org/show_bug.cgi?id=175449

Attachment 318405: Patch

https://bugs.webkit.org/attachment.cgi?id=318405&action=review




--- Comment #11 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 318405
  --> https://bugs.webkit.org/attachment.cgi?id=318405
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318405&action=review

r=me with remaining issues resolved before landing.

Since this fixes the masm probe for 32-bit x86 on Windows, I suggest that you
re-enable the DFG for Windows in Platform.h as follows:
#if CPU(X86) && OS(WINDOWS)
#define ENABLE_DFG_JIT 1
#endif

Do this in the section of code with the FIXME comment about
https://bugs.webkit.org/show_bug.cgi?id=175449.  You should also replace that
FIXME comment with the one for your 64-bit fix.

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:351
> +	   mov[PROBE_CPU_EBP_OFFSET + esp], ebp

Please put a space after mov.  Same for all "mov[..." below ... unless this
funky "mov[" syntax is required by MSVC.  Is it required?

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:390
> +	   call[PROBE_PROBE_FUNCTION_OFFSET + ebp]

Ditto.	Can we add a space after call?

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:464
> +

Please remove the extra line here.

> Source/JavaScriptCore/assembler/testmasm.cpp:694
> -	   return !filter || !!strcasestr(testName, filter);
> +	   return !filter || !!strstr(testName, filter);

Let's express this as:
#if OS(UNIX)
    return !filter || !!strcasestr(testName, filter);
#else
    return !filter || !!strstr(testName, filter);
#endif

It's good to be able to keep the extra functionality on platforms that can
support it.