Modify JIT to support double mapped memory #52600

janvorli · 2021-05-11T15:04:10Z

This change modifies JIT so that it can generate code into
double mapped memory. The code is written into RW mapped memory,
but the relative offsets are computed using the related RX locations.

The change doesn't modify the runtime to provide double mapped memory
yet, the JIT2EE interface allocMem returns the same addresses as RW and
RX. The runtime changes will be part of follow-up PRs. However, it was
already tested with the double mapping enabled locally.

This change modifies JIT so that it can generate code into double mapped memory. The code is written into RW mapped memory, but the relative offsets are computed using the related RX locations. The change doesn't modify the runtime to provide double mapped memory yet, the JIT2EE interface allocMem returns the same addresses as RW and RX. The runtime changes will be part of follow-up PRs. However, it was already tested with the double mapping enabled locally.

jkotas · 2021-05-11T15:13:23Z

src/coreclr/inc/corjit.h

-            void **             roDataBlock     /* OUT */
+            void **             coldCodeBlockRW,/* OUT */
+            void **             roDataBlock,     /* OUT */
+            void **             roDataBlockRW   /* OUT */


This method takes 11 arguments now and it is unlikely we are done adding more arguments to it. Would it be better to refactor it to take struct with everything instead?

AndyAyersMS · 2021-05-11T18:39:44Z

cc @dotnet/jit-contrib

src/coreclr/jit/emit.cpp

* Change signature of allocMem to pass everything in a single structure * Extract code writing part of the emitEndCodeGen to a separate method and wrap the call to it in eeRunWithErrorTrap

sandreenko

the jit/spmi changes look good to me,
I am concerned about future changes of this code, what would happen if a jit developer uses a wrong address for a write (dstRW instead of dstRE, or dstRE instead of dstRW), will we see the issue at runtime only if we execute the code? will we see the issue on the platforms without W^X protection? Can we catch them at compilation time?

sandreenko · 2021-05-12T09:33:21Z

src/coreclr/jit/emitarm64.cpp

@@ -10253,7 +10253,7 @@ unsigned emitter::emitOutputCall(insGroup* ig, BYTE* dst, instrDesc* id, code_t
 /*static*/ unsigned emitter::emitOutput_Instr(BYTE* dst, code_t code)
 {
    assert(sizeof(code_t) == 4);
-    *((code_t*)dst) = code;
+    *((code_t*)(dst + writeableOffset)) = code;


If I understand correctly we keeep dst name for dstRE and name all RW accesses as dstRW?

I would prefer to see a named variable here, if I understand correctly it is

unsigned emitter::emitOutput_Instr(BYTE* dstRE, code_t code) BYTE* dstRW = dst + writeableOffsetl

That would require quite a lot of changes over the JIT code base as all code addresses in JIT are read-execute and the RW are used only at places where it is needed. So everywhere that dst is used now, it would become dstRE. That seemed like a noise change to me that would obscure the real changes.

what would happen if a jit developer uses a wrong address for a write (dstRW instead of dstRE, or dstRE instead of dstRW)

If you attempt to write using dstRE address when jitting, the JIT would crash as it would not be able to write there. If you use the dstRW address instead of dstRE, it would be fine unless you use it for code relative address computation.
I've tried to use the dstRW only locally so that it would be hard to make such a mistake. The only exception is the NOP generating code that calls itself recursively and it would have to update the RE pointers to match the RW all the time.
In fact, I'd rather remove the dstRW usage at other places and change it to (dst + writeableOffset).

will we see the issue on the platforms without W^X protection?

There will be no such platforms (except of crossgen usage of the JIT)

Can we catch them at compilation time?

We would have to change all the BYTE* dst to const BYTE* dst, but it would still require the JIT developer to be disciplined enough to not to cast away the constness anywhere.

Frankly, my view on further updates to JIT were that everything would be done as before with the only exception being places where writes to the executable memory are added. At those places, the writeableOffset would need to be applied. But a bug in such a place would be easy and quick to discover - the JIT would just crash with access violation.

I've actually decided to create and use dstRW at all places. The one above was actually the only one that was not done that way.

src/coreclr/jit/emitarm64.cpp

src/coreclr/jit/emitxarch.cpp

jkotas · 2021-05-12T13:29:58Z

src/coreclr/jit/emit.cpp

+
+    // Ensure that any attempt to write code after this point will fail
+    writeableOffset = 0;
+    // Notify the EE that all code writing is done. It can switch off writeable code memory.


Does this meaninfully shorten the windows of where the writeable mapping is open vs. just closing the writeable mapping once we are done JITing and return to VM?

I am wondering whether it would make sense to emit the code into a scratch buffer, and then open the writeable mapping just around memcpy of the scratch buffer to the final location.

I have originally had the unmapping after the method jitting was completed, but I have thought it may be better to put it right after the code is written, so I've modified it this way.
Using a scratch buffer sounds like an interesting idea that I'd like to try. So l think I'll update this PR by removing the extraction of the writeCode method and the doneWritingCode method. It is a no-op in the current state anyways, so I can possibly put it back in a later stage.

I did that update. @AndyAyersMS I have removed the runWithErrorTrap and doneWritingCode again based on Jan's idea.

Fine by me, having the runtime own the cleanup makes sense.

This change reverts the addition of the doneWritingCode as it seems using a scrach buffer to write the code and then creating the target writeable mapping only for memcpy of the code to that would be beneficial. I want to explore that and this JIT change doesn't do anything in the doneWritingCode yet. The change to extract part of the emitEndCodeGen into a separate writeCode method was reverted too.

janvorli added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 11, 2021

janvorli added this to the 6.0.0 milestone May 11, 2021

janvorli requested review from AndyAyersMS and sandreenko May 11, 2021 15:04

janvorli self-assigned this May 11, 2021

jkotas reviewed May 11, 2021

View reviewed changes

AndyAyersMS reviewed May 11, 2021

View reviewed changes

src/coreclr/jit/emit.cpp Outdated Show resolved Hide resolved

Apply JIT formatting patch

d325732

runfoapp bot mentioned this pull request May 11, 2021

CI is currently broken "You cannot extract a file outside of the target path" #52596

Closed

Reflect PR feedback

26bdeef

* Change signature of allocMem to pass everything in a single structure * Extract code writing part of the emitEndCodeGen to a separate method and wrap the call to it in eeRunWithErrorTrap

sandreenko approved these changes May 12, 2021

View reviewed changes

janvorli added 2 commits May 12, 2021 12:51

Reflect PR feedback and apply formatting patch

2fb2933

Yet another formatting patch

f11ea73

jkotas reviewed May 12, 2021

View reviewed changes

AndyAyersMS approved these changes May 12, 2021

View reviewed changes

janvorli merged commit 4ca5d81 into dotnet:main May 12, 2021

janvorli deleted the double-mapped-jit branch May 12, 2021 21:04

ghost locked as resolved and limited conversation to collaborators Jun 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify JIT to support double mapped memory #52600

Modify JIT to support double mapped memory #52600

janvorli commented May 11, 2021

jkotas May 11, 2021

janvorli May 12, 2021

AndyAyersMS commented May 11, 2021

sandreenko left a comment

sandreenko May 12, 2021

janvorli May 12, 2021

janvorli May 12, 2021

jkotas May 12, 2021

janvorli May 12, 2021

janvorli May 12, 2021

AndyAyersMS May 12, 2021

Modify JIT to support double mapped memory #52600

Modify JIT to support double mapped memory #52600

Conversation

janvorli commented May 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyAyersMS commented May 11, 2021

sandreenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment