[PATCH] D20741: [LibFuzzer] Reimplement how the optional user functions are called.

[Kostya Serebryany via llvm-commits]

kcc added inline comments.

================
Comment at: lib/Fuzzer/FuzzerDriver.cpp:422
@@ +421,3 @@
+  assert(argc && argv && "Argument pointers cannot be nullptr");
+  fuzzer::ExternalFunctions EF;
+  EF.Init();
----------------
delcypher wrote:
> kcc wrote:
> > move all of this into the other FuzzerDriver. 
> > This one is an atavism of my poor design choice in the past, let's not resurrect it. 
> A problem with your suggestion is it makes calling the `LLVMFuzzerInitialize(int *argc, char ***argv)` function difficult because we don't have the `argv` anymore (inside `static int FuzzerDriver(const std::vector<std::string> &Args, UserCallback Callback, fuzzer::ExternalFunctions EF)` ). We can emulate `argc` using `Args.size()` but `argv` is more of pain because we have `std::vector<std::string>` not `std::vector<const char*>`.
> 
> Also if the intention of `LLVMFuzzerInitialize(int *argc, char ***argv)` is to allow the command line arguments to be changed then what you're proposing doesn't work because the user needs to make the modifications before we create the `std::vector<std::string>` not after we create it.
Can you simply merge these two functions leaving just this one? 
FuzzerDriver(int *argc, char ***argv, UserCallback Callback);
using std::vector there is unnecessary. 

================
Comment at: lib/Fuzzer/FuzzerExtFunctions.h:26
@@ +25,3 @@
+  int (*LLVMFuzzerInitialize)(int *argc, char ***argv) = nullptr;
+  size_t (*LLVMFuzzerCustomMutator)(uint8_t *Data, size_t Size, size_t MaxSize,
+                                    unsigned int Seed) = nullptr;
----------------
We still have a duplication between here and FuzzerExtFunction.def
Can you use a bit more macro tricks to eliminate FuzzerExtFunction.def? 

================
Comment at: lib/Fuzzer/FuzzerInternal.h:475
@@ -471,1 +474,3 @@
+  // Interface to functions that may or may not be available.
+  ExternalFunctions EF;
 };
----------------
delcypher wrote:
> kcc wrote:
> > I did not suggest to pass it by value. 
> > Can't it be created inside Fuzzer::Fuzzer with a default CTOR? 
> > 
> We can create an instance of `ExternalFunctions` inside the `Fuzzer` class in the default constructor but then we have the problem of also needing the call `LLVMFuzzerInitialize` which requires an instance of `ExternalFunctions` and that has to be done before we create the `Fuzzer` class.
> 
> So either we need to make two instances of `ExternalFunctions` and initialize both of them (one inside the `Fuzzer` class and one outside) OR a single instance of `ExternalFunctions` which we initialize and pass to the `Fuzzer` class.
> 
> I can do either. Decide which you'd prefer and I will implement that.
I'd prefer to have two instances of EF. That's a tiny bit slower at startup but leaves the code simpler. 


http://reviews.llvm.org/D20741