Bug 177586 – Enable gigacage on iOS

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 322061 [details]
maybe it will work

This is a super wimpy version of gigacage for iOS.  I think it's a good place to start.

[Filip Pizlo]

Created attachment 322084 [details]
seems to work fine

[Build Bot]

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


ERROR: Source/JavaScriptCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/bmalloc/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 6 files


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

[Michael Saboff]

Comment on attachment 322084 [details]
seems to work fine

r=me

[Filip Pizlo]

Created attachment 322088 [details]
the patch

[Filip Pizlo]

Comment on attachment 322088 [details]
the patch

And now, with a changelog!

[Filip Pizlo]

Comment on attachment 322088 [details]
the patch

r=michael on the previous version of this patch

[Mark Lam]

Comment on attachment 322088 [details]
the patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        The hardest part of enabling Gigacage on iOS is that it requires loading global variables whil

typo: /whil/while/

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/222625/webkit

[Radar WebKit Bug Importer]

<rdar://problem/34721892>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 177664

[Filip Pizlo]

Created attachment 322223 [details]
a better approach

I'm still testing it.

[Build Bot]

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


ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:57:  g_wasEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:258:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:167:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:168:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/HeapKind.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 5 in 19 files


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

[Filip Pizlo]

Created attachment 322284 [details]
better patch

I think this will actually work.

[Build Bot]

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


ERROR: Source/bmalloc/bmalloc/HeapKind.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:57:  g_wasEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:258:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:167:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:168:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 25 files


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

[Filip Pizlo]

Looks like GTK is red because gcc segfaulted.

[Filip Pizlo]

Created attachment 322985 [details]
the patch

Rebased and tested it more

[Build Bot]

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


ERROR: Source/bmalloc/bmalloc/HeapKind.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:57:  g_wasEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:258:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:167:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:168:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 25 files


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

[Build Bot]

Comment on attachment 322985 [details]
the patch

Attachment 322985 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4779049

New failing tests:
workers/wasm-long-compile.html

[Build Bot]

Created attachment 323002 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[JF Bastien]

Comment on attachment 322985 [details]
the patch

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

r=me with some comments.

> Source/JavaScriptCore/ChangeLog:8
> +        The hardest part of enabling Gigacage on iOS is that it requires loading global variables whil

"while"

> Source/bmalloc/bmalloc/Gigacage.cpp:108
> +} // anonymous namespce

I like that you moved the typo too.

> Source/bmalloc/bmalloc/Gigacage.h:41
> +#define GIGACAGE_ALLOCATION_CAN_FAIL 1

Can this be an option instead? Seems like we want to be able to disable it.

It would also be nice for the changelog to say what happens when it goes from using gigacage to it failing. What happens to allocations then, is it monotonic, etc.

> Source/bmalloc/bmalloc/Gigacage.h:72
> +BINLINE bool wasEnabled() { return g_wasEnabled; }

Can you just export the function, instead of exposing the bool?

> Source/bmalloc/bmalloc/HeapKind.h:97
> +    default:

I'd just make this Primary and have no default. That way the compiler will yell if another kind is added.

> Source/bmalloc/bmalloc/HeapKind.h:113
> +    default:

I'd just make this Primary and have no default. That way the compiler will yell if another kind is added.

[Filip Pizlo]

(In reply to JF Bastien from comment #21)
> Comment on attachment 322985 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322985&action=review
> 
> r=me with some comments.
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        The hardest part of enabling Gigacage on iOS is that it requires loading global variables whil
> 
> "while"

Fixed.

> 
> > Source/bmalloc/bmalloc/Gigacage.cpp:108
> > +} // anonymous namespce
> 
> I like that you moved the typo too.

Fixed.

> 
> > Source/bmalloc/bmalloc/Gigacage.h:41
> > +#define GIGACAGE_ALLOCATION_CAN_FAIL 1
> 
> Can this be an option instead? Seems like we want to be able to disable it.

