move dynamic interpreter thunk generation OOP #1707
Conversation
|
|
||
| if (!emitBufferManager->ProtectBufferWithExecuteReadWriteForInterpreter(allocation)) | ||
| { | ||
| Js::Throw::JITOperationFailed(GetLastError()); |
There was a problem hiding this comment.
Js::Throw::JITOperationFailed(GetLastError()); [](start = 12, length = 46)
don't need this. VirtualProtectEx call failure has been checked right after the call. if it's not system call failure, there's fail fast, also the last error will be meaningless
There was a problem hiding this comment.
true, then I guess we should be able to assert that this call succeeds?
There was a problem hiding this comment.
I think this call should never never return false, if we reach here and get a false return, I think it means we missed a path that we should fail fast. you can double check with Satheesh
In reply to: 82306816 [](ancestors = 82306816)
can you fix the memory leak here, I missed this in my change Refers to: lib/Common/Memory/MemUtils.cpp:45 in 2863255. [](commit_id = 2863255, deletion_comment = False) |
MikeHolman
force-pushed
the
oopite
branch
4 times, most recently
from
542f0e6
to
9adef24
Compare
| Js::Throw::FatalInternalError(); | ||
| } | ||
| } | ||
| if (byteCount % 2 != 0) |
There was a problem hiding this comment.
if (byteCount % 2 != 0) [](start = 11, length = 24)
you might want to move this before the for loop to avoid out of bounds write.
There was a problem hiding this comment.
i changed this to use different algorithm instead that doesn't need to use heap.
| Js::Throw::JITOperationFailed(GetLastError()); | ||
| } | ||
|
|
||
| success = emitBufferManager->CommitReadWriteBufferForInterpreter(allocation, remoteBuffer, bufferSize); |
There was a problem hiding this comment.
CommitReadWriteBufferForInterpreter [](start = 37, length = 35)
you might want to add JITOperationFailed in the false path for this API. It will return false, when the protection is unsuccessful - which can happen with the CFG around -
|
Looks good, otherwise. |
|
|
MikeHolman
force-pushed
the
oopite
branch
3 times, most recently
from
2387c62
to
5e5099f
Compare
Merge pull request #1707 from MikeHolman:oopite
No description provided.