Bug 175449 – Implement 32-bit MacroAssembler::probe support for Windows.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

This is needed in order to enable the DFG.

[Radar WebKit Bug Importer]

<rdar://problem/33837002>

[Mark Lam]

What needs to be done:
1. Copy the implementation of ctiMasmProbeTrampoline in MacroAssemblerX86Common.cpp into a .S file.  Note: there's a 32-bit and 64-bit version.
2. Make sure that the COMPILE_ASSERT and static_asserts are in effect.
   This is needed to ensure that the asm code will populate the ProbeContext the way that C++ code expect it to.
3. Convert the inline asm instructions into MSVC asm.
4. Change the part for calling a C function in ctiMasmProbeTrampoline to follow Windows' calling convention.
5. Run the testmasm test to validate the fix works before committing.

[Per Arne Vollan]

Created attachment 318152 [details]
Patch

[Mark Lam]

Comment on attachment 318152 [details]
Patch

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

I still have to audit your inline asm, but here are comments for now.  Also, please rebase your patch so that it can apply to trunk.  Thanks.

> Source/JavaScriptCore/ChangeLog:3
> +        Implement MacroAssembler::probe support for Windows.

If you're only going to do 32-bit in this patch, please update the bug name and ChangeLog title to say: "Implement 32-bit MacroAssembler::probe support for Windows."  And file a new bug for the 64-bit version.

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:108
> +#if !COMPILER(MSVC) // Compile error with MSVC

Why is this a compilation error on MSVC?  What does the error look like?  These static assertions are important to make sure that the inline asm matches up with what C++ code expects.

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:329
> +#if COMPILER(MSVC)

Don't you have to add && CPU(X86) here?

[Per Arne Vollan]

(In reply to Mark Lam from comment #4)
> Comment on attachment 318152 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318152&action=review
> 
> I still have to audit your inline asm, but here are comments for now.  Also,
> please rebase your patch so that it can apply to trunk.  Thanks.
> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        Implement MacroAssembler::probe support for Windows.
> 
> If you're only going to do 32-bit in this patch, please update the bug name
> and ChangeLog title to say: "Implement 32-bit MacroAssembler::probe support
> for Windows."  And file a new bug for the 64-bit version.
> 

Will do.

> > Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:108
> > +#if !COMPILER(MSVC) // Compile error with MSVC
> 
> Why is this a compilation error on MSVC?  What does the error look like? 
> These static assertions are important to make sure that the inline asm
> matches up with what C++ code expects.
> 

MSVC error:
"error C2131: expression did not evaluate to a constant"
"note: failure was caused by unevaluable pointer value"

I tested it with runtime ASSERTs, and they all passed, but compile asserts would be best.

> > Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:329
> > +#if COMPILER(MSVC)
> 
> Don't you have to add && CPU(X86) here?

I think this already is inside CPU(X86).

Thanks for reviewing!

[Mark Lam]

Comment on attachment 318152 [details]
Patch

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

>>> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:108
>>> +#if !COMPILER(MSVC) // Compile error with MSVC
>> 
>> Why is this a compilation error on MSVC?  What does the error look like?  These static assertions are important to make sure that the inline asm matches up with what C++ code expects.
> 
> MSVC error:
> "error C2131: expression did not evaluate to a constant"
> "note: failure was caused by unevaluable pointer value"
> 
> I tested it with runtime ASSERTs, and they all passed, but compile asserts would be best.

Can you play with it a bit to find out why MSVC thinks that the expression is not constant?

>>> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:329
>>> +#if COMPILER(MSVC)
>> 
>> Don't you have to add && CPU(X86) here?
> 
> I think this already is inside CPU(X86).
> 
> Thanks for reviewing!

Oh, you're right.

[Per Arne Vollan]

(In reply to Mark Lam from comment #6)
> Comment on attachment 318152 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318152&action=review
> 
> >>> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:108
> >>> +#if !COMPILER(MSVC) // Compile error with MSVC
> >> 
> >> Why is this a compilation error on MSVC?  What does the error look like?  These static assertions are important to make sure that the inline asm matches up with what C++ code expects.
> > 
> > MSVC error:
> > "error C2131: expression did not evaluate to a constant"
> > "note: failure was caused by unevaluable pointer value"
> > 
> > I tested it with runtime ASSERTs, and they all passed, but compile asserts would be best.
> 
> Can you play with it a bit to find out why MSVC thinks that the expression
> is not constant?
> 

Yes, I will look into this :)

[Per Arne Vollan]

Created attachment 318396 [details]
Patch

[Per Arne Vollan]

Created attachment 318402 [details]
Patch

[Per Arne Vollan]

Created attachment 318405 [details]
Patch

[Mark Lam]

Comment on attachment 318405 [details]
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.

[Per Arne Vollan]

Created attachment 318522 [details]
Patch

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #12)
> Created attachment 318522 [details]
> Patch

Just testing rebased patch on EWS.

[Build Bot]

Attachment 318522 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Platform.h:801:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:350:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:353:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:354:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:355:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:356:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:357:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:360:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:362:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:364:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:366:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:368:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:370:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:382:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:387:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:389:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:412:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:428:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:468:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:470:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:472:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:474:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:476:  Extra space before [.  [whitespace/brackets] [5]
Total errors found: 23 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

Created attachment 318524 [details]
Patch

[Build Bot]

Attachment 318524 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Platform.h:801:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:350:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:353:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:354:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:355:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:356:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:357:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:360:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:362:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:364:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:366:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:368:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:370:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:382:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:387:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:389:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:412:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:428:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:468:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:470:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:472:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:474:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp:476:  Extra space before [.  [whitespace/brackets] [5]
Total errors found: 23 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

Thanks for reviewing!

[Per Arne Vollan]

Committed r220926: <https://trac.webkit.org/changeset/220926/webkit>