| Summary: | REGRESSION: (r232114 - 232120): Multiple JSC Stress test failures on 32 bit architecture | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dawei Fenton (:realdawei) <realdawei> | ||||||||
| Component: | Tools / Tests | Assignee: | 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
Dawei Fenton (:realdawei)
2018-06-04 13:50:36 PDT
Created attachment 341981 [details]
Patch
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 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 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. (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. (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. (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 Created attachment 341989 [details]
Patch
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? (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. Created attachment 341993 [details]
Skip the tests.
Comment on attachment 341993 [details] Skip the tests. Clearing flags on attachment: 341993 Committed r232525: <https://trac.webkit.org/changeset/232525> All reviewed patches have been landed. Closing bug. 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 |