executor: Introduce Executor "Trait Objects" by using Type Erasure #41
Conversation
retrage
force-pushed
the
feature/2022-02-28-executor-trait-objects
branch
from
b94a6f2
to
0c64660
Compare
| #include "fuzzuf/feedback/inplace_memory_feedback.hpp" | ||
| #include "fuzzuf/utils/common.hpp" | ||
|
|
||
| /** |
There was a problem hiding this comment.
The same comments as DynAFLCapable We should deal with this class after the comments in DynAFLCapable are all resolved.
There was a problem hiding this comment.
Sorry for being late. I was off last Friday.
I have some points to discuss and left them as comments. So please check them.
Answering each of your concerns:
- Type Erasure Implementation
I implemented the classes with type erasure on my own. This is usually discouraged because bugs in my own implementation of type erasure are difficult to debug and can lead to "shooting myself in the foot". However, I found that the implementation using boost/type_erasure did not meet our needs in this case, as the amount of code using boost was not much different from my own implementation, and error messages tended to be more difficult. For this reason, I decided to implement type erasure myself. There is room for discussion on this policy.
Probably using boost/type_erasure is a better solution for those who are familiar with it. This is because its components called concept and interface would make clear what we want to define as a trait and where we implement it. However, considering there are many people who don't know about boost/type_erasure (even among us!), we would have to leave lots of comments about the things that is trivial to those who know it. Perhaps, ridiculously, the amount of comments can be even larger if we use boost/type_erasure and leave thoughtful comments, compared to our own implementation.
Moreover, as you noted, depending on concepts we define, the amount of code would not be reduced, and compilation errors would be more difficult in boost/type_erasure. Therefore, I have no opposition to have our own type erasure implementation in this situation.
- Performance
Because type erasure hides the actual type, runtime performance may suffer depending on compile-time optimizations. Although I have not yet measured and compared performance, I did not experience any performance degradation when running the unit test.
Please review the type erasure classes to ensure that no unnecessary copies are made. They take std::shared_ptr as a constructor argument rather than executor itself. I believe that the executor is not created unnecessarily, but I am not sure if this implementation is correct.
Putting it simply, what type erasure does is just realizing something like polymorphism(or dynamic dispatch) without inheritance. Hence, even if we used inheritance, defining a base class and derived classes to do the same as this PR, the performance of that implementation would be the same as the performance of this one. So I think we don't have to be worried too much (though of course we should take benchmarks if possible).
Compared to template functions/classes, type erasure, as well as inheritance, will perform worse. But I think it must be more problematic that we have to write lots of template<> everywhere and will get a humongous binary as a result of compilation.
I suggested a fix for one unnecessary copy, but actually they don't affect the performance usually. This is because your concerning unnecessary copies are occurred in just std::shared_ptr<T>, not Titself. So they just unnecessarily take locks and increment refcounts at worst. Moreover, those copies would happen in initialization, not during a fuzzing campaign right?
- Naming
These classes are placed under include/fuzzuf/executor/traits. I used the traits directory name because it is designed to be aware of Rust's trait objects, but it may cause misunderstanding because trait in C++ is a different concept from Rust's trait. If you have a better alternative directory name, please comment.
I think naming "trait" is somewhat relevant. As an alternative wording, "concept" can be used, but as you may have thought, concept is usually a more general idea describing the conditions for SFINAE or sometimes other things in BOOST and thus ambiguous. I think trait is nearer to what we mean by this design.
The maintainer of this repository will set up a reviewer for each PR.
PR reviewers should review this PR in terms of the checklist below before moving on to a detailed code review. Please comment on their initial response by filling in the checklist below.
Optional Entries
- The reviewer assigned more reviewers if needed.
- The reviewer noted that it is necessary to break out some of the changes in this PR into other PRs if needed.
- The reviewer noted that the initial response is insufficient if needed.
Mandatory Entries
- The title of this PR summarizes the changes made by this PR properly.
- The target branch of this PR is as intended.
- The reviewer understands the issues in this PR.
- The reviewer plans to review with an appropriate workload based on the importance of this PR.
retrage
force-pushed
the
feature/2022-02-28-executor-trait-objects
branch
2 times, most recently
from
4991ef2
to
1bff8b7
Compare
Signed-off-by: Akira Moroo <akiram@ricsec.co.jp>
This commit adds `AFLExecutorInterface` class to represent required functions for an executor to support AFL-compatible algorithms. This class accepts `std::shared_ptr<T>` of any class `T` as long as it satisfies the function requirements. This behavior mimics Rust's trait objects [1] using type erasure. [1] https://doc.rust-lang.org/book/ch17-02-trait-objects.html Signed-off-by: Akira Moroo <akiram@ricsec.co.jp>
This commit adds `LibFuzzerExecutorInterface` class to represent required functions for an executor to support libFuzzer-compatible algorithms. This class accepts `std::shared_ptr<T>` of any class `T` as long as it satisfies the function requirements. This behavior mimics Rust's trait objects [1] using type erasure. [1] https://doc.rust-lang.org/book/ch17-02-trait-objects.html Signed-off-by: Akira Moroo <akiram@ricsec.co.jp>
This commit changes to use `AFLExecutorInterface` instead of using `NativeLinuxExecutor` directly so that the algorithms are independent from actual executor type. Note that this commit does not affect algorithm behaviors and the executor is still fixed to `NativeLinuxExecutor`. Signed-off-by: Akira Moroo <akiram@ricsec.co.jp>
This commti changes to use `LibFuzzerExecutorInterface` instead of using `NativeLinuxExecutor` directly so that the algorithms are independent form actual executor type. Note that this commit does not affect algorithm behaviors and the executor is still fixed to `NativeLinuxExecutor`. Signed-off-by: Akira Moroo <akiram@ricsec.co.jp>
retrage
force-pushed
the
feature/2022-02-28-executor-trait-objects
branch
from
7726766
to
99f2876
Compare
Type of PR
Related Issue
#30
Importance of PR
PR Overview
For the background, see #30.
This PR proposes introducing "trait object" in Rust's term [1] to represent what methods must be implemented for an executor to support the algorithm. By this change, the algorithms can use executors without knowing the actual executor type since the classes abstract the executors. I implemented the introduced classes (
DynAFLCapableandDynLibFuzzerCapable) using type erasure as suggested by @potetisensei. Note that this PR adds bare-type erasure classes only, so it does not affect the behaviors of the algorithm and CLI. I'm going to create another PR that introduces CLI changes for selecting executors at runtime to show the advantage of this change.[1] https://doc.rust-lang.org/book/ch17-02-trait-objects.html
Concerns (Optional)
I have several concerns about this change:
1. Type Erasure Implementation
I implemented the classes with type erasure on my own. This is usually discouraged because bugs in my own implementation of type erasure are difficult to debug and can lead to "shooting myself in the foot". However, I found that the implementation using boost/type_erasure did not meet our needs in this case, as the amount of code using boost was not much different from my own implementation, and error messages tended to be more difficult. For this reason, I decided to implement type erasure myself. There is room for discussion on this policy.
2. Performance
Because type erasure hides the actual type, runtime performance may suffer depending on compile-time optimizations. Although I have not yet measured and compared performance, I did not experience any performance degradation when running the unit test.
Please review the type erasure classes to ensure that no unnecessary copies are made. They take
std::shared_ptr<TExecutor>as a constructor argument rather than executor itself. I believe that the executor is not created unnecessarily, but I am not sure if this implementation is correct.3. Naming
These classes are placed under
include/fuzzuf/executor/traits. I used thetraitsdirectory name because it is designed to be aware of Rust's trait objects, but it may cause misunderstanding because trait in C++ is a different concept from Rust's trait. If you have a better alternative directory name, please comment.Optional Entries
Mandatory Entries
Optional Entries
Mandatory Entries
Optional Entries
Mandatory Entries