Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracing function runtime patch #1060

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
8d706a5
Add runtime patch and restore calls on trace transition.
Pavel-Durov Mar 27, 2024
e1fd774
Add docstring in patch module
Pavel-Durov Mar 27, 2024
ee56310
Add message to panic on empty trace in swt.
Pavel-Durov Mar 27, 2024
461448f
Add patch calls to side-traces.
Pavel-Durov Mar 27, 2024
e237278
Add panic if runtime patch is called on non-x86 platform.
Pavel-Durov Mar 27, 2024
0699dbf
Add test to swt patch.
Pavel-Durov Mar 27, 2024
0db3e4f
Add restore assertion to patch test.
Pavel-Durov Mar 27, 2024
6994b59
Add assert to function restore.
Pavel-Durov Mar 27, 2024
0e88a29
Add x86 compile-time guard
Pavel-Durov Mar 29, 2024
86e404b
Add compile-time guard.
Pavel-Durov Mar 29, 2024
3725c71
Remove PROT_EXEC on mprotect.
Pavel-Durov Mar 29, 2024
799644e
Revert "Remove PROT_EXEC on mprotect."
Pavel-Durov Mar 29, 2024
d572cab
Add documentation.
Pavel-Durov Mar 29, 2024
0bd8f36
Add module docstring.
Pavel-Durov Mar 29, 2024
08029de
Merge branch 'master' into tracing-function-runtime-patch
Pavel-Durov Mar 29, 2024
b2aee3d
Encapsulate x86 instructions.
Pavel-Durov Mar 29, 2024
f1794a2
Merge branch 'master' into tracing-function-runtime-patch
Pavel-Durov Apr 7, 2024
c46bb5f
Merge branch 'master' into tracing-function-runtime-patch
Pavel-Durov Apr 9, 2024
40c0696
Remove PROT_EXEC from mprotect.
Pavel-Durov Apr 9, 2024
4401153
Format.
Pavel-Durov Apr 9, 2024
5ead893
Remove not needed cfgs.
Pavel-Durov Apr 11, 2024
b6fcbd4
Add x86_64 guard.
Pavel-Durov Apr 11, 2024
b334bfa
Merge branch 'master' into tracing-function-runtime-patch
Pavel-Durov Apr 11, 2024
6f4bdc3
Set mprotect as read+write on patch.
Pavel-Durov Apr 13, 2024
8e00241
Use Layout::from_size_align for aligned page calculation.
Pavel-Durov Apr 14, 2024
ada68bb
Change match to unwrap().
Pavel-Durov Apr 18, 2024
3e7d023
Recover and panic on mprotect.
Pavel-Durov Apr 18, 2024
18b7fee
Remove +size from start_offset calculation.
Pavel-Durov Apr 18, 2024
e1be48c
Add comments.
Pavel-Durov Apr 18, 2024
0428dc1
Use unwrap over expect.
Pavel-Durov Apr 20, 2024
6be7bef
Early return if mprotect failes to set memmory page as writable.
Pavel-Durov Apr 20, 2024
6f0abb3
Add panic on mprotect failure.
Pavel-Durov Apr 23, 2024
94597ef
Merge branch 'master' into tracing-function-runtime-patch
Pavel-Durov May 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions ykcapi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ use std::{
ptr,
sync::Arc,
};
#[cfg(tracer_swt)]
use ykrt::trace_basicblock;
use ykrt::{HotThreshold, Location, MT};

