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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
