Remove the Fuzzer dependency from Executor

[riesentoaster]

No description provided.

[riesentoaster]

This is the only place where the ExecutionProcessor constraint appears. Things seem to work without it, so I'm not sure why it's there.

[tokatoka]

i like the change.
yeah probably fuzzer is not needed for run_target
i think the only thing that we cannot get is the scheduler and feedback and i don't think people wants to get that from inside run_target (?)

what do you think @domenukk

meanwhile can you fix the ci?

[domenukk]

Why do we need the objectives? For crash handling? Why not also pass in Feedback? What if I want to mark timeouts interesting or some stuff like that?

[tokatoka]

What if I want to mark timeouts interesting or some stuff like that?

hmm yeah that's a problem..

[tokatoka]

@riesentoaster
i think if you can add feedbacks then it's ok..

[riesentoaster]

Why would the executor directly call/change feedbacks anyway? Shouldn't this be done through observers? That seems like the canonical way to pass information around.

[riesentoaster]

Actually, when thinking about this some more, I think Executor should not receive a reference to objective either. If an executor needs a reference to this, it should be passed in the constructor of the executor.

[riesentoaster]

And the entire thing is a bit of a mess anyway. Some opinions (although this would be a major restructuring and out of scope for this PR):

• Executor should have two methods:
  • One with a default implementation doing all the more-or-less constant operation on state (like increasing the execution and reseting observers).
  • The other should just execute the input and only receive a reference to the input.
  • If an executor really needs access to state in addition to the constant logic across all executors, one can always override the default impl of the first function, although I'm not sure why that would ever be necessary (the pointer collection in InProcessExecutors should again happen in the constructor).
• I'm not sure why pre_exec_child/post_exec_child on the observer exist — there doesn't seem to be any implementation in the repo besides a copy-paste impl of pre_exec/post_exec. Removing this would further remove places state is necessary in the execution.

[tokatoka]

Why would the executor directly call/change feedbacks anyway? Shouldn't this be done through observers? That seems like the canonical way to pass information around.

because of crash handlers. crash handler needs feedbacks and objectives
you need to write the pointers of them at each harness execution
why not write them at initialization? because they might move their address.

[riesentoaster]

Why would the address of feedbacks/objectives change?

[tokatoka]

because it is not wrapped in Pin? i think

[domenukk]

I mean we could just let executor own feedback and objective?
Other option might be to have the crash handler as closure that captures them and gets set for every exec(?)

[tokatoka]

I mean we could just let executor own feedback and objective? Other option might be to have the crash handler as closure that captures them and gets set for every exec(?)

no it's a problem for the Fuzzer.
Fuzzer doesn't own executor. but it needs reference to objectives and feedbacks to work. that's why currently fuzzer owns them

[riesentoaster]

So I've just reimplemented Executor to only receive state and input, and that would simplify a lot of things, across both executors and stages. Quite a few extra generics that could get removed. Plus it is just much cleaner dependency-wise. I think this would be a good overall improvement.

Also: TIL about Pin. I assume wrapping objectives and feedbacks in Pin would get really annoying. I don't think that's feasible.

Somewhat related: I thought about the fuzzer some more. I think we should implement a separate Evaluator struct at some point, which would then get passed to the stages instead of the entire fuzzer. This would also mean there would be a good interface for all the replaying-for-reliability or filtering we've just bodged into fuzzer. It would also mean fuzzer doesn't need to pass itself to other functions, which is always ugly.

InProcessExecutor needs access to the objectives and managers to perform logic that is very similar to the evaluator (evaluate objectives, create testcase, append metadata, put in state). So shouldn't that be handled in the evaluator somewhere? This would mean we'd need some tighter coupling between the evaluator and executor, and I'm not quite sure how this would work, but in the evaluator we have all the necessary references and it's where the code is otherwise.

[tokatoka]

So I've just reimplemented Executor to only receive state and input, and that would simplify a lot of things, across both executors and stages. Quite a few extra generics that could get removed. Plus it is just much cleaner dependency-wise. I think this would be a good overall improvement.

How can you resolve the problem that we need to pass the pointers?

Somewhat related: I thought about the fuzzer some more. I think we should implement a separate Evaluator struct at some point, which would then get passed to the stages instead of the entire fuzzer. This would also mean there would be a good interface for all the replaying-for-reliability or filtering we've just bodged into fuzzer. It would also mean fuzzer doesn't need to pass itself to other functions, which is always ugly.

Yes, fuzzer struct needs some changes. fuzzer struct pasing itself to others are also causing me another problem. i'm thinking about #2894

InProcessExecutor needs access to the objectives and managers to perform logic that is very similar to the evaluator (evaluate objectives, create testcase, append metadata, put in state). So shouldn't that be handled in the evaluator somewhere? This would mean we'd need some tighter coupling between the evaluator and executor, and I'm not quite sure how this would work, but in the evaluator we have all the necessary references and it's where the code is otherwise.

(btw it's the crash handler that needs it not the executor itself that needs the pointers to all the objects)
i don't fully understand. in that case we still need to pass the evaluator to the executor. then ist that any different than that current code that pass fuzzer to the executor. (or what is the difference between evaluator and fuzzer, and why passing evaluator is better?)

[riesentoaster]

How can you resolve the problem that we need to pass the pointers?

I haven't. I'm passing the objective and manager as a mut borrow to the constructor, and storing the pointer in GenericInProcessExecutorInner. And at least for baby_fuzzer, this works. To be clear: This is unsafe. I'm relying on the fact that the compiler doesn't move the objective/manager around, which isn't guaranteed.

Yes, fuzzer struct needs some changes. fuzzer struct pasing itself to others are also causing me another problem. i'm thinking about #2894

All the more reasons.

(btw it's the crash handler that needs it not the executor itself that needs the pointers to all the objects) I don't fully understand. in that case we still need to pass the evaluator to the executor. then ist that any different than that current code that pass fuzzer to the executor. (or what is the difference between evaluator and fuzzer, and why passing evaluator is better?)

I imagined that the evaluator would own (or at least have a reference to) the executor, not the other way around. So it'd essentially be a wrapper around the executor that deals with objectives.

[riesentoaster]

The changes: riesentoaster#1

[riesentoaster]

Let's move the discussion to an issue, since it involves major restructuring: #2897.

In the meantime, I would like to merge this PR as is, since it improves the situation slightly, and is necessary for other changes (#2888).

[riesentoaster]

@domenukk @tokatoka can I get a review?

[tokatoka]

to merge this, feedbacks has to be added to the argument

[riesentoaster]

I'm not sure it's worth going through all the different executors again if we're talking about restructuring them again pretty soon.

Do you know of any executor that actually use the feedback?

[tokatoka]

no there's nobody that is currently using that. but it totally makes sense to have feedback in run_target. else we cannot put testcases that timeout into the corpus

[riesentoaster]

Let's close this PR then and wait for the larger restructuring of Executor and Fuzzer.