#[no_mangle]
Expand Down Expand Up @@ -97,9 +95,3 @@ pub extern "C" fn yk_location_new() -> Location {
pub extern "C" fn yk_location_drop(loc: Location) {
drop(loc)
}

#[cfg(tracer_swt)]
#[no_mangle]
pub extern "C" fn yk_trace_basicblock(function_index: usize, block_index: usize) {
trace_basicblock(function_index, block_index);
}
3 changes: 2 additions & 1 deletion ykrt/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ pub fn print_jit_state(state: &str) {
}

#[cfg(tracer_swt)]
pub fn trace_basicblock(function_index: usize, block_index: usize) {
#[no_mangle]
pub extern "C" fn yk_trace_basicblock(function_index: usize, block_index: usize) {
if mt::is_tracing() {
trace::trace_basicblock(function_index, block_index)
}
Expand Down
21 changes: 21 additions & 0 deletions ykrt/src/mt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ use std::{
thread,
};

#[cfg(tracer_swt)]
use crate::trace::swt::patch::{patch_trace_function, restore_trace_function};

use parking_lot::{Condvar, Mutex, MutexGuard};
#[cfg(not(all(feature = "yk_testing", not(test))))]
use parking_lot_core::SpinWait;
Expand Down Expand Up @@ -263,6 +266,11 @@ impl MT {
TransitionControlPoint::StartTracing(hl) => {
#[cfg(feature = "yk_jitstate_debug")]
print_jit_state("start-tracing");
#[cfg(tracer_swt)]
unsafe {
restore_trace_function();
}

let tracer = {
let lk = self.tracer.lock();
Arc::clone(&*lk)
Expand All @@ -279,6 +287,11 @@ impl MT {
}
}
TransitionControlPoint::StopTracing => {
#[cfg(tracer_swt)]
unsafe {
patch_trace_function();
}

// Assuming no bugs elsewhere, the `unwrap`s cannot fail, because `StartTracing`
// will have put a `Some` in the `Rc`.
let (hl, thread_tracer, promotions) = THREAD_MTTHREAD.with(|mtt| {
Expand Down Expand Up @@ -325,6 +338,10 @@ impl MT {
self.stats.timing_state(TimingState::None);
#[cfg(feature = "yk_jitstate_debug")]
print_jit_state("stop-side-tracing");
#[cfg(tracer_swt)]
unsafe {
patch_trace_function();
}
self.queue_compile_job(
(utrace, promotions.into_boxed_slice()),
hl,
Expand Down Expand Up @@ -544,6 +561,10 @@ impl MT {
TransitionGuardFailure::StartSideTracing(hl) => {
#[cfg(feature = "yk_jitstate_debug")]
print_jit_state("start-side-tracing");
#[cfg(tracer_swt)]
unsafe {
restore_trace_function();
}
let tracer = {
let lk = self.tracer.lock();
Arc::clone(&*lk)
Expand Down
3 changes: 2 additions & 1 deletion ykrt/src/trace/swt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use std::sync::Once;
use std::{cell::RefCell, collections::HashMap, error::Error, ffi::CString, sync::Arc};

mod iterator;
pub(crate) mod patch;
ltratt marked this conversation as resolved.
Show resolved Hide resolved
use iterator::SWTraceIterator;

static FUNC_NAMES_INIT: Once = Once::new();
Expand Down Expand Up @@ -111,7 +112,7 @@ impl TraceRecorder for SWTTraceRecorder {
Err(TraceRecorderError::TraceTooLong)
} else if aot_blocks.is_empty() {
// FIXME: who should handle an empty trace?
panic!();
panic!("swt encountered an empty trace!");
} else {
Ok(Box::new(SWTraceIterator::new(aot_blocks)))
}
Expand Down
134 changes: 134 additions & 0 deletions ykrt/src/trace/swt/patch.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
use libc::{mprotect, size_t, sysconf, PROT_EXEC, PROT_READ, PROT_WRITE};
ltratt marked this conversation as resolved.
Show resolved Hide resolved
use std::mem;
use std::{ffi::c_void, sync::Once};

#[cfg(tracer_swt)]
ltratt marked this conversation as resolved.
Show resolved Hide resolved
use crate::yk_trace_basicblock;

#[cfg(tracer_swt)]
static ORIGINAL_INSTRUCTIONS_INIT: Once = Once::new();
#[cfg(tracer_swt)]
static mut ORIGINAL_INSTRUCTIONS: [u8; 1] = [0; 1];
#[cfg(tracer_swt)]
static mut PATCH_INSTRUCTIONS: [u8; 1] = [0xC3];
ltratt marked this conversation as resolved.
Show resolved Hide resolved

/// This function is used to save the original instructions of a function to .
///
/// # Arguments
///
/// * `function_ptr` - A usize representing the memory address of the function.
/// * `instructions` - A mutable pointer to a u8 where the original instructions will be saved.
/// * `num_of_instructions` - A usize indicating the number of instructions to save.
///
#[cfg(tracer_swt)]
unsafe fn save_original_instructions(
function_ptr: usize,
instructions: *mut u8,
num_of_instructions: usize,
) {
let func_ptr: *const () = function_ptr as *const ();
std::ptr::copy_nonoverlapping(func_ptr as *const u8, instructions, num_of_instructions);
}

/// This function is used to patch a function instructions at runtime.
///
/// # Arguments
///
/// * `function_ptr` - A usize representing the memory address of the function to be patched.
/// * `code` - A constant pointer to a u8 vector where the new instructions are located.
/// * `size` - A size_t indicating the number of bytes to copy from `code`.
///
#[cfg(tracer_swt)]
unsafe fn patch_function(function_ptr: usize, code: *const u8, size: size_t) {
let page_size = sysconf(libc::_SC_PAGESIZE) as usize;

let func_address = ((function_ptr as usize) & !(page_size - 1)) as *mut c_void;
let page_size_aligned = (((function_ptr as usize) + mem::size_of_val(&function_ptr))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use std::alloc::Layout::align and friends to do this calculation for us.

I'm also not sure what the difference between function_ptr and func_address is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not sure what the difference between function_ptr and func_address is.

My understanding/intention here is that function_ptr is just the pointer to the function while func_address is the page address (maybe it should be renamed)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated 👉 8e00241

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might get lost but there are some comments on 8e00241.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I did missed that! sorry

- (func_address as usize)) as usize;

// Set function memory region to be writable
let result = mprotect(
func_address,
page_size_aligned,
PROT_READ | PROT_WRITE | PROT_EXEC,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need the PROT_EXEC here since you are changing it back to executable again below. In fact, on some systems you are not allowed to mark a region writeable and executable at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.
But when I remove PROT_EXEC I get some of the tests failing in a non-deterministic way 🤔
I guess its because somehow test execution overlaps and this runtime patching takes effect while other tests try to execute these instructions.
I get the same result when I run the tests sequentially as:

YKB_TRACER=swt cargo test -- --test-threads=1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, on some systems you are not allowed to mark a region writeable and executable at the same time.

I think Lukas's point is that you can mprotect(PROT_READ | PROT_WRITE) or mprotect(PROT_READ | PROT_EXEC) but you can't combine PROT_EXEC and PROT_WRITE: you have to write the page and only then mark it as executable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated 👉 40c0696
I get that :)
My issue was with failing the test, but I can't reproduce it anymore 😶

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that I can't combine PROT_WRITE and PROT_EXEC but..

I again get this issue with failing tests if I set PROT_READ | PROT_WRITE before callingcopy_nonoverlapping (patching the function):

Exited due to signal: 11

But when I set it as PROT_READ | PROT_WRITE | PROT_EXEC it works fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The normal trick here is: set to PROT_READ|PROT_WRITE do the writes; then set it to PROT_READ | PROT_EXEC. That way you never have PROT_WRITE and PROT_EXEC set at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand and I wish it would work for me but I get segfault when I do it.
Will dig deeper.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm going mad 🙃
Its working fine with PROT_READ | PROT_WRITE on patch and PROT_READ | PROT_EXEC restore.
updated 👉 6f4bdc3

);
if result != 0 {
panic!("Failed to change memory protection to be writable");
}

// Set function memory region back to be non-writable
std::ptr::copy_nonoverlapping(code, function_ptr as *mut u8, size);

let result = mprotect(func_address, page_size_aligned, PROT_READ | PROT_EXEC);
if result != 0 {
panic!("Failed to change memory protection to not writable");
}
}

fn is_x86() -> bool {
std::env::consts::ARCH == "x86" || std::env::consts::ARCH == "x86_64"
}

/// This function is used to patch the `yk_trace_basicblock`
/// function with a single `ret` (0xC3) instruction.
#[cfg(tracer_swt)]
pub(crate) unsafe fn patch_trace_function() {
if !is_x86() {
ltratt marked this conversation as resolved.
Show resolved Hide resolved
panic!("Only x86_64 architecture is supported for runtime patching!");
}
ORIGINAL_INSTRUCTIONS_INIT.call_once(|| {
save_original_instructions(
yk_trace_basicblock as usize,
ORIGINAL_INSTRUCTIONS.as_mut_ptr(),
1,
);
});

patch_function(yk_trace_basicblock as usize, PATCH_INSTRUCTIONS.as_ptr(), 1);
}

/// This function is used to restore the original behavior of a
/// previously patched `yk_trace_basicblock` function.
#[cfg(tracer_swt)]
pub(crate) unsafe fn restore_trace_function() {
if !is_x86() {
panic!("Only x86_64 architecture is supported for runtime patching!");
}
ORIGINAL_INSTRUCTIONS_INIT.call_once(|| {
save_original_instructions(
yk_trace_basicblock as usize,
ORIGINAL_INSTRUCTIONS.as_mut_ptr(),
1,
);
});
patch_function(
yk_trace_basicblock as usize,
ORIGINAL_INSTRUCTIONS.as_ptr(),
1,
);
}

#[cfg(test)]
mod patch_tests {
use super::*;

fn test_function() -> i32 {
return 42;
}

#[test]
fn test_runtime_patch() {
unsafe {
assert_eq!(test_function(), 42);
save_original_instructions(
test_function as usize,
ORIGINAL_INSTRUCTIONS.as_mut_ptr(),
1,
);
patch_function(test_function as usize, PATCH_INSTRUCTIONS.as_ptr(), 1);
assert_eq!(test_function(), 0);
patch_function(test_function as usize, ORIGINAL_INSTRUCTIONS.as_ptr(), 1);
assert_eq!(test_function(), 42);
}
}
}