Closed Bug 1411170 Opened 2 years ago Closed 2 years ago

asm cache write

Categories

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

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: yuyin-hf, Assigned: yuyin-hf)

Details

Attachments

(1 file, 2 obsolete files)

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
 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.
Attachment #8921356 - Flags: review?(luke)
Component: Untriaged → JavaScript Engine: JIT
Product: Firefox → Core
Version: 55 Branch → 57 Branch
Assignee: nobody → yuyin-hf
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Ah, makes sense.  Instead of adding #ifdefs to only make MIPS R+W, let's do it for all platforms.
Priority: -- → P1
Attachment #8921719 - Flags: review?(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
Attachment #8921356 - Attachment is obsolete: true
Attachment #8921356 - Flags: review?(luke)
Attachment #8921719 - Flags: review?(luke)
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
(In reply to yuyin from comment #6)
Yes, just making that one tiny change makes sense to me.
Attachment #8921719 - Attachment is obsolete: true
Attachment #8923656 - Flags: review?(luke)
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.
Attachment #8923656 - Flags: review?(luke) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d99a96c8da92
Asmjs cache memory need readable. r=luke
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d99a96c8da92
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.