Bug 175446

Summary: Make the MASM_PROBE mechanism mandatory for DFG and FTL builds.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, benjamin, bugs-noreply, buildbot, cdumez, clopez, cmarcelo, commit-queue, dbates, guijemont, keith_miller, msaboff, ossy, pvollan, ryanhaddad, saam, simon.fraser, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 175514    
Bug Blocks: 174645    
Attachments:
Description Flags
proposed patch.
none
proposed patch.
none
proposed patch.
none
proposed patch.
saam: review+
patch for landing w/ Windows fix. none

Description Mark Lam 2017-08-10 15:01:43 PDT 
This is needed in order to support https://bugs.webkit.org/show_bug.cgi?id=174645.

Once consequence of this is that the DFG will now be disabled for the MIPS port.
Comment 1 Radar WebKit Bug Importer 2017-08-10 15:02:16 PDT 
<rdar://problem/33836545>
Comment 2 Mark Lam 2017-08-10 15:16:31 PDT 
Looks like I have to disable it for Windows as well.
Comment 4 Mark Lam 2017-08-10 15:39:56 PDT 
Created attachment 317862 [details]
proposed patch.

Let's try this on the EWS.
Comment 5 Build Bot 2017-08-10 15:41:52 PDT 
Attachment 317862 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Platform.h:785:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
ERROR: Source/WTF/wtf/Platform.h:788:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Mark Lam 2017-08-10 15:57:59 PDT 
Created attachment 317864 [details]
proposed patch.
Comment 7 Build Bot 2017-08-10 16:00:01 PDT 
Attachment 317864 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Platform.h:785:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
ERROR: Source/WTF/wtf/Platform.h:788:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Mark Lam 2017-08-10 16:30:20 PDT 
Created attachment 317877 [details]
proposed patch.
Comment 9 Build Bot 2017-08-10 16:32:25 PDT 
Attachment 317877 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/MacroAssemblerPrinter.cpp:55:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerPrinter.cpp:57:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Platform.h:785:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
ERROR: Source/WTF/wtf/Platform.h:788:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
Total errors found: 4 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Mark Lam 2017-08-10 17:18:45 PDT 
Created attachment 317892 [details]
proposed patch.
Comment 11 Build Bot 2017-08-10 17:20:00 PDT 
Attachment 317892 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/MacroAssemblerPrinter.cpp:55:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerPrinter.cpp:57:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Platform.h:785:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
ERROR: Source/WTF/wtf/Platform.h:788:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Mark Lam 2017-08-10 19:31:26 PDT 
Created attachment 317903 [details]
patch for landing w/ Windows fix.

Thanks for the review.
Comment 13 Build Bot 2017-08-10 19:34:12 PDT 
Attachment 317903 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/assembler/MacroAssemblerPrinter.cpp:55:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/MacroAssemblerPrinter.cpp:57:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Platform.h:785:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
ERROR: Source/WTF/wtf/Platform.h:788:  CPP comments are not allowed in Platform.h, please use C comments /* ... */  [build/cpp_comment] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 WebKit Commit Bot 2017-08-10 22:31:35 PDT 
Comment on attachment 317903 [details]
patch for landing w/ Windows fix.

Clearing flags on attachment: 317903

