[PATCH] D36274: Introduce FuzzMutate library

Vedant Kumar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Aug 11 17:08:00 PDT 2017


vsk added a comment.

Very cool. I've just done an initial pass and will need to take a closer look. The main piece of feedback I've got is that adding more docs would really clarify the code. I've pointed out some specific areas which could use doxygen comments. A small writeup in docs/Fuzzing would also be great -- maybe that makes more sense as a separate patch?



================
Comment at: include/llvm/FuzzMutate/IRMutator.h:36
+
+  virtual uint64_t getWeight(size_t CurrentSize, size_t MaxSize,
+                             uint64_t CurrentWeight) = 0;
----------------
This method could use a doxygen comment. To a casual reader, it may not be evident why a mutation strategy has a weight. This would help clarify other mutate* methods below.


================
Comment at: include/llvm/FuzzMutate/IRMutator.h:78
+
+  using IRMutationStrategy::mutate;
+  void mutate(Function &F, RandomIRBuilder &IB) override;
----------------
Out of curiosity, why is this using decl needed? Is 'mutate' somehow ambiguous here?


================
Comment at: include/llvm/FuzzMutate/OpDescriptor.h:31
+void makeConstantsWithType(Type *T, std::vector<Constant *> &Cs);
+std::vector<Constant *> makeConstantsWithType(Type *T);
+
----------------
This group could use a doxygen comment. It would help to clarify whether these are pseudo-random constants.


================
Comment at: include/llvm/FuzzMutate/OpDescriptor.h:33
+
+/// A predicate to answer whether some value is suitable for the next source of
+/// a particular operation.
----------------
IIUC, this could be more clear as: "to answer whether some value generator is suitable as a source for a particular operation".


================
Comment at: include/llvm/FuzzMutate/OpDescriptor.h:37
+public:
+  using PredT = std::function<bool(ArrayRef<Value *> Cur, const Value *New)>;
+  using MakeT = std::function<std::vector<Constant *>(
----------------
Please add a comment explaining the intended usage of the predicate (e.g you could clarify why the current value is list-like, while the new value isn't).


================
Comment at: include/llvm/FuzzMutate/OpDescriptor.h:39
+  using MakeT = std::function<std::vector<Constant *>(
+      ArrayRef<Value *> Cur, ArrayRef<Type *> BaseTypes)>;
+
----------------
Please add a comment explaining what the inputs to this value generator are supposed to be (e.g you could clarify the constraints on the returned values -- need they be random?).


================
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) {
----------------
Why have an unused NoneType parameter?


================
Comment at: include/llvm/FuzzMutate/OpDescriptor.h:54
+        Constant *V = UndefValue::get(T);
+        if (Pred(Cur, V))
+          makeConstantsWithType(T, Result);
----------------
Nit: It may enhance readability to capture this and use matches() directly. Feel free to ignore this if you don't feel that way.


================
Comment at: include/llvm/FuzzMutate/OpDescriptor.h:124
+    transform(Ts, std::back_inserter(Result),
+              [](Type *T) { return UndefValue::get(PointerType::get(T, 0)); });
+    return Result;
----------------
A for loop would be clearer here. You can also use PointerType::getUnqual, which implies addrspace(0).


================
Comment at: include/llvm/FuzzMutate/RandomIRBuilder.h:36
+  Value *findOrCreateSource(BasicBlock &BB, ArrayRef<Instruction *> Insts,
+                            ArrayRef<Value *> Srcs, fuzzerop::SourcePred Pred);
+  Value *newSource(BasicBlock &BB, ArrayRef<Instruction *> Insts,
----------------
Please add a comment explaining what a source is and where this searches for sources. At first glance, it's not clear if this searches the BB's instructions, the Insts array, or the Srcs array. In general here it would help to clarify how the Insts and Srcs arrays are used.


================
Comment at: lib/FuzzMutate/Operations.cpp:43
+  Ops.push_back(cmpOpDescriptor(1, Instruction::ICmp, CmpInst::ICMP_SLT));
+  Ops.push_back(cmpOpDescriptor(1, Instruction::ICmp, CmpInst::ICMP_SLE));
+}
----------------
Have you considered using the FIRST_ICMP_PREDICATE and LAST_ICMP_PREDICATE macros from InstrTypes.h to condense this?


================
Comment at: lib/FuzzMutate/Operations.cpp:68
+  Ops.push_back(cmpOpDescriptor(1, Instruction::FCmp, CmpInst::FCMP_UNE));
+  Ops.push_back(cmpOpDescriptor(1, Instruction::FCmp, CmpInst::FCMP_TRUE));
+}
----------------
Ditto for the FIRST_FCMP_PREDICATE and LAST_FCMP_PREDICATE macros?


================
Comment at: lib/FuzzMutate/Operations.cpp:110
+  case Instruction::Or:
+  case Instruction::Xor:
+    return {Weight, {anyIntType(), matchFirstType()}, buildOp};
----------------
It looks like this list is duplicated in describeFuzzerIntOpts. Would it make sense to share it with the HANDLE_X pattern?


================
Comment at: lib/FuzzMutate/Operations.cpp:116
+  case Instruction::FDiv:
+  case Instruction::FRem:
+    return {Weight, {anyFloatType(), matchFirstType()}, buildOp};
----------------
Ditto.


Repository:
  rL LLVM

https://reviews.llvm.org/D36274





More information about the llvm-commits mailing list