1359051 - js/src/jit/Ion.cpp:3528:30: error: no member named 'cacheFlush' in 'js::jit::ExecutableAllocator'

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan Beich]

In file included from obj-aarch64-unknown-freebsd11.0/js/src/Unified_cpp_js_src12.cpp:29:
js/src/jit/Ion.cpp:3528:30: error: no member named 'cacheFlush' in 'js::jit::ExecutableAllocator'
        ExecutableAllocator::cacheFlush((void*)start_, size_t(stop_ - start_));
        ~~~~~~~~~~~~~~~~~~~~~^

http://thunderx1.nyi.freebsd.org/data/110arm64-quarterly/439184/logs/firefox-53.0_2,1.log

Comment 1

[Sean Stangl [:sstangl]]

Attachment: 0001-Bug-1359051-Define-cacheFlush-for-non-Simulator-ARM6.patch

cacheFlush is incorrectly gated on __linux__.

Comment 2

[Ted Campbell [:tcampbell]]

Comment on attachment 8860972 [details] [diff] [review]
0001-Bug-1359051-Define-cacheFlush-for-non-Simulator-ARM6.patch

I wonder if you want to guard !XP_IOS. It would seem the system sys_icache_invalidate should be preferred on that target (at least avoid confusion when we do support that target).

Comment 3

[Jan Beich]

Comment on attachment 8860972 [details] [diff] [review]
0001-Bug-1359051-Define-cacheFlush-for-non-Simulator-ARM6.patch

Together with bug 1359142 fix this patch unbreaks standalone SpiderMonkey build on FreeBSD 11.0 aarch64. I haven't tried building via Gecko due to qemu-user-static issues.

(In reply to Ted Campbell [:tcampbell] from comment #2)
> sys_icache_invalidate should be preferred on that target

compiler-rt seems to agree https://github.com/llvm-mirror/compiler-rt/blob/775df7943e07/lib/builtins/clear_cache.c#L146

Comment 4

[Ted Campbell [:tcampbell]]

Thanks for the link, Jan.

sstangl: r=me if you guard defined(JS_CODEGEN_ARM64) && defined(!XP_IOS), or extend the XP_IOS case above to include ARM64.

Comment 5

[Ted Campbell [:tcampbell]]

bleh. !defined(XP_IOS).. you know what I mean

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for a fix for 53. We could still take a patch for 54 though.

Comment 7

[Jan Beich]

Comment on attachment 8860972 [details] [diff] [review]
0001-Bug-1359051-Define-cacheFlush-for-non-Simulator-ARM6.patch

After applying downstream (with comment 4 fix) FF53 builds fine:
https://svnweb.freebsd.org/ports/branches/2017Q2/www/firefox/files/patch-bug1359051?view=markup&pathrev=439552
http://thunderx1.nyi.freebsd.org/data/110arm64-quarterly/439718/logs/firefox-53.0_2,1.log

Comment 8

[Jan Beich]

Attachment: Bug 1359051 - Define cacheFlush for non-Simulator ARM64 builds.

Review commit: https://reviewboard.mozilla.org/r/135586/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135586/

Comment 9

[Ted Campbell [:tcampbell]]

Comment on attachment 8863851 [details]
Bug 1359051 - Define cacheFlush for non-Simulator ARM64 builds.

https://reviewboard.mozilla.org/r/135586/#review138596

Thanks for following this up =)

I can land it if you need once the try finishes.

Comment 10

[Jan Beich]

Comment on attachment 8863851 [details]
Bug 1359051 - Define cacheFlush for non-Simulator ARM64 builds.

https://reviewboard.mozilla.org/r/135586/#review138598

::: js/src/jit/ExecutableAllocator.h:262
(Diff revision 1)
>  #elif defined(JS_CODEGEN_ARM) && (defined(__FreeBSD__) || defined(__NetBSD__))
>      static void cacheFlush(void* code, size_t size)
>      {
>          __clear_cache(code, reinterpret_cast<char*>(code) + size);
>      }
> -#elif defined(JS_CODEGEN_ARM) && defined(XP_IOS)
> +#elif (defined(JS_CODEGEN_ARM) || defined(JS_CODEGEN_ARM64)) && defined(XP_IOS)

Try has no iOS builders, so this is only checked for typos.

Comment 11

[Ted Campbell [:tcampbell]]

Ah, good point. Can we go back to the version where with defined(JS_CODEGEN_ARM64) && !defined(XP_IOS). Defer the IOS case until we have someone testing it.

Comment 12

[Jan Beich]

(In reply to Ted Campbell [:tcampbell] from comment #11)
> Ah, good point. Can we go back to the version where with
> defined(JS_CODEGEN_ARM64) && !defined(XP_IOS). Defer the IOS case until we
> have someone testing it.

What's the point going back to the version that doesn't compile? aarch64 on any OS is also Tier3.

Comment 13

[Ted Campbell [:tcampbell]]

Fair point. I've pushed to autoland queue. Sorry for the confusion.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c53e5de60627
Define cacheFlush for non-Simulator ARM64 builds. r=tcampbell

Comment 15

[Jan Beich]

Comment on attachment 8863851 [details]
Bug 1359051 - Define cacheFlush for non-Simulator ARM64 builds.

Approval Request Comment
[ESR consideration]: Better opt-in JIT support on aarch64.
[Feature/Bug causing the regression]: bug 1323115 regression and bug 1271565 feature
[User impact if declined]: Broken build on aarch64 on any OS except Android or Linux unless --disable-ion is added to .mozconfig.
[Is this code covered by automated tests?]: aarch64 is NPOTB
[Has the fix been verified in Nightly?]: Yes, standalone and as part of FF53 downstream.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Can only break build due to typos.
[String changes made/needed]: None

Comment 16

[Jan Beich]

Attachment: rebased for esr52

In case "hg import" is picky about patch context.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c53e5de60627

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8863851 [details]
Bug 1359051 - Define cacheFlush for non-Simulator ARM64 builds.

Fix a broken build on aarch64. Beta54+. Should be in 54 beta 6.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d22924480749

Comment 20

[Andrei Vaida [:avaida]]

(In reply to Jan Beich from comment #15)
> [Is this code covered by automated tests?]: aarch64 is NPOTB
> [Has the fix been verified in Nightly?]: Yes, standalone and as part of FF53
> downstream.
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Jan's assessment on manual testing needs.

Comment 21

[Julien Cristau [:jcristau]]

Comment on attachment 8863941 [details] [diff] [review]
rebased for esr52



build fix for tier-3 arm64 builds, esr52.2+

Comment 22

[Julien Cristau [:jcristau]]

Comment on attachment 8863851 [details]
Bug 1359051 - Define cacheFlush for non-Simulator ARM64 builds.

moved esr52 approval flag to the rebased patch

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/8d7dbe5c6587