1411170 - asm cache write

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[yuyin]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/604.1.38 (KHTML, like Gecko) Version/11.0 Safari/604.1.38

Steps to reproduce:

run jit-test


Actual results:

some tests failed, for example asmjs/testCaching.js


Expected results:

tests should pass

Comment 1

[yuyin]

 when serialize wasm code, it will call function "StoreAsmJSModuleInCache"( AsmJS.cpp), and it mprotect a write only memory.

 when serialize, it call function "StaticallyUnlink" to unpatch symbolic address, on mips platform, symbolic address is encode in the instructions('lui' 'ori' ...), unlatch symbolic address should first read the instructions to get the symbolic address and check if it is the right address, so the memory need readable.

I have push a patch to let the memory readable, if you have better idea, please let me know, thank you.

Comment 2

[yuyin]

Attachment: 0001-MIPS-Memory-of-asmjs-cache-need-readable.patch

Comment 3

[Luke Wagner [:luke]]

Ah, makes sense.  Instead of adding #ifdefs to only make MIPS R+W, let's do it for all platforms.

Comment 4

[yuyin]

Attachment: 0001-Asmjs-cache-memory-need-readable.patch

Comment 5

[Luke Wagner [:luke]]

Since this changes jsapi.h, you'll also need to fix the uses in the browser:
  http://searchfox.org/mozilla-central/search?q=SetAsmJSCacheOps

Comment 6

[yuyin]

thank you for review， yes I change the function name in jsapi.h.

in fact, 
http://searchfox.org/mozilla-central/source/dom/asmjscache/AsmJSCache.cpp#280
though it use name 'OpenEntryForWrite', but it use 'READWRITE'  flag to map memory. So it does not need change anything and I did not change the function name there.

maybe I just need to do this:

-------------------------

 {
     if (!jsCachingEnabled || !jsCacheAsmJSPath)
         return JS::AsmJSCache_Disabled_ShellFlags;
@@ -7816,7 +7817,7 @@ ShellOpenAsmJSCacheEntryForWrite(HandleObject global, const char16_t* begin,
     if (memory == MAP_FAILED)
         return JS::AsmJSCache_InternalError;
     MOZ_ASSERT(*(uint32_t*)memory == 0);
-    if (mprotect(memory, serializedSize, PROT_WRITE))
+    if (mprotect(memory, serializedSize, PROT_WRITE | PROT_READ))
-----------------------------------------------

and keep the original function name ‘ShellOpenAsmJSCacheEntryForWrite’？



(In reply to Luke Wagner [:luke] from comment #5)
> Since this changes jsapi.h, you'll also need to fix the uses in the browser:
>   http://searchfox.org/mozilla-central/search?q=SetAsmJSCacheOps

Comment 7

[Luke Wagner [:luke]]

(In reply to yuyin from comment #6)
Yes, just making that one tiny change makes sense to me.

Comment 8

[yuyin]

Attachment: 0001-Asmjs-cache-memory-need-readable.patch

Comment 9

[yuyin]

ok, thank you, attached.

(In reply to Luke Wagner [:luke] from comment #7)
> (In reply to yuyin from comment #6)
> Yes, just making that one tiny change makes sense to me.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d99a96c8da92
Asmjs cache memory need readable. r=luke

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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