Refactor - Sanitization of memory accesses in JIT #6
Conversation
| @@ -200,7 +200,7 @@ const ANCHOR_INTERNAL_FUNCTION_CALL_PROLOGUE: usize = 12; | |||
| const ANCHOR_INTERNAL_FUNCTION_CALL_REG: usize = 13; | |||
| const ANCHOR_CALL_REG_UNSUPPORTED_INSTRUCTION: usize = 14; | |||
| const ANCHOR_TRANSLATE_MEMORY_ADDRESS: usize = 21; | |||
| const ANCHOR_COUNT: usize = 30; // Update me when adding or removing anchors | |||
| const ANCHOR_COUNT: usize = 34; // Update me when adding or removing anchors | |||
There was a problem hiding this comment.
You haven't added or removed anchors, have you?
There was a problem hiding this comment.
The store immediate ones have their own ANCHOR_TRANSLATE_MEMORY_ADDRESS now.
src/jit.rs
Outdated
| } else if destination != REGISTER_SCRATCH { | ||
| self.emit_ins(X86Instruction::load_immediate(destination, value.wrapping_sub(self.immediate_value_key))); | ||
| self.emit_ins(X86Instruction::load_immediate(REGISTER_SCRATCH, self.immediate_value_key)); | ||
| self.emit_ins(X86Instruction::alu(OperandSize::S64, 0x01, REGISTER_SCRATCH, destination, 0, None)); |
There was a problem hiding this comment.
I like the comments you add for clarity next to instructions. One could be useful here for signaling an addition.
src/jit.rs
Outdated
| @@ -1190,8 +1184,12 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { | |||
| } | |||
|
|
|||
| if self.config.enable_address_translation { | |||
| let access_type = if value.is_none() { AccessType::Load } else { AccessType::Store }; | |||
| let anchor = ANCHOR_TRANSLATE_MEMORY_ADDRESS + len.trailing_zeros() as usize + 4 * (access_type as usize); | |||
| let access_type = match value { | |||
There was a problem hiding this comment.
Calling this access_type may be confusing with AccessType. What about altering the enum to represent load, store reg and store imm?
There was a problem hiding this comment.
AccessType comes from the memory mapping and that does not know about store immediate.
So I renamed access_type to anchor_base.
| } else { // AccessType::Store | ||
| if *access_type == 8 { | ||
| // Second half of emit_sanitized_load_immediate() | ||
| self.emit_ins(X86Instruction::alu(OperandSize::S64, 0x81, 0, RSP, lower_key, Some(X86IndirectAccess::OffsetIndexShift(-96, RSP, 0)))); |
There was a problem hiding this comment.
How about adding a comment about the -96 offset? Does it come from stack_slot_of_value_to_store?
src/jit.rs
Outdated
| @@ -1650,8 +1650,10 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { | |||
| self.emit_ins(X86Instruction::xchg(OperandSize::S64, REGISTER_SCRATCH, RSP, Some(X86IndirectAccess::OffsetIndexShift(0, RSP, 0)))); // Swap return address and self.pc | |||
| self.emit_ins(X86Instruction::conditional_jump_immediate(0x85, self.relative_to_anchor(ANCHOR_THROW_EXCEPTION, 6))); | |||
|
|
|||
| // unwrap() the result into REGISTER_SCRATCH | |||
| self.emit_ins(X86Instruction::load(OperandSize::S64, REGISTER_PTR_TO_VM, REGISTER_SCRATCH, X86IndirectAccess::Offset(self.slot_in_vm(RuntimeEnvironmentSlot::ProgramResult) + std::mem::size_of::<u64>() as i32))); | |||
| if *access_type == 0 { // AccessType::Load | |||
There was a problem hiding this comment.
Why is this restricted to load?
There was a problem hiding this comment.
Only load instructions do return a value, store instructions don't.
There is no need to generate a new one for each.
…vm_addr) into ANCHOR_TRANSLATE_MEMORY_ADDRESS.
…e_to_store, constant) into ANCHOR_TRANSLATE_MEMORY_ADDRESS.
Lichtso
force-pushed
the
refactor/sanitization_in_memory_access
branch
from
6cc19b5 to
1132ec9
Compare
No description provided.