[PATCH] D40393: [FuzzMutate] Don't crash when we can't remove instruction from empty function

[Igor Laevsky via Phabricator via llvm-commits]

igor-laevsky added inline comments.


================
Comment at: lib/FuzzMutate/IRMutator.cpp:150-151
       RS.sample(&Inst, /*Weight=*/1);
-  assert(!RS.isEmpty() && "No instructions to delete");
+  if (RS.isEmpty())
+    return;
+
----------------
bogner wrote:
> This is reasonable and should fix the problem, but it would be kind of nice if we could also make it so the deleter strategy was never chosen in this case. That's pretty tricky with the current getWeight API (it doesn't really have enough information), but we should think about it for the future.
I guess it boils down to the question of wether or not we want to guarantee "forward progress" of the mutator (i,e guarantee that at least some transformation is always performed). Initially I implemented more involved version of this change - if randomly chosen function appeared to be empty we would have picked the first non empty one and removed from it. Then if there would be no such function we would assert. However in the end I realised that forward progress guarantee is not that important considering the amount of tests OSSFuzz runs.


================
Comment at: unittests/FuzzMutate/StrategiesTest.cpp:99-101
+  // We need to choose 'func1' in order for the crash to appear.
+  // Loop 10 times and assume we are lucky.
+  for (int i = 0; i < 10; ++i) {
----------------
bogner wrote:
> Thanks for adding the test! I'm not a huge fan of the probabilistic nature of it, but I can't think of a better way to do that without making either the sampler or the mutators quite a bit more complicated.
If it makes things slightly better - since seed is fixed this is deterministic with the given libc implementation. Hopefully there shouldn't be too much different implementations.


Repository:
  rL LLVM

https://reviews.llvm.org/D40393