Bug 186283

Summary: REGRESSION: (r232114 - 232120): Multiple JSC Stress test failures on 32 bit architecture
Product: WebKit Reporter: Dawei Fenton (:realdawei) <realdawei>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: REOPENED ---    
Severity: Normal CC: commit-queue, lforschler, mark.lam, ryanhaddad, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Skip the tests. none

Description Dawei Fenton (:realdawei) 2018-06-04 13:50:36 PDT
A set of JSC stress test failures have been occurring since May 23.

https://build.webkit.org/builders/Apple%20High%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/1978/steps/webkit-32bit-jsc-test/logs/stdio

** The following JSC stress test failures have been introduced:
	executableAllocationFuzz.yaml/executableAllocationFuzz/v8-raytrace.js.executable-allocation-fuzz-default
	executableAllocationFuzz.yaml/executableAllocationFuzz/v8-raytrace.js.executable-allocation-fuzz-no-cjit
	slowMicrobenchmarks.yaml/slowMicrobenchmarks/default-derived-constructor.js.default
	slowMicrobenchmarks.yaml/slowMicrobenchmarks/default-derived-constructor.js.no-cjit
	stress/put-direct-index-broken-2.js.dfg-maximal-flush-validate-no-cjit
	stress/splay-flash-access.js.default
	stress/splay-flash-access.js.ftl-no-cjit
	stress/spread-forward-call-varargs-stack-overflow.js.dfg-maximal-flush-validate-no-cjit
Comment 1 Dawei Fenton (:realdawei) 2018-06-05 11:40:44 PDT
Created attachment 341981 [details]
Patch
Comment 2 Mark Lam 2018-06-05 11:52:19 PDT
Comment on attachment 341981 [details]
Patch

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

> JSTests/stress/splay-flash-access.js:1
> +//@skip if $architecture == "x86"

I don't think you should drop the old condition.  This should be //@skip if $memoryLimited or $architecture == "x86"
Comment 3 Ryan Haddad 2018-06-05 11:54:33 PDT
Comment on attachment 341981 [details]
Patch

My understanding was that the goal was to skip these on the 32-bit JSC bot. This will skip the tests on 64 bit.
Comment 4 Ryan Haddad 2018-06-05 11:59:06 PDT
Comment on attachment 341981 [details]
Patch

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

> JSTests/executableAllocationFuzz.yaml:26
> +      if ($hostOS == "windows" || $architecture == "x86")

I believe this should be "i386", not "x86".

> JSTests/slowMicrobenchmarks/default-derived-constructor.js:1
> +//@skip if $architecture == "x86"

Ditto.

> JSTests/stress/put-direct-index-broken-2.js:1
> +//@skip if $architecture == "x86"

Ditto.

>> JSTests/stress/splay-flash-access.js:1
>> +//@skip if $architecture == "x86"
> 
> I don't think you should drop the old condition.  This should be //@skip if $memoryLimited or $architecture == "x86"

Ditto.

> JSTests/stress/spread-forward-call-varargs-stack-overflow.js:1
> +//@skip if $architecture == "x86"

Ditto.
Comment 5 Mark Lam 2018-06-05 12:33:13 PDT
(In reply to Ryan Haddad from comment #4)
> Comment on attachment 341981 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341981&action=review
> 
> > JSTests/executableAllocationFuzz.yaml:26
> > +      if ($hostOS == "windows" || $architecture == "x86")
> 
> I believe this should be "i386", not "x86".

Not true.  The $architecture value is determined by Tools/Scripts/run-jsc-stress-tests.  According to run-jsc-stress-tests, "x86" is distinct from "x86_64", and is used to represent 32-bit only.  For example, see the line:

$isFTLPlatform = !($architecture == "x86" || $architecture == "arm" || $architecture == "mips" || $hostOS == "windows")

... where the platform is considered to be not an FTL platform if $architecture is "x86" i.e. "x86" is 32-bit only.  There's also no code in run-jsc-stress-tests that sets $architecture to "i386".

"x86" is the right value to test against for 32-bit x86 architecture.
Comment 6 Ryan Haddad 2018-06-05 13:00:27 PDT
(In reply to Mark Lam from comment #5)
> (In reply to Ryan Haddad from comment #4)
> > Comment on attachment 341981 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=341981&action=review
> > 
> > > JSTests/executableAllocationFuzz.yaml:26
> > > +      if ($hostOS == "windows" || $architecture == "x86")
> > 
> > I believe this should be "i386", not "x86".
> 
> Not true.  The $architecture value is determined by
> Tools/Scripts/run-jsc-stress-tests.  According to run-jsc-stress-tests,
> "x86" is distinct from "x86_64", and is used to represent 32-bit only.  For
> example, see the line:
> 
> $isFTLPlatform = !($architecture == "x86" || $architecture == "arm" ||
> $architecture == "mips" || $hostOS == "windows")
> 
> ... where the platform is considered to be not an FTL platform if
> $architecture is "x86" i.e. "x86" is 32-bit only.  There's also no code in
> run-jsc-stress-tests that sets $architecture to "i386".
> 
> "x86" is the right value to test against for 32-bit x86 architecture.
I stand corrected! Thanks, Mark.
Comment 7 Dawei Fenton (:realdawei) 2018-06-05 13:06:37 PDT
(In reply to Mark Lam from comment #2)
> Comment on attachment 341981 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341981&action=review
> 
> > JSTests/stress/splay-flash-access.js:1
> > +//@skip if $architecture == "x86"
> 
> I don't think you should drop the old condition.  This should be //@skip if
> $memoryLimited or $architecture == "x86"

ok got it, will make this change to keep both conditions
Comment 8 Dawei Fenton (:realdawei) 2018-06-05 13:32:06 PDT
Created attachment 341989 [details]
Patch
Comment 9 Mark Lam 2018-06-05 13:39:16 PDT
Comment on attachment 341989 [details]
Patch

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

> JSTests/slowMicrobenchmarks/default-derived-constructor.js:1
> +//@skip if $architecture == "x86"

Sorry I didn't catch this earlier.  Stylistically, I think we should have a space between '//@' and 'skip'.  I know there's a few violations of this, but in general, that's how we do it.  Can you please fix?
Comment 10 Dawei Fenton (:realdawei) 2018-06-05 13:41:46 PDT
(In reply to Mark Lam from comment #9)
> Comment on attachment 341989 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341989&action=review
> 
> > JSTests/slowMicrobenchmarks/default-derived-constructor.js:1
> > +//@skip if $architecture == "x86"
> 
> Sorry I didn't catch this earlier.  Stylistically, I think we should have a
> space between '//@' and 'skip'.  I know there's a few violations of this,
> but in general, that's how we do it.  Can you please fix?

yep, you got it.
Comment 11 Dawei Fenton (:realdawei) 2018-06-05 14:02:41 PDT
Created attachment 341993 [details]
Skip the tests.
Comment 12 WebKit Commit Bot 2018-06-05 15:57:12 PDT
Comment on attachment 341993 [details]
Skip the tests.

Clearing flags on attachment: 341993

Committed r232525: <https://trac.webkit.org/changeset/232525>
Comment 13 WebKit Commit Bot 2018-06-05 15:57:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2018-06-05 15:58:20 PDT
<rdar://problem/40827539>
Comment 15 Dawei Fenton (:realdawei) 2018-06-06 17:59:39 PDT
Re-opening this bug to reflect the fact that these tests were skipped.  The cause of the failures needs further investigation.   The regression occurred somewhere in the range of r232114 - 232120