[PATCH] D36274: Introduce FuzzMutate library
Vedant Kumar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 15 11:27:57 PDT 2017
vsk added inline comments.
================
Comment at: include/llvm/FuzzMutate/RandomIRBuilder.h:53
+ /// Create a user for \c V in \c BB.
+ void newSink(BasicBlock &BB, ArrayRef<Instruction *> Insts, Value *V);
+ Value *findPointer(BasicBlock &BB, ArrayRef<Instruction *> Insts,
----------------
Since connectToSink() can create a store, its comment should say it does more than just finding a sink. There may also be an opportunity here to fold newSink() into connectToSink().
================
Comment at: lib/FuzzMutate/IRMutator.cpp:56
+ std::vector<Type *> Types;
+ for (auto Getter : AllowedTypes)
+ Types.push_back(Getter(M.getContext()));
----------------
Use const auto & here? Copying a std::function may not be cheap.
================
Comment at: lib/FuzzMutate/IRMutator.cpp:79
+ IRMutationStrategy::mutate(F, IB);
+ eliminateDeadCode(F);
+}
----------------
Wdyt of looping here until we're sure that the injected IR isn't DCE'd? If it's expensive to evaluate a mutation, we might be able to run faster if we're sure each mutation has done work.
================
Comment at: lib/FuzzMutate/IRMutator.cpp:89
+ describeFuzzerAggregateOps(Ops);
+ describeFuzzerVectorOps(Ops);
+ return Ops;
----------------
Is it worth adding test coverage for this?
================
Comment at: lib/FuzzMutate/IRMutator.cpp:119
+
+ // Choose an operation that's constrained be valid for the type of the
+ // source, collect any other sources it needs, and then build it.
----------------
nit, 'to be valid'
================
Comment at: lib/FuzzMutate/IRMutator.cpp:122
+ fuzzerop::OpDescriptor OpDesc = chooseOperation(Srcs[0], IB);
+ for (auto Pred : makeArrayRef(OpDesc.SourcePreds).slice(1))
+ Srcs.push_back(IB.findOrCreateSource(BB, InstsBefore, Srcs, Pred));
----------------
Use const auto & here?
================
Comment at: lib/FuzzMutate/IRMutator.cpp:180
+ if (!RS)
+ RS.sample(IB.newSource(*BB, InstsBefore, {}, Pred), /*Weight=*/1);
+
----------------
Wdyt of always considering adding a new source? It'd be sampled along with the other options, with the same weight. I expect that this would increase coverage without causing chaos.
================
Comment at: lib/FuzzMutate/RandomIRBuilder.cpp:63
+ IP = ++I->getIterator();
+ return new LoadInst(Ptr, "L", &*IP);
+}
----------------
Out of curiosity, are all sources with pointer type LoadInsts?
https://reviews.llvm.org/D36274
More information about the llvm-commits
mailing list