Page MenuHomePhabricator

implemented proto to llvm
ClosedPublic

Authored by emmettneyman on Jun 12 2018, 4:15 PM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • removed unneeded file
  • fixed proto_to_cxx.cpp

Where is the fuzz target?

tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp
34

I'd suggest wrapper functions that return unused variable names, so your code below won't need to juggle these globals.

44

The way these globals are being used seems confusing and error-prone.

Maybe what you want is to replace the operator<< overloads with functions that emit the LLVM and then return the variable name that the result is stored in. Then the parent node can use the returned variable name without juggling globals.

49

getelementptr should make this much simpler.

81

When will this last return be executed?

133

Probably not a big deal, but maybe load the rvalue first? I'd expect the frontend to do that normally to simplify register allocation.

133

Also, I think it is easier to read if you put each statement on its own line.

145

Space before %b.

154

Can we put %z = ... on the next line for consistency?

157

Can we put the first newline on previous statement for consistency?

  • refactored loop_proto_to_llvm.cpp

Where is the fuzz target?

I wanted to implement the proto_to_llvm converter before the fuzz target.

  • removed typo in emitted llvm insn

I wanted to implement the proto_to_llvm converter before the fuzz target.

The fuzz target should make testing your converter way easier. I'd recommend adding it to this patch so that you're less likely to need a bug-fixing patch later.

tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp
33

You can hide this as a static int inside get_var.

47

What's the point of storing then loading the value? Can you just return val?

50

Returning a pair can be confusing (which element is which?). I'd suggest passing os to these functions, writing the instructions to os, and then returning just the result variable.

121

If all these cases are the same, we can simplify the code to

case BinaryOp::EQ:
case BinaryOp::NE:
...
op = "add";
break;
130

If AssignmentStatementToString has no extra return value, I think its more concise to just keep the operator<< overload. (Same for below functions)

Implemented HandleLLVM fuzz target

-HandleLLVM function compiles LLVM IR
-ExampleClangLLVMProtoFuzzer wraps the fuzz target
-Next step is to compile at different opt levels and run the executables
  • removed unnecessary includes and linked libs
morehouse requested changes to this revision.Jun 18 2018, 5:54 PM
morehouse added inline comments.
tools/clang-fuzzer/CMakeLists.txt
72

llvm

82–83

Maybe remove clangHandleCXX here, so you can use this variable for linking clang-llvm-proto-fuzzer.

tools/clang-fuzzer/handle-llvm/CMakeLists.txt
6

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?

