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

EventProcessor should not exist #2910

Open
tokatoka opened this issue Jan 29, 2025 · 3 comments
Open

EventProcessor should not exist #2910

tokatoka opened this issue Jan 29, 2025 · 3 comments
Assignees

Comments

@tokatoka
Copy link
Member

tokatoka commented Jan 29, 2025

We are trying to add EM, Z to the executors here #2873, which I think is a correct thing
Now the problem is that we have this code.

impl<E, EMH, I, S, SHM, SP, Z> EventProcessor<E, S, Z>
    for LlmpRestartingEventManager<EMH, I, S, SHM, SP>
where
    E: HasObservers,
    E::Observers: DeserializeOwned,
    EMH: EventManagerHooksTuple<I, S>,
    I: DeserializeOwned + Input,
    S: HasImported + HasCurrentTestcase<I> + HasSolutions<I> + Stoppable + Serialize,
    SHM: ShMem,
    SP: ShMemProvider<ShMem = SHM>,
    Z: ExecutionProcessor<Self, I, E::Observers, S> + EvaluatorObservers<E, Self, I, S>,

and this
Z: ExecutionProcessor<Self, I, E::Observers, S> + EvaluatorObservers<E, Self, I, S>, is not letting me to compile things, if, for example, CentralizedEventManager is used. Because the fuzzer's EM is CentralizedEventManager, but this implementation's Self is LlmpRestartingEventManager so it won't compile.

To me the biggest problem here is that EventProcessor defined on event manager. I think we should make this a public function...
and delete this EventProcessor trait.

@tokatoka tokatoka changed the title EventProcessor should be defined on Fuzzer, not EventManger or it should not exist at all. EventProcessor should be defined on Fuzzer, not EventManger. or it should not exist at all. Jan 29, 2025
@tokatoka tokatoka self-assigned this Jan 29, 2025
@tokatoka
Copy link
Member Author

tokatoka commented Jan 29, 2025

The problem I see here is that
this code is doing two stuff.

  1. receiving testcase from llmp comm channel. this is the job of event manager
  2. evaluating the incoming testcase. but this is not the job of event manager

This code is solely called from fuzzer/mod.rs in fuzz_loop
therefore I propose to change this EventProcessor to EventReceiver which only focuses on the first job.
then we can move the 2nd job to fuzzer/mod.rs instead. and somehow redesign the logic of incoming-testcase processing

@tokatoka
Copy link
Member Author

@domenukk
opinion?
I think this is the core of all the problem
not just the problem that the code doesn't compile. but also the bloated trait bounds in event manager module

@tokatoka tokatoka changed the title EventProcessor should be defined on Fuzzer, not EventManger. or it should not exist at all. EventProcessor should not exist Jan 29, 2025
@domenukk
Copy link
Member

Yes the evaluation doesn't have to be part of event manager. Sounds like we might even be able to give the event manager a callback of sorts that does the eval?

A simple public fn might be an issue if someone wants to change the behavior in their own fuzzer

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

No branches or pull requests

2 participants