[PATCH] D36274: Introduce FuzzMutate library

[Justin Bogner via Phabricator via llvm-commits]

bogner added a comment.

Adding some docs in docs/Fuzzer sounds like a good idea. I'll see about doing that soon but I think it makes sense for that to be a separate patch.



================
Comment at: include/llvm/FuzzMutate/IRMutator.h:78
+
+  using IRMutationStrategy::mutate;
+  void mutate(Function &F, RandomIRBuilder &IB) override;
----------------
vsk wrote:
> Out of curiosity, why is this using decl needed? Is 'mutate' somehow ambiguous here?
This pulls in the full overload set, so that the base class's mutate(Module) will choose a function and call mutate(Function) here.


================
Comment at: include/llvm/FuzzMutate/OpDescriptor.h:48
+  SourcePred(PredT Pred, MakeT Make) : Pred(Pred), Make(Make) {}
+  SourcePred(PredT Pred, NoneType) : Pred(Pred) {
+    Make = [Pred](ArrayRef<Value *> Cur, ArrayRef<Type *> BaseTypes) {
----------------
vsk wrote:
> Why have an unused NoneType parameter?
I found it clearer to force the user to explicitly opt in to using the default generator - SourcePred(P, None) rather than SourcePred(P). If you think that's overkill I could drop this.


================
Comment at: include/llvm/FuzzMutate/OpDescriptor.h:54
+        Constant *V = UndefValue::get(T);
+        if (Pred(Cur, V))
+          makeConstantsWithType(T, Result);
----------------
vsk wrote:
> Nit: It may enhance readability to capture this and use matches() directly. Feel free to ignore this if you don't feel that way.
It's kind of sketchy to use member functions in a constructor. I'd rather not.


================
Comment at: lib/FuzzMutate/Operations.cpp:110
+  case Instruction::Or:
+  case Instruction::Xor:
+    return {Weight, {anyIntType(), matchFirstType()}, buildOp};
----------------
vsk wrote:
> It looks like this list is duplicated in describeFuzzerIntOpts. Would it make sense to share it with the HANDLE_X pattern?
These aren't in their own list anywhere today (they're mixed with the float ops in Instructions.def) so I think that would entail creating a new .def file and including that in a couple of places here. I'm not convinced the decreased readability of having to go look up what's in that list is worth the couple of lines of savings here.


https://reviews.llvm.org/D36274