Closed Bug 460993 Opened 13 years ago Closed 11 years ago

NJ needs to mark code pages as R+X when done

Categories

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P2)

defect

Tracking

(Not tracked)

VERIFIED FIXED
flash10.1

People

(Reporter: stejohns, Assigned: edwsmith)

References

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tamarin, fixed-in-tracemonkey)

Attachments

(4 obsolete files)

Currently NJ always marks code pages as R+W+X. It really needs to set permission to R+W during code generation, then R+X when done.
Flags: flashplayer-triage+
Flags: flashplayer-qrb?
Flags: flashplayer-qrb? → flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → flash10.x
Priority: P3 → P4
Priority: P4 → P2
related:  Bug 506693
Assignee: nobody → edwsmith
Status: NEW → ASSIGNED
First prototype.  We allocate code 64K at a time, each time we have to touch a 64K page, unprotect it, and then at the end of jitting each method, mark each touched page RX.
Depends on: 538393
Attachment #420405 - Attachment is obsolete: true
Removes calls to VMPI_setPageProtection from CodeAlloc, and adds a new protect/unprotect API to CodeAlloc, along with an SPI for the vm to implement actual page protection.

It is up to the VM to call codeAlloc->protect() before executing jit'd code, but CodeAlloc will internally call unprotect() before modifying blocks, as code is generated.

A flag per code chunk is used so that only modified pages are unprotected and reprotected.
Implements CodeAlloc.[un]protectCodeChunk() two ways, controlled by the AVMPLUS_PROTECT_JITMEM feature.  With the feature disabled, code memory is RWX all the time.  With it enabled, pages are RW when generating code and RX when executing.

Initial testing shows aproximatley 10% slower startup time with AVMFEATURE_PROTECT_JITMEM=1 compared to =0, due to the extra page protection calls.
Attachment #420579 - Flags: review?(rreitmai)
Attachment #420580 - Flags: review?(rreitmai)
Comment on attachment 420579 [details] [diff] [review]
Adds protect/unprotect API to CodeAlloc

Does unprotect() cover the case where a page is partially filled with executable code (which with background compiling could even be running at the time)
and we're wish to add instructions to it?
Attachment #420579 - Flags: review?(rreitmai) → review+
Comment on attachment 420580 [details] [diff] [review]
Use the CodeAlloc protection api to remove W on executable pages.

r+, assuming above comment is addressed
Attachment #420580 - Flags: review?(rreitmai) → review+
the whole patch assumes single-threading, i.e. that no page being protected or unprotected has other executing threads on it.
That makes it easier; so in the case of an OOM failure what state will the pages be left in.

I.e. assume that we're compiling code (i.e we've marked all pages involved as 'RW') and the allocator cannot obtain any more memory for the JIT, will the pages remain as 'RW' or are we able to mark pages back to 'RX'.

Or is it assumed that on OOM we'll flush all JIT'd code and if so, do we have code in place to enforce this policy.
Attachment #420579 - Flags: superreview?(nnethercote)
In case of an OOM failure, the state of CodeAlloc is however it was left, so a recovering host could call CodeAlloc::protect() which would re-protect each touched page.
Comment on attachment 420579 [details] [diff] [review]
Adds protect/unprotect API to CodeAlloc

I'm not sure about the performance implications and the single-threaded requirement, whether they are a problem for TM.  If it's off by default that's less of an issue but I'll defer to Gal here.

Nit:  I find the words "protect" and "unprotect" entirely unintuitive here.  "markRX" and "markRW" would be better.
Attachment #420579 - Flags: superreview?(nnethercote) → superreview?(gal)
how about markExec() and markWrite(); the implementation of these methods is up to the host vm which may not use RX and RW, but instead make code RWX all the time and make these methods no-ops.
Comment on attachment 420579 [details] [diff] [review]
Adds protect/unprotect API to CodeAlloc

The patch leaves us a choice whether we want to take the runtime hit or not. We protect/unprotect repeatedly as we run code since we extend code all the time. Our runtime overhead might be thus larger. On the other hand, we do compile less than tamarin on startup. Lets wire this up in TM and see how it goes. I think its a good enough starting point.
Attachment #420579 - Flags: superreview?(gal) → superreview+
"exec" and "write" are not as immediately intuitive to me as the spelled-out forms markExecutable() and markWritable(), mostly because neither "exec" nor "write" is an adjective.
Comment on attachment 420579 [details] [diff] [review]
Adds protect/unprotect API to CodeAlloc

pushed to nanojit-central
http://hg.mozilla.org/projects/nanojit-central/rev/a1002278492b
Attachment #420579 - Attachment is obsolete: true
Whiteboard: fixed-in-nanojit
Comment on attachment 420580 [details] [diff] [review]
Use the CodeAlloc protection api to remove W on executable pages.

pushed to tamarin-redux
http://hg.mozilla.org/tamarin-redux/rev/137e096d78fa
Attachment #420580 - Attachment is obsolete: true
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tamarin
http://hg.mozilla.org/tracemonkey/rev/69d5a3454a6e
Whiteboard: fixed-in-nanojit, fixed-in-tamarin → fixed-in-nanojit, fixed-in-tamarin, fixed-in-tracemonkey
This broke TM on Windows (WinCE?).  *All* of the VMPI_setPageProtection() calls in avmplus.cpp are bogus, the refer to non-existent variables.  So I backed it out:

http://hg.mozilla.org/projects/nanojit-central/rev/f743ffa4673f

Unfortunately backing it out involved a merge and now the update-nanojit script isn't working for TM, so I am unable to back it out of TM.  I don't know if I did something wrong or if this is a shortcoming of update-nanojit.
I think I know how to fix this now, but I need to check with Graydon (via email) because I don't want to break things even more.
Applies to nanojit-central tip
Attachment #422735 - Flags: review?(nnethercote)
Backed out (with Graydon's help) from TM:

http://hg.mozilla.org/tracemonkey/rev/d85d14f9ff3d
Patch looks good, but I'll run it on the TM try server before giving r+ :)
Comment on attachment 422735 [details] [diff] [review]
(do-over) Adds page protection SPI, and fixes bogus VMPI_setPageProtection calls from the first patch

TM try server results look good enough.
Attachment #422735 - Flags: review?(nnethercote) → review+
Comment on attachment 422735 [details] [diff] [review]
(do-over) Adds page protection SPI, and fixes bogus VMPI_setPageProtection calls from the first patch

pushed to n-c
http://hg.mozilla.org/projects/nanojit-central/rev/3c9030f46c15
Attachment #422735 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/69d5a3454a6e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
QA Contact: nanojit → dschaffe
QA Contact: dschaffe → brbaker
Edwin does this need/warrant a testcase? If not I will remove the in-testsuite? and verify/fixed this bug as it has been pushed into all necessary repos
Flags: in-testsuite?
Yes, we need a selftest that ensures all memory owned by CodeAlloc is readonly when we aren't in the middle of jitting.
verifying fixed by patches landing in all repos and acceptance suite passing. 

Leaving in-testsuite? until selftest media is generated
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.