[PATCH] D49526: Updated llvm-proto-fuzzer to execute the compiled code

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: clang/tools/clang-fuzzer/fuzzer-initialize/fuzzer_initialize.cpp:44
+  
+  PassRegistry &Registry = *llvm::PassRegistry::getPassRegistry();
+  initializeCore(Registry);
----------------
Unnecessary `llvm::`


================
Comment at: clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp:89
+  Builder.Inliner = createFunctionInliningPass(OptLevel, SizeLevel, false);
+  Builder.LoopVectorize = true;
+  Builder.populateFunctionPassManager(FPM);
----------------
If we do this, do we need to explicitly add the vectorizer pass below?


================
Comment at: clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp:95
+// Mimics the opt tool to run an optimization pass over the provided IR
+std::string OptLLVM(const std::string &IR, CodeGenOpt::Level &OLvl) {
+  // Create a module that will run the optimization passes
----------------
`OLvl` is never used.


================
Comment at: clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp:105
+  std::string FeaturesStr = getFeaturesStr();
+  setFunctionAttributes(CPUStr, FeaturesStr, *M);
+  
----------------
Let's simplify to `setFunctionAttributes(getCPUStr(), getFeaturesStr(), *M)`.


================
Comment at: clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp:110
+  TargetLibraryInfoImpl TLII(ModuleTriple);
+  Passes.add(new TargetLibraryInfoWrapperPass(TLII));
+  Passes.add(createTargetTransformInfoWrapperPass(TargetIRAnalysis()));
----------------
Can simplify to `Passes.add(new TargetLibraryInfoWrapperPass(M->getTargetTriple))`.


================
Comment at: clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp:115
+      make_unique<legacy::FunctionPassManager>(M.get());
+  FPasses->add(createTargetTransformInfoWrapperPass(TargetIRAnalysis()));
+
----------------
Why do we add a `TargetTransformInfoWrapperPass` to both `Passes` and `FPasses`?


================
Comment at: clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp:115
+      make_unique<legacy::FunctionPassManager>(M.get());
+  FPasses->add(createTargetTransformInfoWrapperPass(TargetIRAnalysis()));
+
----------------
morehouse wrote:
> Why do we add a `TargetTransformInfoWrapperPass` to both `Passes` and `FPasses`?
Do we need both `Passes` and `FPasses`?


================
Comment at: clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp:121
+    ErrorAndExit("Cannot create IR pass");
+  Passes.add(P);
+  
----------------
Let's simplify to `Passes.add(createLoopVectorizePass())`.


================
Comment at: clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp:127
+  FPasses->doFinalization();
+  Passes.add(createVerifierPass());
+  
----------------
Why do we keep adding passes in different places?


================
Comment at: clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp:144
+                                           Context);
+  Module *M = Owner.get();
+  if (!M)
----------------
Why not just rename `Owner` to `M` and remove this line?


================
Comment at: clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp:158
+  builder.setMCJITMemoryManager(
+      std::unique_ptr<RTDyldMemoryManager>(RTDyldMM));
+  builder.setOptLevel(OLvl);
----------------
Please simplify to `builder.setMCJITMemoryManager(std::make_unique<RTDyldMemoryManager>())`.


================
Comment at: clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp:159
+      std::unique_ptr<RTDyldMemoryManager>(RTDyldMM));
+  builder.setOptLevel(OLvl);
+  builder.setTargetOptions(InitTargetOptionsFromCodeGenFlags());
----------------
If the JIT does optimization already, do we need to call `OptLLVM` at all?  Will the vectorizer kick in without `OptLLVM`?


================
Comment at: clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp:168
+  if (!EntryFunc)
+    ErrorAndExit("Function not found in module");
 
----------------
Move this closer to where it's used.


================
Comment at: clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp:171
+  EE->finalizeObject();
+  EE->runStaticConstructorsDestructors(false);
+
----------------
Do we need to call this again to run destructors after we execute `f()` ?


================
Comment at: clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp:181
   
-  raw_null_ostream OS;
-  Target->addPassesToEmitFile(PM, OS, nullptr, TargetMachine::CGFT_ObjectFile,
-                              false);
-  PM.run(*M);
+  (*f)(a, b, c, 1);
+}
----------------
I think `f(a, b, c, 1)` will also work.


Repository:
  rC Clang

https://reviews.llvm.org/D49526