tools/clang-fuzzer/handle-llvm/\
31 ↗(On Diff #151801)

Please sort includes alphabetically, with handle_llvm.h separate at the top.

42 ↗(On Diff #151801)

Nit: avoid empty lines at beginning or end of a {} block (here and below)

62 ↗(On Diff #151801)

Does this initialization need to happen every time the fuzzer generates a new input, or can we call this from LLVMFuzzerInitialize() instead?

71 ↗(On Diff #151801)
  1. 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.
  2. I don't think you need to construct StringRefs or the MemoryBufferRef here. Instead you can probably do parseIR(MemoryBufferRef(S, "IR"), Err, ...) below.
79 ↗(On Diff #151801)

I'd move the unique_ptr definition to the same line as parseIR.

81 ↗(On Diff #151801)

What's the point of wrapping sys::getDefaultTargetTriple() if we always unwrap TheTriple below?

84 ↗(On Diff #151801)

What does UpgradeDebugInfo=false do here? The documentation warns about setting this bool to false.

84 ↗(On Diff #151801)

What if parseIR returns nullptr?

88 ↗(On Diff #151801)

What if lookupTarget returns nullptr?

90 ↗(On Diff #151801)

Please separate to 2 statements.

98 ↗(On Diff #151801)

Fix indentation here.

101 ↗(On Diff #151801)

Maybe add a default case here with an error message?

104 ↗(On Diff #151801)

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.

113 ↗(On Diff #151801)

Any reason not to use the newer PassManager?

127 ↗(On Diff #151801)

What's the reason for this cast? MachineModuleInfo() accepts TargetMachine * as an argument.

130 ↗(On Diff #151801)

Eventually you will want to save the outputs to do A/B runs. This is fine for now though.

tools/clang-fuzzer/handle-llvm/handle_llvm.cpp
15

Apparently you created a file called \ with the same text. Please remove that file, and apply my comments there to this file instead.

tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp
58

Simpler to do os << ptr_var << " = getelementptr" ... (here and below)

72

Is it still possible for the protobuf to not have a constant, a binOp, or a varRef?

114

Nit: Maybe var_ref or lvalue would be more descriptive names.

115

Might make for slightly faster IR if you do the rvalue first. Would at least simplify register allocation.

This revision now requires changes to proceed.Jun 18 2018, 5:54 PM
emmettneyman added inline comments.Jun 19 2018, 10:51 AM
tools/clang-fuzzer/handle-llvm/CMakeLists.txt
6

It builds without any errors both with all the libraries from handle-cxx/ linked in and without any of the libraries linked in (as I have here). Which is preferred?

morehouse added inline comments.Jun 19 2018, 11:02 AM
tools/clang-fuzzer/handle-llvm/CMakeLists.txt
6

I prefer what you have here, if shared libraries are OK.

emmettneyman added inline comments.Jun 19 2018, 11:43 AM
tools/clang-fuzzer/handle-llvm/\
62 ↗(On Diff #151801)

I was following the pattern from handle_cxx.cpp in which the initialization calls were made every time a new input was generated. However looking at the documentation and at llvm-isel-fuzzer.cpp, it seems like putting it in LLVMFuzzerInitialize() makes more sense. I will make this change in the next commit.

emmettneyman added inline comments.Jun 19 2018, 2:44 PM
tools/clang-fuzzer/handle-llvm/\
113 ↗(On Diff #151801)

Clang (llc) and the llvm-isel-fuzzer both still use the legacy PassManager. The newer PassManager has a new interface that doesn't use Modules.

emmettneyman added inline comments.Jun 19 2018, 3:13 PM
tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp
72

Right now, since the protobufs use oneof, it is possible for the protobuf to not have a constant binOp, or varRef. For now, I'll generate "dummy" code for this case.

Then, I'll submit a new patch that eliminates the use of oneof for both cxx_proto.proto and cxx_loop_proto so that "dummy" code isn't necessary.

  • made changes to handle_llvm.cpp in response to reviewer comments

If you haven't already, please apply for commit access: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access.

That way you can land this after it's accepted.

tools/clang-fuzzer/handle-llvm/handle_llvm.cpp
24

This should come before llvm/IR/Module.h.

36

Can make this static to avoid future namespace collisions.

49

Maybe exit() as well on error? (here and below)

Exiting will cause the fuzz target to fail, so you can debug more easily. Otherwise, these error messages could go unnoticed.

59

It's easier to follow if you move this closer to where Context is first used.

67

You can actually omit the last argument here since you're using the default.

82

Easier to follow if you move these two lines closer to where CPUStr and FeaturesStr are used.

106

I believe you can avoid creating the MMI here since it looks like addPassesToEmitFile will do it for you.

  • minor changes to improve readability and style
morehouse accepted this revision.Jun 19 2018, 5:38 PM
morehouse added inline comments.
tools/clang-fuzzer/handle-llvm/handle_llvm.cpp
51

exit(1) will indicate an abnormal exit. (here and below)

65

This comment is unnecessary. Clearly a new Context is being created on the next line.

This revision is now accepted and ready to land.Jun 19 2018, 5:38 PM
  • removed unnecessary comment and fixed exit statement

Looks like exit(0) is still there.

  • actually fixed error statement

Looks like exit(0) is still there.

oops, forgot to :w

emmettneyman closed this revision.Jun 20 2018, 8:54 AM
sbc100 added a subscriber: sbc100.Jun 22 2018, 12:16 PM

I think this broke the BUILD_SHARED_LIBS=1 build: https://reviews.llvm.org/D48503