[PATCH] D48106: implemented proto to llvm

[Matt Morehouse via Phabricator via cfe-commits]

morehouse requested changes to this revision.
morehouse added inline comments.
This revision now requires changes to proceed.


================
Comment at: tools/clang-fuzzer/CMakeLists.txt:72
 
+  # Build the lllvm protobuf fuzzer
+  add_clang_executable(clang-llvm-proto-fuzzer
----------------
llvm


================
Comment at: tools/clang-fuzzer/CMakeLists.txt:83
     clangFuzzerInitialize
     clangHandleCXX
     )
----------------
Maybe remove `clangHandleCXX` here, so you can use this variable for linking `clang-llvm-proto-fuzzer`.


================
Comment at: tools/clang-fuzzer/handle-llvm/CMakeLists.txt:5
+  handle_llvm.cpp
+  )
----------------
There's fewer libraries linked here than in `handle-cxx/` (not saying this is wrong, but it could be).  Do you get link errors if you build `clang-llvm-proto-fuzzer` with shared libraries?


================
Comment at: tools/clang-fuzzer/handle-llvm/\:31
+#include "llvm/Analysis/TargetLibraryInfo.h"
+#include "llvm/Support/raw_ostream.h"
+
----------------
Please sort includes alphabetically, with handle_llvm.h separate at the top.


================
Comment at: tools/clang-fuzzer/handle-llvm/\:42
+void InitializeEverything() {
+
+  InitializeAllTargets();
----------------
Nit:  avoid empty lines at beginning or end of a `{}` block (here and below)


================
Comment at: tools/clang-fuzzer/handle-llvm/\:62
+  initializeScavengerTestPass(*Registry);
+
+}
----------------
Does this initialization need to happen every time the fuzzer generates a new input, or can we call this from `LLVMFuzzerInitialize()` instead?


================
Comment at: tools/clang-fuzzer/handle-llvm/\:71
+  StringRef *ID = new StringRef("IR");
+  MemoryBufferRef *ir = new MemoryBufferRef(*IRString, *ID);
+
----------------
# Avoid using `new` when you could create the object on the stack instead.  This will prevent you from introducing memory leaks if you forget to call `delete` later.
# I don't think you need to construct StringRefs or the MemoryBufferRef here.  Instead you can probably do `parseIR(MemoryBufferRef(S, "IR"), Err, ...)` below.


================
Comment at: tools/clang-fuzzer/handle-llvm/\:79
+  SMDiagnostic Err;
+  std::unique_ptr<Module> M;
+  Triple TheTriple;
----------------
I'd move the `unique_ptr` definition to the same line as `parseIR`.


================
Comment at: tools/clang-fuzzer/handle-llvm/\:81
+  Triple TheTriple;
+  TheTriple.setTriple(sys::getDefaultTargetTriple());
+
----------------
What's the point of wrapping `sys::getDefaultTargetTriple()` if we always unwrap `TheTriple` below?


================
Comment at: tools/clang-fuzzer/handle-llvm/\:84
+  // Set the Module to include the the IR code to be compiled
+  M = parseIR(*ir, Err, Context, false);
+
----------------
What does `UpgradeDebugInfo=false` do here?  The documentation warns about setting this bool to false.


================
Comment at: tools/clang-fuzzer/handle-llvm/\:84
+  // Set the Module to include the the IR code to be compiled
+  M = parseIR(*ir, Err, Context, false);
+
----------------
morehouse wrote:
> What does `UpgradeDebugInfo=false` do here?  The documentation warns about setting this bool to false.
What if `parseIR` returns nullptr?


================
Comment at: tools/clang-fuzzer/handle-llvm/\:88
+  std::string Error;
+  const Target *TheTarget = TargetRegistry::lookupTarget(TheTriple.getTriple(),
+                                                         Error);
----------------
What if `lookupTarget` returns nullptr?


================
Comment at: tools/clang-fuzzer/handle-llvm/\:90
+                                                         Error);
+  std::string CPUStr = getCPUStr(), FeaturesStr = getFeaturesStr();
+
----------------
Please separate to 2 statements.


================
Comment at: tools/clang-fuzzer/handle-llvm/\:98
+      switch(A[2]) {  
+	case '0': OLvl = CodeGenOpt::None; break;
+        case '1': OLvl = CodeGenOpt::Less; break;
----------------
Fix indentation here.


================
Comment at: tools/clang-fuzzer/handle-llvm/\:101
+        case '2': OLvl = CodeGenOpt::Default; break;
+        case '3': OLvl = CodeGenOpt::Aggressive; break;
+      }
----------------
Maybe add a default case here with an error message?


================
Comment at: tools/clang-fuzzer/handle-llvm/\:104
+    }
+  }
+
----------------
It might make sense to move command line arg parsing to a helper function, and then call that function closer to the top.  That way if there's a bad argument we can quit before doing all the IR parsing.


================
Comment at: tools/clang-fuzzer/handle-llvm/\:113
+
+  legacy::PassManager PM;
+  TargetLibraryInfoImpl TLII(Triple(M->getTargetTriple()));
----------------
Any reason not to use the newer PassManager?


================
Comment at: tools/clang-fuzzer/handle-llvm/\:127
+
+  LLVMTargetMachine &LLVMTM = static_cast<LLVMTargetMachine&>(*Target);
+  MachineModuleInfo *MMI = new MachineModuleInfo(&LLVMTM);
----------------
What's the reason for this cast?  `MachineModuleInfo()` accepts `TargetMachine *` as an argument.


================
Comment at: tools/clang-fuzzer/handle-llvm/\:130
+  Target->addPassesToEmitFile(PM, OS, nullptr, TargetMachine::CGFT_ObjectFile,
+                              false, MMI);
+
----------------
Eventually you will want to save the outputs to do A/B runs.  This is fine for now though.


================
Comment at: tools/clang-fuzzer/handle-llvm/handle_llvm.cpp:14
+//===----------------------------------------------------------------------===//
+
+#include "llvm/IR/LLVMContext.h"
----------------
Apparently you created a file called `\` with the same text.  Please remove that file, and apply my comments there to this file instead.


================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:57
+  std::string insn = ptr_var + " = getelementptr i32, i32* " + arr + ", i64 %ct\n";
+  os << insn;
+  return ptr_var;
----------------
Simpler to do `os << ptr_var << " = getelementptr" ...`  (here and below)


================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:71
+    return val_var;
+  }
+}
----------------
Is it still possible for the protobuf to not have a constant, a binOp, or a varRef?


================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:113
+std::ostream &operator<<(std::ostream &os, const AssignmentStatement &x) {
+  std::string ref = VarRefToString(os, x.varref());
+  std::string rv = RvalueToString(os, x.rvalue());
----------------
Nit:  Maybe `var_ref` or `lvalue` would be more descriptive names.


================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:114
+  std::string ref = VarRefToString(os, x.varref());
+  std::string rv = RvalueToString(os, x.rvalue());
+  std::string insns = "store i32 " + rv + ", i32* " + ref + "\n";
----------------
Might make for slightly faster IR if you do the rvalue first.  Would at least simplify register allocation.


Repository:
  rC Clang

https://reviews.llvm.org/D48106