Committed r220579: <http://trac.webkit.org/changeset/220579>
Comment 15 WebKit Commit Bot 2017-08-10 22:31:37 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 16 Carlos Alberto Lopez Perez 2017-08-11 03:39:40 PDT 
(In reply to WebKit Commit Bot from comment #14)
> Comment on attachment 317903 [details]
> patch for landing w/ Windows fix.
> 
> Clearing flags on attachment: 317903
> 
> Committed r220579: <http://trac.webkit.org/changeset/220579>

It seems this broke the build on WebKitGTK+ x86 (32-bits):

/tmp/cc52ttH2.s: Assembler messages:
/tmp/cc52ttH2.s:6: Error: no such instruction: `pushfd'
/tmp/cc52ttH2.s:88: Error: no such instruction: `popfd'

https://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/3262/steps/compile-webkit/logs/stdio/text
Comment 17 Mark Lam 2017-08-11 07:32:04 PDT 
(In reply to Carlos Alberto Lopez Perez from comment #16)
> (In reply to WebKit Commit Bot from comment #14)
> > Comment on attachment 317903 [details]
> > patch for landing w/ Windows fix.
> > 
> > Clearing flags on attachment: 317903
> > 
> > Committed r220579: <http://trac.webkit.org/changeset/220579>
> 
> It seems this broke the build on WebKitGTK+ x86 (32-bits):
> 
> /tmp/cc52ttH2.s: Assembler messages:
> /tmp/cc52ttH2.s:6: Error: no such instruction: `pushfd'
> /tmp/cc52ttH2.s:88: Error: no such instruction: `popfd'
> 
> https://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/3262/
> steps/compile-webkit/logs/stdio/text

According to https://en.wikipedia.org/wiki/X86_instruction_listings, pushfd and popfd have been valid x86 instructions since the 80386.  This looks like a gad (or whatever assembler you're using) issue.  I don't have access to a GTK machine.  Can you or someone on the GTK side look into what the issue is and file a new bug to fix it?  Please cc me so that I can review the fix.  Thanks.
Comment 18 Yusuke Suzuki 2017-08-11 08:44:56 PDT 
Committed r220592: <http://trac.webkit.org/changeset/220592>
Comment 19 Ryan Haddad 2017-08-11 09:32:58 PDT 
(In reply to WebKit Commit Bot from comment #14)
> Comment on attachment 317903 [details]
> patch for landing w/ Windows fix.
> 
> Clearing flags on attachment: 317903
> 
> Committed r220579: <http://trac.webkit.org/changeset/220579>
This change also broke CLoop and Windows Debug builds:
https://build.webkit.org/builders/Apple%20Sierra%20LLINT%20CLoop%20%28BuildAndTest%29/builds/4349

https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/3281
Comment 20 Mark Lam 2017-08-11 09:55:37 PDT 
CLoop build fix landed in r220600: <http://trac.webkit.org/r220600>.
Comment 21 Csaba Osztrogonác 2017-08-12 04:44:44 PDT 
FYI, it broke the AArch64 Linux build. MacroAssemblerARM64.cpp wasn't added
to the cmake build system and after adding it, I got the following error:

{standard input}:34: Error: operand 2 should be an integer register -- `stp x28,fp,[sp,#((2*8)+(28*8))]'
{standard input}:37: Error: operand 1 should be an integer register -- `str lr,[sp,#((((2*8)+(35*8))+(32*8))+(0*8))]'
{standard input}:38: Error: operand 1 should be an integer or stack pointer register -- `add lr,lr,#2*8'
{standard input}:39: Error: operand 1 should be an integer register -- `str lr,[sp,#((2*8)+(32*8))]'
{standard input}:94: Error: operand 1 should be an integer register -- `mov lr,#0'
{standard input}:104: Error: operand 1 should be an integer register -- `mov lr,#1'
{standard input}:106: Error: operand 1 should be an integer register -- `ldr fp,[sp,#((((2*8)+(35*8))+(32*8))+(0*8))]'
{standard input}:114: Error: operand 1 should be an integer register -- `str lr,[sp,#((2*8)+(0*8))]'
{standard input}:115: Error: operand 2 should be an integer register -- `ldp x28,lr,[x27,#((2*8)+(27*8))]'
{standard input}:116: Error: operand 2 should be an integer register -- `stp x28,lr,[sp,#((2*8)+(27*8))]'
{standard input}:117: Error: operand 2 should be an integer register -- `ldp x28,lr,[x27,#((2*8)+(29*8))]'
{standard input}:118: Error: operand 2 should be an integer register -- `stp x28,lr,[sp,#((2*8)+(29*8))]'
{standard input}:119: Error: operand 2 should be an integer register -- `ldp x28,lr,[x27,#((2*8)+(31*8))]'
{standard input}:120: Error: operand 2 should be an integer register -- `stp x28,lr,[sp,#((2*8)+(31*8))]'
{standard input}:121: Error: operand 2 should be an integer register -- `ldp x28,lr,[x27,#((2*8)+(33*8))]'
{standard input}:122: Error: operand 2 should be an integer register -- `stp x28,lr,[sp,#((2*8)+(33*8))]'
{standard input}:123: Error: operand 1 should be an integer register -- `ldr lr,[sp,#((2*8)+(0*8))]'
{standard input}:125: Error: operand 1 should be an integer register -- `cbnz lr,.LctiMasmProbeTrampolineEnd'
{standard input}:126: Error: operand 1 should be an integer register -- `ldr lr,[sp,#((2*8)+(31*8))]'
{standard input}:127: Error: operand 1 should be an integer or stack pointer register -- `sub lr,lr,#(6*8)'
{standard input}:129: Error: integer 64-bit register expected at operand 2 -- `str x27,[lr,#(5*8)]'
{standard input}:130: Error: operand 1 should be an integer register -- `str lr,[sp,#((2*8)+(31*8))]'
{standard input}:131: Error: operand 1 should be an integer register -- `str fp,[sp,#((2*8)+(32*8))]'
{standard input}:133: Error: operand 1 should be an integer register -- `ldr lr,[sp,#((2*8)+(31*8))]'
{standard input}:134: Error: operand 1 should be an integer or stack pointer register -- `sub lr,lr,#(6*8)'
{standard input}:136: Error: integer 64-bit register expected at operand 3 -- `stp x27,x28,[lr,#(0*8)]'
{standard input}:138: Error: integer 64-bit register expected at operand 3 -- `stp x27,x28,[lr,#(2*8)]'
{standard input}:141: Error: integer 64-bit register expected at operand 3 -- `stp x27,x28,[lr,#(4*8)]'
{standard input}:147: Error: operand 1 should be a floating-point register -- `ldp fp,lr,[sp],#2*8'
Comment 22 Csaba Osztrogonác 2017-08-12 04:45:36 PDT 
note: we don't have ARM buildbots for a while because of bug174993
Comment 23 Csaba Osztrogonác 2017-08-12 05:00:32 PDT 
and it broke the ARMv7 (traditional) build too:
../../Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:203:1: error: static assertion failed: ProbeContext_size_matches_ctiMasmProbeTrampoline
Comment 24 Csaba Osztrogonác 2017-08-12 05:04:56 PDT 
and it broke the GTK's ARM build too (maybe ARMv7 Thumb2):

/tmp/cco6SOuf.s: Assembler messages:
/tmp/cco6SOuf.s:48: Error: VFP single precision register expected -- `vstmia.64 ip!,{ d16-d31 }'
/tmp/cco6SOuf.s:55: Error: VFP single precision register expected -- `vldmdb.64 ip!,{ d16-d31 }'
/tmp/cco6SOuf.s:88: writing to APSR without specifying a bitmask is deprecated
Comment 25 Mark Lam 2017-08-12 08:33:27 PDT 
(In reply to Csaba Osztrogonác_OOO_until_21st_Aug from comment #21)
> FYI, it broke the AArch64 Linux build. MacroAssemblerARM64.cpp wasn't added
> to the cmake build system and after adding it, I got the following error:
...

Will fix in https://bugs.webkit.org/show_bug.cgi?id=175512
Comment 26 Mark Lam 2017-08-12 10:36:20 PDT 
(In reply to Csaba Osztrogonác_OOO_until_21st_Aug from comment #23)
> and it broke the ARMv7 (traditional) build too:
> ../../Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:203:1: error:
> static assertion failed: ProbeContext_size_matches_ctiMasmProbeTrampoline

Let's address this in https://bugs.webkit.org/show_bug.cgi?id=175513.
Comment 27 Mark Lam 2017-08-12 10:57:29 PDT 
(In reply to Csaba Osztrogonác_OOO_until_21st_Aug from comment #24)
> and it broke the GTK's ARM build too (maybe ARMv7 Thumb2):
> 
> /tmp/cco6SOuf.s: Assembler messages:
> /tmp/cco6SOuf.s:48: Error: VFP single precision register expected --
> `vstmia.64 ip!,{ d16-d31 }'
> /tmp/cco6SOuf.s:55: Error: VFP single precision register expected --
> `vldmdb.64 ip!,{ d16-d31 }'
> /tmp/cco6SOuf.s:88: writing to APSR without specifying a bitmask is
> deprecated

Let's address these in https://bugs.webkit.org/show_bug.cgi?id=175514.
Comment 28 Carlos Alberto Lopez Perez 2017-08-13 05:08:10 PDT 
(In reply to Csaba Osztrogonác_OOO_until_21st_Aug from comment #24)
> and it broke the GTK's ARM build too (maybe ARMv7 Thumb2):
> 
> /tmp/cco6SOuf.s: Assembler messages:
> /tmp/cco6SOuf.s:48: Error: VFP single precision register expected --
> `vstmia.64 ip!,{ d16-d31 }'
> /tmp/cco6SOuf.s:55: Error: VFP single precision register expected --
> `vldmdb.64 ip!,{ d16-d31 }'
> /tmp/cco6SOuf.s:88: writing to APSR without specifying a bitmask is
> deprecated

GTK+ ARMv7 build is Thumb2.

Is JSCOnly thumb2 working?
Comment 29 Csaba Osztrogonác 2017-08-13 06:43:11 PDT 
(In reply to Carlos Alberto Lopez Perez from comment #28)
> (In reply to Csaba Osztrogonác_OOO_until_21st_Aug from comment #24)
> > and it broke the GTK's ARM build too (maybe ARMv7 Thumb2):
> > 
> > /tmp/cco6SOuf.s: Assembler messages:
> > /tmp/cco6SOuf.s:48: Error: VFP single precision register expected --
> > `vstmia.64 ip!,{ d16-d31 }'
> > /tmp/cco6SOuf.s:55: Error: VFP single precision register expected --
> > `vldmdb.64 ip!,{ d16-d31 }'
> > /tmp/cco6SOuf.s:88: writing to APSR without specifying a bitmask is
> > deprecated
> 
> GTK+ ARMv7 build is Thumb2.
> 
> Is JSCOnly thumb2 working?

Yes, it works fine:
https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/builds/1428

We use GCC 5.2.0 on it (custom cross compiler built with crosstool-ng) 
with the following options set by default:
-mthumb
-mthumb-interwork 
-march=armv7-a
-mtune=cortex-a15
-mfpu=neon-vfpv4
Comment 30 Mark Lam 2017-08-14 09:43:12 PDT 
Speculative fix for Windows build landed in r220701: <http://trac.webkit.org/r220701>.
Comment 31 Simon Fraser (smfr) 2017-08-16 15:30:02 PDT 
Looks like this broke the GTK build: https://build.webkit.org/builders/GTK%20Linux%20ARM%20Release/builds/855
Comment 32 Mark Lam 2017-08-16 16:31:07 PDT 
(In reply to Simon Fraser (smfr) from comment #31)
> Looks like this broke the GTK build:
> https://build.webkit.org/builders/GTK%20Linux%20ARM%20Release/builds/855

This issue is tracked in https://bugs.webkit.org/show_bug.cgi?id=175514.

For now, to green the bot, I've disabled the DFG for GTK ARM_THUMB2 in r220816: <http://trac.webkit.org/r220816>.