-
Notifications
You must be signed in to change notification settings - Fork 56
Register tracing export #178
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
base: main
Are you sure you want to change the base?
Changes from 12 commits
4684027
7a28d76
515423f
14f498b
932a465
5367f76
727bf38
46dd6ac
406f474
502c9e7
381f79f
3af9774
456778b
53c71c1
23b360c
4e0d934
4129ea5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -446,6 +446,8 @@ pub mod file; | |||
| #[cfg(any(feature = "fuzz", feature = "fuzz-fd"))] | ||||
| pub mod fuzz; | ||||
| pub mod program; | ||||
| #[cfg(feature = "register-tracing")] | ||||
| pub mod register_tracing; | ||||
| pub mod sysvar; | ||||
|
|
||||
| // Re-export result module from mollusk-svm-result crate | ||||
|
|
@@ -509,6 +511,8 @@ pub struct Mollusk { | |||
| /// programs comes from the sysvars. | ||||
| #[cfg(feature = "fuzz-fd")] | ||||
| pub slot: u64, | ||||
|
|
||||
| pub enable_register_tracing: bool, | ||||
|
||||
| ..Default::default() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah crud. What about just using the environment variable everywhere for the logic gate? The problem is, if someone flips this to true on Mollusk directly, but doesn't provide the SBF_TRACE_DIR, it's going to disable tracing anyway. This would be confusing for people.
Alternatively, we could choose a default dir under target if we really want this to be public. However, doing something like this:
let mut mollusk = Mollusk::default();
mollusk.enable_register_tracing = true;
......would still not add the callback and not activate tracing, as per the current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah crud. What about just using the environment variable everywhere for the logic gate? The problem is, if someone flips this to
trueon Mollusk directly, but doesn't provide theSBF_TRACE_DIR, it's going to disable tracing anyway. This would be confusing for people.
You're right - it would be confusing. There's another case that also concerns me - it's related to the environment variable only approach. As I'm going to write some tests for this feature it's that I would need to set the envvar from within the test itself. Setting envvars is unsafe as tests are run in the same process but in different threads. Hence having multiple tests playing with the SBF_TRACE_DIR var is UB. This is the reason I added with_register_tracing in the first place - just having a constructor that has the ability to instantiate mollusk with register tracing but without the need to play with the envvar.
Alternatively, we could choose a default dir under
targetif we really want this to be public. However, doing something like this:
I think it's probably a good idea to have a default path.
let mut mollusk = Mollusk::default(); mollusk.enable_register_tracing = true; ......would still not add the callback and not activate tracing, as per the current implementation.
I think I came up with some sort of a possible solution - I can bury the enable_register_tracing bool in the callback structure - thus nobody would be able to set it without actually providing a handler - I'll try to rework it to see the final outcome and will show it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting envvars is unsafe as tests are run in the same process but in different threads. Hence having multiple tests playing with the SBF_TRACE_DIR var is UB.
Why wouldn't the SBF_TRACE_DIR variable just be readonly across all threads?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I came up with some sort of a possible solution - I can bury the enable_register_tracing bool in the callback structure - thus nobody would be able to set it without actually providing a handler - I'll try to rework it to see the final outcome and will show it here.
Cool! Ping me when you throw it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed the changes. I've also added one test implementing a custom register tracing callback counting the number of JMP class instructions executed. Tests help me reason about the API and its consumption later. On one side it's now that you can tweak existing codebases with the SBF_TRACE_DIR and on the other side it's possible to enable register tracing without the need to tweak the same environment variable at all - hence the with_register_tracing constructor.
What I don't like up to now is that you can instantiate Mollusk::default() and change the callback expecting this will enable register tracing. Unfortunately it won't because it's the ProgramCache that also needs to have register tracing tweaked. Yet doesn't such post-fact changes also not work for the rest of the Mollusk fields where there's a cross dependency between some of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting envvars is unsafe as tests are run in the same process but in different threads. Hence having multiple tests playing with the SBF_TRACE_DIR var is UB.
Why wouldn't the
SBF_TRACE_DIRvariable just be readonly across all threads?
It could be but if we need to enable tracing for one test and skip it for another the envvar is not very convenient IMO.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this new constructor? The existing Mollusk::new will call into Self::default, and the logic gate where you check for the SBF_TRACE_DIR environment variable will be evaluated. So people can just use Mollusk::new!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this new constructor? The existing
Mollusk::newwill call intoSelf::default, and the logic gate where you check for theSBF_TRACE_DIRenvironment variable will be evaluated. So people can just useMollusk::new!
I'd be glad to have your review up to now and if you prefer I'll rework it to have everything gated behind the environment variable - I have in mind something like this:
diff --git a/harness/src/lib.rs b/harness/src/lib.rs
index 815ec07..b89830f 100644
--- a/harness/src/lib.rs
+++ b/harness/src/lib.rs
@@ -689,32 +689,6 @@ impl Mollusk {
mollusk
}
- /// Create a new Mollusk instance just like the `new` method but
- /// with register tracing enabled using a default callback.
- ///
- /// If `SBF_TRACE_DIR` is set it will override the passed `sbf_trace_dir`.
- #[cfg(feature = "register-tracing")]
- pub fn with_register_tracing(
- program_id: &Pubkey,
- program_name: &str,
- sbf_trace_dir: &str,
- ) -> Self {
- let mut mollusk = Mollusk::default();
- mollusk.invocation_inspect_callback =
- Box::new(register_tracing::DefaultRegisterTracingCallback {
- sbf_trace_dir: std::env::var("SBF_TRACE_DIR").unwrap_or(sbf_trace_dir.into()),
- });
- mollusk.program_cache = ProgramCache::new(
- &mollusk.feature_set,
- &mollusk.compute_budget,
- mollusk
- .invocation_inspect_callback
- .is_register_tracing_callback(),
- );
- mollusk.add_program(program_id, program_name, &DEFAULT_LOADER_KEY);
- mollusk
- }
-
/// Add a program to the test environment.
///
/// If you intend to CPI to a program, this is likely what you want to use.
diff --git a/harness/src/register_tracing.rs b/harness/src/register_tracing.rs
index ddb3cc0..7c18b1e 100644
--- a/harness/src/register_tracing.rs
+++ b/harness/src/register_tracing.rs
@@ -85,7 +85,7 @@ impl InvocationInspectCallback for DefaultRegisterTracingCallback {
// This callback is specifically implemented to handle register tracing.
fn is_register_tracing_callback(&self) -> bool {
- true
+ std::env::var("SBF_TRACE_DIR").is_ok()
}
}
diff --git a/harness/tests/register_tracing.rs b/harness/tests/register_tracing.rs
index ba9e85c..0c30d15 100644
--- a/harness/tests/register_tracing.rs
+++ b/harness/tests/register_tracing.rs
@@ -87,11 +87,11 @@ fn test_custom_register_tracing_callback() {
}
std::env::set_var("SBF_OUT_DIR", "../target/deploy");
+ std::env::set_var("SBF_TRACE_DIR", "/tmp/sbf_trace_dir");
let program_id = Pubkey::new_unique();
let payer_pk = Pubkey::new_unique();
- let mut mollusk =
- Mollusk::with_register_tracing(&program_id, "test_program_primary", "sbf_trace_dir");
+ let mut mollusk = Mollusk::new(&program_id, "test_program_primary");
// Phase 1 - basic register tracing test.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| use { | ||
| crate::{ | ||
| file::{default_shared_object_dirs, read_file}, | ||
| InvocationInspectCallback, | ||
| }, | ||
| sha2::{Digest, Sha256}, | ||
| solana_program_runtime::invoke_context::{Executable, InvokeContext, RegisterTrace}, | ||
| solana_pubkey::Pubkey, | ||
| solana_transaction_context::{InstructionAccount, InstructionContext}, | ||
| std::{fs::File, io::Write, path::PathBuf}, | ||
| }; | ||
|
|
||
| pub struct DefaultRegisterTracingCallback; | ||
|
|
||
| impl InvocationInspectCallback for DefaultRegisterTracingCallback { | ||
| fn before_invocation(&self, _: &Pubkey, _: &[u8], _: &[InstructionAccount], _: &InvokeContext) { | ||
| } | ||
|
|
||
| fn after_invocation(&self, invoke_context: &InvokeContext) { | ||
| invoke_context.iterate_vm_traces( | ||
| &|instruction_context: InstructionContext, | ||
| executable: &Executable, | ||
| register_trace: RegisterTrace| { | ||
| if let Err(e) = default_register_tracing_callback( | ||
| instruction_context, | ||
| executable, | ||
| register_trace, | ||
| ) { | ||
| eprintln!("Error collecting the register tracing: {}", e); | ||
| } | ||
| }, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| pub fn default_register_tracing_callback( | ||
| instruction_context: InstructionContext, | ||
| executable: &Executable, | ||
| register_trace: RegisterTrace, | ||
| ) -> Result<(), Box<dyn std::error::Error>> { | ||
| if register_trace.is_empty() { | ||
| // Can't do much with an empty trace. | ||
| return Ok(()); | ||
| } | ||
|
|
||
| if let Ok(sbf_trace_dir) = &std::env::var("SBF_TRACE_DIR") { | ||
procdump marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| let current_dir = std::env::current_dir()?; | ||
| let sbf_trace_dir = current_dir.join(sbf_trace_dir); | ||
| std::fs::create_dir_all(&sbf_trace_dir)?; | ||
|
|
||
| let trace_digest = compute_hash(as_bytes(register_trace)); | ||
| let base_fname = sbf_trace_dir.join(&trace_digest[..16]); | ||
| let mut regs_file = File::create(base_fname.with_extension("regs"))?; | ||
| let mut insns_file = File::create(base_fname.with_extension("insns"))?; | ||
| let mut so_hash_file = File::create(base_fname.with_extension("exec.sha256"))?; | ||
|
|
||
| // Get program_id. | ||
| let program_id = instruction_context.get_program_key()?; | ||
|
|
||
| // Persist the preload hash of the executable. | ||
| let _ = so_hash_file.write( | ||
| find_executable_pre_load_hash(executable) | ||
| .ok_or(format!( | ||
| "Can't find shared object for executable with program_id: {program_id}" | ||
| ))? | ||
| .as_bytes(), | ||
| ); | ||
|
|
||
| // Get the relocated executable. | ||
| let (_, program) = executable.get_text_bytes(); | ||
| for regs in register_trace.iter() { | ||
| // The program counter is stored in r11. | ||
| let pc = regs[11]; | ||
| // From the executable fetch the instruction this program counter points to. | ||
| let insn = | ||
| solana_program_runtime::solana_sbpf::ebpf::get_insn_unchecked(program, pc as usize) | ||
| .to_array(); | ||
|
|
||
| // Persist them in files. | ||
| let _ = regs_file.write(as_bytes(regs.as_slice()))?; | ||
| let _ = insns_file.write(insn.as_slice())?; | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| pub(crate) fn as_bytes<T>(slice: &[T]) -> &[u8] { | ||
| unsafe { std::slice::from_raw_parts(slice.as_ptr() as *const u8, std::mem::size_of_val(slice)) } | ||
| } | ||
|
|
||
| fn find_so_files(dirs: &[PathBuf]) -> Vec<PathBuf> { | ||
| let mut so_files = Vec::new(); | ||
|
|
||
| for dir in dirs { | ||
| if dir.is_dir() { | ||
| if let Ok(entries) = std::fs::read_dir(dir) { | ||
| for entry in entries.flatten() { | ||
| let path = entry.path(); | ||
| if path.is_file() && path.extension().is_some_and(|ext| ext == "so") { | ||
| so_files.push(path); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| so_files | ||
| } | ||
|
|
||
| fn find_executable_pre_load_hash(executable: &Executable) -> Option<String> { | ||
| find_so_files(&default_shared_object_dirs()) | ||
| .iter() | ||
| .filter_map(|file| { | ||
| let so = read_file(file); | ||
| // Reconstruct a loaded Exectuable just to compare its relocated | ||
| // text bytes with the passed executable ones. | ||
| // If there's a match return the preload hash of the corresponding shared | ||
| // object. | ||
| Executable::load(&so, executable.get_loader().clone()) | ||
| .ok() | ||
| .map(|e| Some((so, e))) | ||
| .unwrap_or(None) | ||
| }) | ||
| .filter(|(_, e)| executable.get_text_bytes().1 == e.get_text_bytes().1) | ||
| .map(|(so, _)| compute_hash(&so)) | ||
| .next_back() | ||
| } | ||
|
|
||
| fn compute_hash(slice: &[u8]) -> String { | ||
| hex::encode(Sha256::digest(slice).as_slice()) | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.