[PATCH] D36324: Integrate Kostya's clang-proto-fuzzer with LLVM.

[Matt Morehouse via Phabricator via cfe-commits]

morehouse added inline comments.


================
Comment at: clang/cmake/modules/ProtobufMutator.cmake:13
+    -DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}
+    -DCMAKE_BUILD_TYPE=Debug
+  BUILD_COMMAND ${CMAKE_MAKE_PROGRAM}
----------------
vitalybuka wrote:
> Why this is debug?
> 
I was just using what the libprotobuf-mutator readme suggested.  But I can change it to use CMAKE_BUILD_TYPE instead.


================
Comment at: clang/tools/clang-fuzzer/CMakeLists.txt:12
+    # Hack to bypass LLVM's cmake sources check and allow multiple libraries and
+    # executables from this directory.
+    set(LLVM_OPTIONAL_SOURCES
----------------
vitalybuka wrote:
> You already download mutator, so maybe just DOWNLOAD_PROTOBUF and simplify this piece?
That would be simpler if only protobuf-mutator needed protobuf.  But since we need protobuf for some of the source files here, it would actually make this CMakeLists.txt more complicated since it would have to fish for the paths where protobuf mutator builds protobuf and then redefine variables.


================
Comment at: clang/tools/clang-fuzzer/ClangFuzzer.cpp:20
 
 extern "C" int LLVMFuzzerTestOneInput(uint8_t *data, size_t size) {
   std::string s((const char *)data, size);
----------------
vitalybuka wrote:
> Do we want replace this fuzzer? Why not just add another one?
The idea was to keep ClangFuzzer and add ClangProtoFuzzer alongside it.  However, the two share a fair amount of code, which was factored out into HandleCXX.


================
Comment at: clang/tools/clang-fuzzer/cxx_proto.proto:93
+}
+
+package clang_fuzzer;
----------------
vitalybuka wrote:
> message CxxInput {
>   required Function f = 1;
>   required int/enum opt_level = 2;
>   required enum tripple = 3;
>   required scalar-evolution-max-arith-depth ...
> }
Interesting idea.  This would allow for protobuf-mutator to choose different option combinations, if I understand correctly.

Is that worth adding to this initial patch, though?


https://reviews.llvm.org/D36324