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

add the metrics(pending,pend_fav, own_finds,imported) #1351

Merged
merged 17 commits into from
Sep 6, 2023

Conversation

ToSeven
Copy link
Contributor

@ToSeven ToSeven commented Jul 6, 2023

No description provided.

@ToSeven
Copy link
Contributor Author

ToSeven commented Jul 6, 2023

Sorry to close this PR #1332 by accident

libafl/src/fuzzer/mod.rs Outdated Show resolved Hide resolved
@@ -624,7 +627,13 @@ impl<E, S, SP, Z> EventManager<E, Z> for LlmpEventManager<S, SP>
where
E: HasObservers<State = S> + Executor<Self, Z>,
for<'a> E::Observers: Deserialize<'a>,
S: UsesInput + HasExecutions + HasClientPerfMonitor + HasMetadata,
S: UsesInput
Copy link
Member

Choose a reason for hiding this comment

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

you don't need these type constraints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image There is an error.

Copy link
Member

Choose a reason for hiding this comment

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

hm but why? can you give me full error log?

Copy link
Member

Choose a reason for hiding this comment

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

because here you don't even use those methods then why is it necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Member

@tokatoka tokatoka Jul 6, 2023

Choose a reason for hiding this comment

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

but you don't need this in eventmanager no?

Copy link
Member

Choose a reason for hiding this comment

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

I mean you could just store imported, pending in state, and don't make any traits.
then you don't need these type constraints anymore

Copy link
Contributor Author

@ToSeven ToSeven Jul 6, 2023

Choose a reason for hiding this comment

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

hmm, I merge all 4 metrics into one trait HasAFLStats, then I just need to type one constraint. how about?

Copy link
Member

Choose a reason for hiding this comment

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

ok

libafl/src/state/mod.rs Outdated Show resolved Hide resolved
@tokatoka
Copy link
Member

tokatoka commented Jul 6, 2023

can you remove the unnecessary traits?

@ToSeven ToSeven force-pushed the main branch 5 times, most recently from a5c3d56 to 5e9c5d7 Compare July 7, 2023 13:12
@tokatoka
Copy link
Member

tokatoka commented Jul 7, 2023

is the UI supposed to run on a different process other than the main fuzzer?
@vanhauser-thc

@vanhauser-thc
Copy link
Member

is the UI supposed to run on a different process other than the main fuzzer?
@vanhauser-thc

imho out of the fuzzer process is best, @domenukk was first fighting hard for in broker process then thought about it and … not sure what he would want now :)

@ToSeven ToSeven changed the title add the metrics(pending,own_finds,imported) add the metrics(pending,pend_fav, own_finds,imported) Jul 8, 2023
@ToSeven
Copy link
Contributor Author

ToSeven commented Jul 8, 2023

Please review my code.

@vanhauser-thc
Copy link
Member

@ToSeven can you please resolve the conflicts?

libafl/src/state/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@tokatoka tokatoka left a comment

Choose a reason for hiding this comment

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

I said "how about putting code in calibration stage" before.

but now i think it feels odd to do the stats stuff in calibration stage.
can you separate the code in calibration stage into another stage like StatsReportsStage? (You need to make another stage for this)

@tokatoka
Copy link
Member

@ToSeven

Merge https://github.com/AFLplusplus/LibAFL/tree/stats_stage to your code.
I've made a stage where you should put all your stats-related code.

@ToSeven
Copy link
Contributor Author

ToSeven commented Jul 14, 2023

@ToSeven can you please resolve the conflicts?

ok

@vanhauser-thc
Copy link
Member

also please run cargo fmt and cargo clippy to ensure clean code. maybe you did that but I see things in the code that I think clippy would not allow :)
but besides this and my one comment I think we are close :)
@tokatoka ?

@domenukk
Copy link
Member

domenukk commented Jul 18, 2023

is the UI supposed to run on a different process other than the main fuzzer?
@vanhauser-thc

imho out of the fuzzer process is best, @domenukk was first fighting hard for in broker process then thought about it and … not sure what he would want now :)

As long as it adheres to the Monitor API it can be out of process or in process without minimal code changes ( we can create a Proxy monitor that forwards to an out of process monitor)

@@ -404,6 +439,55 @@ impl<I, C, R, SC> HasExecutions for StdState<I, C, R, SC> {
}
}

impl<I, C, R, SC> HasAFLStats for StdState<I, C, R, SC> {
Copy link
Member

Choose a reason for hiding this comment

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

Should be HasAflStats for consistency.
That being said, maybe there is an even nicer name? We'll need that guy everywhere.

if cur.checked_sub(self.last_report_time).unwrap_or_default() > self.stats_report_interval {
manager.fire(
state,
Event::UpdateUserStats {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should fire one message with all stats in them, else this could lead to more overhead on the receiving end since UI might have to be updated for each new UserStats message, etc

@@ -787,6 +871,10 @@ where
let mut state = Self {
rand,
executions: 0,
pending: 0,
Copy link
Member

Choose a reason for hiding this comment

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

can you add this member in stages/stats.rs?

Copy link
Member

Choose a reason for hiding this comment

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

instead of putting in state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how about adding the "pending" " pend_favor" "own_finds" to stages/stats.rs and remaining the "imported" in state?
I found it not easy to calculate the value of the "imported" in stats.rs. @tokatoka

@tokatoka
Copy link
Member

ideally your change should be only inside stages/stats.rs

@domenukk domenukk enabled auto-merge (squash) August 11, 2023 11:56

/// The [`AFLStatsStage`] is a simple stage that computes and reports some stats.
#[derive(Debug, Clone)]
pub struct AFLStatsStage<E, EM, Z>
Copy link
Member

Choose a reason for hiding this comment

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

We usually use Afl (lowercase fl)

@@ -256,6 +259,7 @@ where
+ HasMetadata
+ HasRand
+ HasCorpus
+ HasImported
Copy link
Member

Choose a reason for hiding this comment

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

push adapter doesn't need this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah

@domenukk
Copy link
Member

We moved the libafl::bolts module to the libafl_bolts create, that's a big reason this PR is failing right now :)

@domenukk
Copy link
Member

We should try this PR together with #1432, maybe create (or extend) a fuzzer in ./fuzzers that uses the StatsStage

@ToSeven
Copy link
Contributor Author

ToSeven commented Sep 4, 2023

We should try this PR together with #1432, maybe create (or extend) a fuzzer in ./fuzzers that uses the StatsStage

I will create a fuzzer with the AFL-Style UI in ./fuzzers

Copy link
Member

@tokatoka tokatoka left a comment

Choose a reason for hiding this comment

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

we can merge this after you resolve the conflict and fix CI.

auto-merge was automatically disabled September 5, 2023 08:58

Head branch was pushed to by a user without write access

@domenukk
Copy link
Member

domenukk commented Sep 6, 2023

Thanks!

@domenukk domenukk merged commit 04aecd9 into AFLplusplus:main Sep 6, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants