Bug 175449 - Implement 32-bit MacroAssembler::probe support for Windows.
Summary: Implement 32-bit MacroAssembler::probe support for Windows.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-10 15:18 PDT by Mark Lam
Modified: 2017-08-18 12:22 PDT (History)
6 users (show)

See Also:


Attachments
Patch (14.00 KB, patch)
2017-08-15 12:52 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (17.44 KB, patch)
2017-08-17 12:03 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (17.39 KB, patch)
2017-08-17 12:48 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (22.08 KB, patch)
2017-08-17 12:55 PDT, Per Arne Vollan
mark.lam: review+
Details | Formatted Diff | Diff
Patch (23.20 KB, patch)
2017-08-18 11:27 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (23.20 KB, patch)
2017-08-18 11:36 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2017-08-10 15:18:51 PDT
This is needed in order to enable the DFG.
Comment 1 Radar WebKit Bug Importer 2017-08-10 15:19:40 PDT
<rdar://problem/33837002>
Comment 2 Mark Lam 2017-08-11 11:02:52 PDT
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.
Comment 3 Per Arne Vollan 2017-08-15 12:52:01 PDT
Created attachment 318152 [details]
Patch
Comment 4 Mark Lam 2017-08-15 13:00:49 PDT
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?
Comment 5 Per Arne Vollan 2017-08-15 13:09:56 PDT
(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!
Comment 6 Mark Lam 2017-08-15 13:12:36 PDT
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.
Comment 7 Per Arne Vollan 2017-08-15 13:17:44 PDT
(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 :)
Comment 8 Per Arne Vollan 2017-08-17 12:03:14 PDT
Created attachment 318396 [details]
Patch
Comment 9 Per Arne Vollan 2017-08-17 12:48:46 PDT
Created attachment 318402 [details]
Patch
Comment 10 Per Arne Vollan 2017-08-17 12:55:53 PDT
Created attachment 318405 [details]
Patch
Comment 11 Mark Lam 2017-08-18 07:52:10 PDT
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.
Comment 12 Per Arne Vollan 2017-08-18 11:27:42 PDT
Created attachment 318522 [details]
Patch
Comment 13 Per Arne Vollan 2017-08-18 11:28:41 PDT
(In reply to Per Arne Vollan from comment #12)
> Created attachment 318522 [details]
> Patch

Just testing rebased patch on EWS.
Comment 14 Build Bot 2017-08-18 11:30:02 PDT
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.
Comment 15 Per Arne Vollan 2017-08-18 11:36:50 PDT
Created attachment 318524 [details]
Patch
Comment 16 Build Bot 2017-08-18 11:38:01 PDT
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.
Comment 17 Per Arne Vollan 2017-08-18 12:21:17 PDT
Thanks for reviewing!
Comment 18 Per Arne Vollan 2017-08-18 12:22:01 PDT
Committed r220926: <https://trac.webkit.org/changeset/220926/webkit>