-
-
Notifications
You must be signed in to change notification settings - Fork 321
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
Introspection #58
Introspection #58
Conversation
… need to work on getting granular information from `Feedback` and `Observer`
…into real_time_benchmarks
Seems like all it's missing is a good old |
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.
Cool stuff
libafl/src/stats/mod.rs
Outdated
const NUM_STAGES: usize = 4; | ||
|
||
/// Number of feedback mechanisms to measure for performance | ||
pub const NUM_FEEDBACKS: usize = 4; |
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.
Maybe this can be generated at builtime somehow? Since feedback tuples are generated at build time
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.
Hmm.. yeah i'll start looking into how this could be re-worked to be known at compile time specific to the target being compiled.
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.
Initially, I think this would clog up the code a bit, since i /think/ we would have to have something like:
pub struct ClientPerfStats<const S: usize, const F: usize> {
...
stages_used: [bool; NUM_STAGES]
... ect
}
which would mean then the ClientPerfs
also needs it:
struct ClientPerfs<const S: usize, const F: usize> {
....
perf_stats: ClientPerfStats<S, F>
}
which would then mean all instances of ClientStats
would have these across the entire code base.
I /think/ we could have a compile time assert that checks on State
creation if the length of the stages or feedback is greater than the number of alloted spaces in the perf stats and maybe print an error if that is the case. Not quite as automatic, though and would require the researcher change the variable if the error triggered.
Thoughts?
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.
Thinking a bit more about this.. I think we would need to have some const fn len()
on FeedbackTuple
and StagesTuple
. I don't think const fn
can be used in a trait from rust-lang/rfcs#2632. So I think the only option here is to add a run-time check that fails if too many stages or feedbacks are added.
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.
Now if you sync from dev HasLen has a const LEN field, you can just do ST::LEN for the cost len of the StagesTuple for instance
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.
FWIW Just tried it, but ST::LEN
doesn't work yet everywhere. It's not available at some places and we would need nightly features at others. We could use Vec
s instead of arrays, though. Not sure if/how much this would impact perf.
Or just leave it as-is...
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.
The arrays are there to ensure we don't do any allocations. I don't mind either way really. We could always just do a vec::with_capacity I guess
|
||
/// Update the total stage counter and increment the stage counter for the next stage | ||
pub fn finish_stage(&mut self) { | ||
// Increment the stage to the next index. The check is only done if this were to |
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.
These short functions may benefit from [inline]
(maybe not necessary with LTO builds?)
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.
Yeah I can add a few just in case for the small ones
Awseome! What is necessary to make this aarch64 compatible? |
/// the fuzzer. | ||
#[cfg(target_arch = "x86_64")] | ||
pub fn read_time_counter() -> u64 { | ||
unsafe { core::arch::x86_64::_rdtsc() } |
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.
Might be worth using https://docs.rs/llvmint/0.0.3/llvmint/fn.readcyclecounter.html, as that will automatically do the right thing for each arch.
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.
Sounds really useful for more obscure platforms!
We should only add the dependency for this feature flag though (and maybe even only for the platforms we can not reach otherwise?)
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.
Oh sure! Didn't know about the llvm intrinsic. If we're okay with the llvmint
dependency then I'm fine with using it.
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.
The original thought was to add those specific instructions similar to Google's benchmark: https://github.com/google/benchmark/blob/master/src/cycleclock.h
I think that would mitigate the external dependency just for one instruction per arch.
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.
But that would require asm... which is not stable.
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.
let's go with
libc
, if available for this archllvmint
dependency otherwise
?
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.
Or since it's a feature flag anyway, just llvmint is fine too, if it's too much work otherwise
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.
Just pushed a fix using llvmint::readcyclecounter
. Looks like the feature needed requires nightly
. I think you guys are wanting to stay on stable
as far as I'm aware, which would mean neither this or the asm!
fix would work. I guess we could also require nightly
for the perf_stats
feature as well.
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.
Edit from the last comment.. i had the wrong cfg_attr
. I've ripped out only the needed extern from llvmint
so we don't need to add that dependency directly to src/cpu.rs
and it seems to work fine now.
…link_llvm_intrinsics on the perf_stats feature
libafl/src/lib.rs
Outdated
@@ -3,6 +3,7 @@ Welcome to libAFL | |||
*/ | |||
|
|||
#![cfg_attr(not(feature = "std"), no_std)] | |||
#![cfg_attr(feature = "perf_stats", feature(link_llvm_intrinsics))] |
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.
Ins't this feature perma-unstable? Might be worth depending on llvmint
instead of requiring nightly.
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 so too. Seems like the stable and hardware-independent way to go for now
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 plan to use it in libafl_frida
anyways, if that helps.
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.
Oof... llvmint
depends on simdty
which uses nightly features...
Not good.
We can either do our own C wrapper or go back to the initial proposal to do it using libc
...
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.
Ok so we do want to enforce std
features. I can convert everything to just using Instant
rather than the pure u64
. It won't work for no_std
obviously. I guess I forgot not everyone doesn't run on nightly
all the time ;-)
I guess asking to run in nightly
for perf_stats
might be a big ask as well?
Yeah llvmint
relying on simdty
was the reason I ripped out just the extern in the first place.
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 the C wrapper is the way to go...
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.
C wrappers are the way to go but be careful, Rust code is compiled with LLVM but the C code may be compiled with GCC or MSVC, depends on the build.rs.
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.
Yeah, I discovered the hard way that __builtin_thread_pointer
is not universally accepted.
@ctfhacker how about renaming this feature to |
Not a problem on my end. Note, the current setup uses clock cycles and not seconds, so we'd have to get the clock speed to covert to seconds. Or just going straight away to Duration if that is a use case we already know about. |
Seconds was just an example. Clock cycles is fine, as long as it's measured in the same way and is fast doing so :) |
I've renamed it to |
* initial b2b implementation * no_std and clippy fixes * b2b testcase added * more correct testcases * fixed b2b * typo * fixed unused warning
I've done some very very rudimentary measurements and, while using |
I will try to remove the hardcoded NUM_FEEDBACKS if I can and then merge |
ok needs a rework to remove thes consts. Btw, NUM_FEEDBACKS = 4 is too few IMO, should be 16 |
@@ -230,7 +230,7 @@ where | |||
let client = stats.client_stats_mut_for(sender_id); | |||
client.update_corpus_size(*corpus_size as u64); | |||
client.update_executions(*executions as u64, *time); | |||
stats.display(event.name().to_string() + " #" + &sender_id.to_string()); | |||
// stats.display(event.name().to_string() + " #" + &sender_id.to_string()); |
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.
@domenukk porcodio, now we have to release again because this line was commented
@@ -241,7 +241,35 @@ where | |||
// TODO: The stats buffer should be added on client add. | |||
let client = stats.client_stats_mut_for(sender_id); | |||
client.update_executions(*executions as u64, *time); | |||
stats.display(event.name().to_string() + " #" + &sender_id.to_string()); | |||
if sender_id == 1 { |
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.
Why this?
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.
Shouldn't be needed, I agree
This PR adds the
perf_stats
feature tolibafl
which would enable performance metrics to each fuzzing instance. This should mostly be used for debugging to further understand what percentage of time is being spent where in the fuzzing pipeline.Default metrics:
With performance metrics:
The metrics are stored in the
ClientPerfStats
struct which is added to theClientStats
struct only when theperf_stats
feature is enabled. The timing is performed usingrdtsc
on x86 (no other archs have been implemented thus far), but is built usingcpu::read_time_counter
which is also only built forx86
. Other archs will have a compile time error that this function has not been created.The bulk of the new code is in
State::execute_input
where timers are added (only withperf_stats
feature) around each phase of the fuzzing of an input. In this way, researchers can understand the performance ramifications of the components under experiment.The tests were done in the
libfuzzer_libpng
example.Performance with performance monitoring
The follow experiment was ran with the
libfuzzer_libpng
example fuzzer.These were executed on the following CPU:
The stats here are a bit weird in the sense that with
perf_stats
, there seems to be moreexec/sec
. I haven't quite figured out why yet, but was hoping someone else could verify on their system as well.Test without
perf_stats
with 1 fuzzerTest with
perf_stats
with 1 fuzzerTest without
perf_stats
with 3 fuzzersTest with
perf_stats
with 3 fuzzers