Can't be an "Options" option because this is bmalloc, so it sits below Options.

> 
> It would also be nice for the changelog to say what happens when it goes
> from using gigacage to it failing. What happens to allocations then, is it
> monotonic, etc.

I'll add blurbs.

> 
> > Source/bmalloc/bmalloc/Gigacage.h:72
> > +BINLINE bool wasEnabled() { return g_wasEnabled; }
> 
> Can you just export the function, instead of exposing the bool?

I need things to inline it and read the bool directly for speed.

> 
> > Source/bmalloc/bmalloc/HeapKind.h:97
> > +    default:
> 
> I'd just make this Primary and have no default. That way the compiler will
> yell if another kind is added.

This was deliberate!  Your approach would cause the compiler to emit more branches.  It would have to emit a separate branch for Primary.  This could work, but I think it's sufficiently ugly that I don't want it:

switch (things) {
case SomeCase:
case SomeOtherCase:
    return 1;
case YetAnotherCase:
}
return 0; // The implicit default will fall through to here also, so there aren't any extra branches.

> 
> > Source/bmalloc/bmalloc/HeapKind.h:113
> > +    default:
> 
> I'd just make this Primary and have no default. That way the compiler will
> yell if another kind is added.

Ditto.

[Filip Pizlo]

(In reply to Build Bot from comment #20)
> Created attachment 323002 [details]
> Archive of layout-test-results from ews114 for mac-elcapitan
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-debug-ews.
> Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

This crash looks totally unrelated.

[Filip Pizlo]

Created attachment 323022 [details]
patch for landing

[Build Bot]

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


ERROR: Source/bmalloc/bmalloc/HeapKind.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:57:  g_wasEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.cpp:258:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:167:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:168:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 25 files


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

[Filip Pizlo]

GTK/WPE build failures look to be in trunk.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/223015/webkit

[Csaba Osztrogonác]

FYI, it broke the AArch64 build on Linux, see https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/1878 for details. Note: I'm not interested in fixing, investigate, help fixing, etc myself. I just reported it, maybe anyone else is interested in fixing it.

[Filip Pizlo]

(In reply to Csaba Osztrogonác from comment #28)
> FYI, it broke the AArch64 build on Linux, see
> https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/
> 1878 for details. Note: I'm not interested in fixing, investigate, help
> fixing, etc myself. I just reported it, maybe anyone else is interested in
> fixing it.

That’s unfortunate. I think we should not claim to run on arm64/Linux if nobody is around to fix things. I don’t even have such a device to test on.

[Yusuke Suzuki]

(In reply to Filip Pizlo from comment #29)
> (In reply to Csaba Osztrogonác from comment #28)
> > FYI, it broke the AArch64 build on Linux, see
> > https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/
> > 1878 for details. Note: I'm not interested in fixing, investigate, help
> > fixing, etc myself. I just reported it, maybe anyone else is interested in
> > fixing it.
> 
> That’s unfortunate. I think we should not claim to run on arm64/Linux if
> nobody is around to fix things. I don’t even have such a device to test on.

Yeah, that's sad thing. I don't have any ARM64 machines too, (except for iPhone :P).
Maybe, disabling it right now for ARM64 Linux would be the easiest option.

[Yusuke Suzuki]

Committed r223025: <http://trac.webkit.org/changeset/223025>

[Filip Pizlo]

(In reply to Yusuke Suzuki from comment #30)
> (In reply to Filip Pizlo from comment #29)
> > (In reply to Csaba Osztrogonác from comment #28)
> > > FYI, it broke the AArch64 build on Linux, see
> > > https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/
> > > 1878 for details. Note: I'm not interested in fixing, investigate, help
> > > fixing, etc myself. I just reported it, maybe anyone else is interested in
> > > fixing it.
> > 
> > That’s unfortunate. I think we should not claim to run on arm64/Linux if
> > nobody is around to fix things. I don’t even have such a device to test on.
> 
> Yeah, that's sad thing. I don't have any ARM64 machines too, (except for
> iPhone :P).
> Maybe, disabling it right now for ARM64 Linux would be the easiest option.

I think we should disable the JIT on that port. 

It would be a lot better if partly-working, partly-maintained, and party-tested ports were maintained out of tree.

[Yusuke Suzuki]

(In reply to Filip Pizlo from comment #32)
> (In reply to Yusuke Suzuki from comment #30)
> > (In reply to Filip Pizlo from comment #29)
> > > (In reply to Csaba Osztrogonác from comment #28)
> > > > FYI, it broke the AArch64 build on Linux, see
> > > > https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/
> > > > 1878 for details. Note: I'm not interested in fixing, investigate, help
> > > > fixing, etc myself. I just reported it, maybe anyone else is interested in
> > > > fixing it.
> > > 
> > > That’s unfortunate. I think we should not claim to run on arm64/Linux if
> > > nobody is around to fix things. I don’t even have such a device to test on.
> > 
> > Yeah, that's sad thing. I don't have any ARM64 machines too, (except for
> > iPhone :P).
> > Maybe, disabling it right now for ARM64 Linux would be the easiest option.
> 
> I think we should disable the JIT on that port. 
> 
> It would be a lot better if partly-working, partly-maintained, and
> party-tested ports were maintained out of tree.

I think ARM64 Linux JIT itself is not so bad status right now (it is tested in the bot, and the count of failures are fairly small). I don't think we do not need to disable it immediately.
But if the failure count becomes super large and nobody maintains it, we should disable it.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #33)
> 
> I think ARM64 Linux JIT itself is not so bad status right now (it is tested
> in the bot, and the count of failures are fairly small). I don't think we do
> not need to disable it immediately.
> But if the failure count becomes super large and nobody maintains it, we
> should disable it.

I think this good status is largely due to,

1. Darwin ARM64 is basically usable for Linux ARM64.
2. We have much stable x64 Linux JSC support, which maintains C++ runtime part of JSC on Linux well.

[Filip Pizlo]

(In reply to Yusuke Suzuki from comment #33)
> (In reply to Filip Pizlo from comment #32)
> > (In reply to Yusuke Suzuki from comment #30)
> > > (In reply to Filip Pizlo from comment #29)
> > > > (In reply to Csaba Osztrogonác from comment #28)
> > > > > FYI, it broke the AArch64 build on Linux, see
> > > > > https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/
> > > > > 1878 for details. Note: I'm not interested in fixing, investigate, help
> > > > > fixing, etc myself. I just reported it, maybe anyone else is interested in
> > > > > fixing it.
> > > > 
> > > > That’s unfortunate. I think we should not claim to run on arm64/Linux if
> > > > nobody is around to fix things. I don’t even have such a device to test on.
> > > 
> > > Yeah, that's sad thing. I don't have any ARM64 machines too, (except for
> > > iPhone :P).
> > > Maybe, disabling it right now for ARM64 Linux would be the easiest option.
> > 
> > I think we should disable the JIT on that port. 
> > 
> > It would be a lot better if partly-working, partly-maintained, and
> > party-tested ports were maintained out of tree.
> 
> I think ARM64 Linux JIT itself is not so bad status right now (it is tested
> in the bot, and the count of failures are fairly small). I don't think we do
> not need to disable it immediately.
> But if the failure count becomes super large and nobody maintains it, we
> should disable it.

Imagine if someone needed to ship trunk WebKit on arm64/Linux. Would we want them to use cloop or the JIT?  I would argue that it would be better if they shipped cloop, if they were using the current trunk. I think this would be better because anyone shipping a product would rather ship something that actually works than something that performs well on benchmarks. 

Based on this, I think we should disable the JIT on arm64/Linux. It’s not enough to just have some bots that build it or even periodically runs out tests. For a JIT port to be shippable it needs some active attention.

[Filip Pizlo]

(In reply to Yusuke Suzuki from comment #34)
> (In reply to Yusuke Suzuki from comment #33)
> > 
> > I think ARM64 Linux JIT itself is not so bad status right now (it is tested
> > in the bot, and the count of failures are fairly small). I don't think we do
> > not need to disable it immediately.
> > But if the failure count becomes super large and nobody maintains it, we
> > should disable it.
> 
> I think this good status is largely due to,
> 
> 1. Darwin ARM64 is basically usable for Linux ARM64.
> 2. We have much stable x64 Linux JSC support, which maintains C++ runtime
> part of JSC on Linux well.

Right, but there are differences that have to be maintained, like errata and ABI subtleties.

[Yusuke Suzuki]

(In reply to Filip Pizlo from comment #36)
> (In reply to Yusuke Suzuki from comment #34)
> > (In reply to Yusuke Suzuki from comment #33)
> > > 
> > > I think ARM64 Linux JIT itself is not so bad status right now (it is tested
> > > in the bot, and the count of failures are fairly small). I don't think we do
> > > not need to disable it immediately.
> > > But if the failure count becomes super large and nobody maintains it, we
> > > should disable it.
> > 
> > I think this good status is largely due to,
> > 
> > 1. Darwin ARM64 is basically usable for Linux ARM64.
> > 2. We have much stable x64 Linux JSC support, which maintains C++ runtime
> > part of JSC on Linux well.
> 
> Right, but there are differences that have to be maintained, like errata and
> ABI subtleties.

Right. I think we need some discussions on mailing list before disabling ARM64 Linux JIT.
But if nobody is interested in maintaining this, I support disabling it.

I don't think removing code would be required because I don't think any ARM64/Linux JIT specific code exists in JSC right now.
And I do not think we should not have a ARM64/Linux bot with JIT.
But the default option should be disabling JIT if there is no maintainer.

[Filip Pizlo]

(In reply to Yusuke Suzuki from comment #37)
> (In reply to Filip Pizlo from comment #36)
> > (In reply to Yusuke Suzuki from comment #34)
> > > (In reply to Yusuke Suzuki from comment #33)
> > > > 
> > > > I think ARM64 Linux JIT itself is not so bad status right now (it is tested
> > > > in the bot, and the count of failures are fairly small). I don't think we do
> > > > not need to disable it immediately.
> > > > But if the failure count becomes super large and nobody maintains it, we
> > > > should disable it.
> > > 
> > > I think this good status is largely due to,
> > > 
> > > 1. Darwin ARM64 is basically usable for Linux ARM64.
> > > 2. We have much stable x64 Linux JSC support, which maintains C++ runtime
> > > part of JSC on Linux well.
> > 
> > Right, but there are differences that have to be maintained, like errata and
> > ABI subtleties.
> 
> Right. I think we need some discussions on mailing list before disabling
> ARM64 Linux JIT.
> But if nobody is interested in maintaining this, I support disabling it.
> 
> I don't think removing code would be required because I don't think any
> ARM64/Linux JIT specific code exists in JSC right now.

There totally is some. :-)

It’s not a lot, but I propose to remove it if it is dead to testing. We have no way of knowing if it has survived refactorings. 

> And I do not think we should not have a ARM64/Linux bot with JIT.
> But the default option should be disabling JIT if there is no maintainer.

Yeah.

[Zan Dobersek]

(In reply to Filip Pizlo from comment #29)
> (In reply to Csaba Osztrogonác from comment #28)
> > FYI, it broke the AArch64 build on Linux, see
> > https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/
> > 1878 for details. Note: I'm not interested in fixing, investigate, help
> > fixing, etc myself. I just reported it, maybe anyone else is interested in
> > fixing it.
> 
> That’s unfortunate. I think we should not claim to run on arm64/Linux if
> nobody is around to fix things. I don’t even have such a device to test on.

Linux port maintainers are committed to maintaining JIT levels on ARM64.  We've actually (while enabling FTL) in the past committed fixes that might as well have positively affected the iOS platform, barring some peculiarities in the Apple ARM64 CPU implementations.

Ossy doesn't represent us in that regard, even if he's actually in control of the tester builders and cares enough to report any failures as they occur, and especially when he immediately relieves himself from any work to fix things.

[Filip Pizlo]

(In reply to Zan Dobersek from comment #39)
> (In reply to Filip Pizlo from comment #29)
> > (In reply to Csaba Osztrogonác from comment #28)
> > > FYI, it broke the AArch64 build on Linux, see
> > > https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/
> > > 1878 for details. Note: I'm not interested in fixing, investigate, help
> > > fixing, etc myself. I just reported it, maybe anyone else is interested in
> > > fixing it.
> > 
> > That’s unfortunate. I think we should not claim to run on arm64/Linux if
> > nobody is around to fix things. I don’t even have such a device to test on.
> 
> Linux port maintainers are committed to maintaining JIT levels on ARM64. 
> We've actually (while enabling FTL) in the past committed fixes that might
> as well have positively affected the iOS platform, barring some
> peculiarities in the Apple ARM64 CPU implementations.
> 
> Ossy doesn't represent us in that regard, even if he's actually in control
> of the tester builders and cares enough to report any failures as they
> occur, and especially when he immediately relieves himself from any work to
> fix things.

Is there a bot running Linux/arm64 tests and benchmarks?  Without those things, you might as well use the cloop.

The fixes I remember were specific to non-Apple CPUs.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 178093

[Keith Miller]

We rolled this out because it most likely regressed Kraken on iOS by 20%. It may have also regressed ARES-6, JetStream and JSBench.

[Keith Miller]

Reopening because bugzilla conflict resolution sucks.

[Filip Pizlo]

Relanded in https://trac.webkit.org/changeset/223113/webkit

[Carlos Alberto Lopez Perez]

(In reply to Filip Pizlo from comment #40)
> Is there a bot running Linux/arm64 tests and benchmarks?  Without those
> things, you might as well use the cloop.
> 

Yes, there are Linux JSC ARMv7 and ARM64 bots here: https://build.webkit.org/waterfall?category=misc (currently maintained by Ossy).

I will look into adding more Linux JSC bots for ARM64 and ARMv7-Thumb2 (as well as EWS bots) maintained by Igalia ASAP.

[Zan Dobersek]

(In reply to Filip Pizlo from comment #40)
> (In reply to Zan Dobersek from comment #39)
> > (In reply to Filip Pizlo from comment #29)
> > > (In reply to Csaba Osztrogonác from comment #28)
> > > > FYI, it broke the AArch64 build on Linux, see
> > > > https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/
> > > > 1878 for details. Note: I'm not interested in fixing, investigate, help
> > > > fixing, etc myself. I just reported it, maybe anyone else is interested in
> > > > fixing it.
> > > 
> > > That’s unfortunate. I think we should not claim to run on arm64/Linux if
> > > nobody is around to fix things. I don’t even have such a device to test on.
> > 
> > Linux port maintainers are committed to maintaining JIT levels on ARM64. 
> > We've actually (while enabling FTL) in the past committed fixes that might
> > as well have positively affected the iOS platform, barring some
> > peculiarities in the Apple ARM64 CPU implementations.
> > 
> > Ossy doesn't represent us in that regard, even if he's actually in control
> > of the tester builders and cares enough to report any failures as they
> > occur, and especially when he immediately relieves himself from any work to
> > fix things.
> 
> Is there a bot running Linux/arm64 tests and benchmarks?  Without those
> things, you might as well use the cloop.
> 

There's existing ones, but we'll be adding our own.

> 
> The fixes I remember were specific to non-Apple CPUs.

r215731 isn't. It fixed test failures on Linux. I wouldn't know about iOS because there's no public testers.
https://trac.webkit.org/changeset/215731/webkit

[Filip Pizlo]

(In reply to Zan Dobersek from comment #46)
> (In reply to Filip Pizlo from comment #40)
> > (In reply to Zan Dobersek from comment #39)
> > > (In reply to Filip Pizlo from comment #29)
> > > > (In reply to Csaba Osztrogonác from comment #28)
> > > > > FYI, it broke the AArch64 build on Linux, see
> > > > > https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/
> > > > > 1878 for details. Note: I'm not interested in fixing, investigate, help
> > > > > fixing, etc myself. I just reported it, maybe anyone else is interested in
> > > > > fixing it.
> > > > 
> > > > That’s unfortunate. I think we should not claim to run on arm64/Linux if
> > > > nobody is around to fix things. I don’t even have such a device to test on.
> > > 
> > > Linux port maintainers are committed to maintaining JIT levels on ARM64. 
> > > We've actually (while enabling FTL) in the past committed fixes that might
> > > as well have positively affected the iOS platform, barring some
> > > peculiarities in the Apple ARM64 CPU implementations.
> > > 
> > > Ossy doesn't represent us in that regard, even if he's actually in control
> > > of the tester builders and cares enough to report any failures as they
> > > occur, and especially when he immediately relieves himself from any work to
> > > fix things.
> > 
> > Is there a bot running Linux/arm64 tests and benchmarks?  Without those
> > things, you might as well use the cloop.
> > 
> 
> There's existing ones, but we'll be adding our own.
> 
> > 
> > The fixes I remember were specific to non-Apple CPUs.
> 
> r215731 isn't. It fixed test failures on Linux. I wouldn't know about iOS
> because there's no public testers.
> https://trac.webkit.org/changeset/215731/webkit

That looks like it might be due to cpu differences.

[Filip Pizlo]

(In reply to Carlos Alberto Lopez Perez from comment #45)
> (In reply to Filip Pizlo from comment #40)
> > Is there a bot running Linux/arm64 tests and benchmarks?  Without those
> > things, you might as well use the cloop.
> > 
> 
> Yes, there are Linux JSC ARMv7 and ARM64 bots here:
> https://build.webkit.org/waterfall?category=misc (currently maintained by
> Ossy).
> 
> I will look into adding more Linux JSC bots for ARM64 and ARMv7-Thumb2 (as
> well as EWS bots) maintained by Igalia ASAP.

The 32-bit JITs are not maintained anymore. We are probably going to remove that code soon.

[Carlos Alberto Lopez Perez]

(In reply to Filip Pizlo from comment #48)
> (In reply to Carlos Alberto Lopez Perez from comment #45)
> > (In reply to Filip Pizlo from comment #40)
> > > Is there a bot running Linux/arm64 tests and benchmarks?  Without those
> > > things, you might as well use the cloop.
> > > 
> > 
> > Yes, there are Linux JSC ARMv7 and ARM64 bots here:
> > https://build.webkit.org/waterfall?category=misc (currently maintained by
> > Ossy).
> > 
> > I will look into adding more Linux JSC bots for ARM64 and ARMv7-Thumb2 (as
> > well as EWS bots) maintained by Igalia ASAP.
> 
> The 32-bit JITs are not maintained anymore. We are probably going to remove
> that code soon.

The JIT on ARMv7 Thumb2 is currently extensively used in production by a lot of people (including us). It receives a fair amount of testing.

[Filip Pizlo]

(In reply to Carlos Alberto Lopez Perez from comment #49)
> (In reply to Filip Pizlo from comment #48)
> > (In reply to Carlos Alberto Lopez Perez from comment #45)
> > > (In reply to Filip Pizlo from comment #40)
> > > > Is there a bot running Linux/arm64 tests and benchmarks?  Without those
> > > > things, you might as well use the cloop.
> > > > 
> > > 
> > > Yes, there are Linux JSC ARMv7 and ARM64 bots here:
> > > https://build.webkit.org/waterfall?category=misc (currently maintained by
> > > Ossy).
> > > 
> > > I will look into adding more Linux JSC bots for ARM64 and ARMv7-Thumb2 (as
> > > well as EWS bots) maintained by Igalia ASAP.
> > 
> > The 32-bit JITs are not maintained anymore. We are probably going to remove
> > that code soon.
> 
> The JIT on ARMv7 Thumb2 is currently extensively used in production by a lot
> of people (including us). It receives a fair amount of testing.

When we add new features to JSC, we don’t want to also add them to 32-bit. There are now many features that have never been ported to 32-bit. This is because there is basically zero manpower going into 32-but maintenance.

You can continue to use that code from a branch or fork if it’s removed from trunk.

[Zan Dobersek]

(In reply to Filip Pizlo from comment #50)
> (In reply to Carlos Alberto Lopez Perez from comment #49)
> > (In reply to Filip Pizlo from comment #48)
> > > (In reply to Carlos Alberto Lopez Perez from comment #45)
> > > > (In reply to Filip Pizlo from comment #40)
> > > > > Is there a bot running Linux/arm64 tests and benchmarks?  Without those
> > > > > things, you might as well use the cloop.
> > > > > 
> > > > 
> > > > Yes, there are Linux JSC ARMv7 and ARM64 bots here:
> > > > https://build.webkit.org/waterfall?category=misc (currently maintained by
> > > > Ossy).
> > > > 
> > > > I will look into adding more Linux JSC bots for ARM64 and ARMv7-Thumb2 (as
> > > > well as EWS bots) maintained by Igalia ASAP.
> > > 
> > > The 32-bit JITs are not maintained anymore. We are probably going to remove
> > > that code soon.
> > 
> > The JIT on ARMv7 Thumb2 is currently extensively used in production by a lot
> > of people (including us). It receives a fair amount of testing.
> 
> When we add new features to JSC, we don’t want to also add them to 32-bit.
> There are now many features that have never been ported to 32-bit. This is
> because there is basically zero manpower going into 32-but maintenance.
> 
> You can continue to use that code from a branch or fork if it’s removed from
> trunk.

We can discuss this on Friday at the contributors meeting.  It's well off the topic of this bug.

[Zan Dobersek]

Comment on attachment 323022 [details]
patch for landing

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

> Source/JavaScriptCore/offlineasm/arm64.rb:941
> +        when "globaladdr"
> +            uid = $asm.newUID
> +            $asm.puts "L_offlineasm_loh_adrp_#{uid}:"
> +            $asm.puts "adrp #{operands[1].arm64Operand(:ptr)}, #{operands[0].asmLabel}@GOTPAGE"
> +            $asm.puts "L_offlineasm_loh_ldr_#{uid}:"
> +            $asm.puts "ldr #{operands[1].arm64Operand(:ptr)}, [#{operands[1].arm64Operand(:ptr)}, #{operands[0].asmLabel}@GOTPAGEOFF]"
> +            $asm.deferAction {
> +                $asm.puts ".loh AdrpLdrGot L_offlineasm_loh_adrp_#{uid}, L_offlineasm_loh_ldr_#{uid}"
> +            }

Two issues here that broke the ARM64 build on Linux:
- GOTPAGE and GOTPAGEOFF relocations don't work with ELF,
- GCC doesn't support the link-time optimization hints that are triggered through the .loh directive.

Opened bug #178130 to address that.

[Michael Catanzaro]

(In reply to Zan Dobersek from comment #51)
> We can discuss this on Friday at the contributors meeting.  It's well off
> the topic of this bug.

Yes, I'm looking forward to talking about this at the Contributors Meeting. Igalia really wants to keep this code in upstream WebKit. We also understand that won't happen unless we significantly ramp up our upstream JSC maintenance efforts.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 178182

[Filip Pizlo]

*** Bug 178050 has been marked as a duplicate of this bug. ***

[Filip Pizlo]

Relanded in https://trac.webkit.org/changeset/223239/webkit