[lldb/Fuzzer] Add fuzzer for expression evaluator

[cassanova]

Summary

This commit adds a fuzzer for LLDB's expression evaluator. The fuzzer takes a different approach than the current fuzzers present, and uses an approach that is currently being used for clang fuzzers.

Instead of fuzzing the evaluator with randomly mutated characters, protobufs are used to generate a subset of C++. This is then converted to valid C++ code and sent to the expression evaluator. In addition, libprotobuf_mutator is used to mutate the fuzzer's inputs from valid C++ code to valid C++ code, rather than mutating from valid code to total nonsense.

[cassanova]

Building the expression evaluator fuzzer is now conditional on the CLANG_ENABLE_PROTO_FUZZER CMake variable being enabled.

Copying the source and header files from is no longer being done in the top-level CMake file, this is instead added to the subdirectories of the clang fuzzer.

The fuzzer uses Clang's CMake modules for libprotobuf_mutator instead of copying the module into LLDB.

[JDevlieghere]

The review shows the diff with the previous iteration of the patch. Can you generate a diff with the current top-of-tree?

[cassanova]

Shows top-of-tree changes, however CMake generation fails that the ProtobufMutator target already exists for clang-fuzzer:

CMake Error at /opt/homebrew/Cellar/cmake/3.23.1_1/share/cmake/Modules/ExternalProject.cmake:3453 (add_custom_target):
  add_custom_target cannot create target "protobuf_mutator" because another
  target with the same name already exists.  The existing target is a custom
  target created in source directory
  "/Users/chelseacassanova/code/llvm-project/clang/tools/clang-fuzzer".

[cassanova]

lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/CMakeLists.txt

21

Commenting out this line causes the project to generate, but I get file not found errors when including the protobuf errors when trying to build the fuzzer.

[cassanova]

The ProtobufMutator CMake module will build targets for clang and lldb individually depending on which project is building the mutator, instead of both fuzzers trying to build the same target.

The expression fuzzer's source file only includes handle-cxx and proto-to-cxx directly, instead of including them from their folders.

The expression fuzzer's CMake file adds the clang-fuzzer binary directory as a include directory so that the lldb fuzzer does not need to generate a second copy of cxx_proto.pb.h and cxx_proto.pb.cc. It also requires the Protobuf library, grabs its definitions and includes the protobuf include dirs to prevent a protobuf header from not being found in the expression fuzzer source file.

[mib]

clang/cmake/modules/ProtobufMutator.cmake

4–5 ↗

(On Diff #444074)

If feels wrong to me that the clang protobuf cmake module knows about lldb.

May be we should just have 2 separate files for clang and lldb

[cassanova]

clang/cmake/modules/ProtobufMutator.cmake

4–5 ↗

(On Diff #444074)

My preferred solution to this was just creating a target called ${LLVM_VARIABLE_THAT_TELLS_YOU_THE_SUBPROJECT_NAME}_protobuf_mutator to avoid using if-statements and direct strings but it looks like clang and lldb aren't defined as subprojects unless they're being built standalone.

Also in the event that some other subproject wanted to use this library then this solution just gets messier. Having 2 different CMake module files for the clang and lldb sides each or putting protobuf mutator in a more central location is another way to approach this

[JDevlieghere]

clang/cmake/modules/ProtobufMutator.cmake

4–5 ↗

(On Diff #444074)

I agree with Ismail. I think we can fix this issue by making it possible to set the PBM_PREFIX in the scope that includes ProtobufMutator.cmake like this:

In clang/cmake/modules/ProtobufMutator.cmake:

if (NOT PBM_PREFIX) 
  set (PBM_PREFIX protobuf_mutator)
endif()

In lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/CMakeLists.txt:

set (PBM_PREFIX lldb_protobuf_mutator)
include(ProtobufMutator)

That said, I think an even better way to do this is by using a cached variable that keep track of whether we've already included the external project. That way we don't have to create two different targets that are essentially the same. If I'm following the code correctly, all the variables that we rely on can be computed without the ExternalProject_Add. I imagine something like:

if (NOT PBM_PROJECT_ADDED) 
  ExternalProject_Add(${PBM_PREFIX}
    PREFIX ${PBM_PREFIX}
    GIT_REPOSITORY https://github.com/google/libprotobuf-mutator.git
    GIT_TAG master
    CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
    CMAKE_CACHE_ARGS -DCMAKE_C_COMPILER:FILEPATH=${CMAKE_C_COMPILER}
                     -DCMAKE_CXX_COMPILER:FILEPATH=${CMAKE_CXX_COMPILER}
    BUILD_BYPRODUCTS ${PBM_LIB_PATH} ${PBM_FUZZ_LIB_PATH}
    UPDATE_COMMAND ""
    INSTALL_COMMAND ""
    )
    set(PBM_PROJECT_ADDED TRUE CACHE BOOL
    "Whether the ProtoBufMutator external project was added")
endif()

I think that should preclude us from creating the target twice.

[cassanova]

Updated the ProtobufMutator CMake module and expression fuzzer CMakeLists file so that the expression fuzzer will create its own target name in its CMake file, and the ProtobufMutator will not attempt to create another target if one already exists

Also updated the expression fuzzer's CMake file to create a directory to store expression fuzzer artifacts.

[JDevlieghere]

lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/cxx_proto.proto

1 ↗

(On Diff #444361)

Do we still need a copy of this for LLDB?

[cassanova]

lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/cxx_proto.proto

1 ↗

(On Diff #444361)

This file is used to generate the cxx_proto.pb.h which is included in the source file. Since we just use the clang-fuzzer directory as an include directory then we shouldn't need this file anymore

[cassanova]

Removed the cxx_proto.proto file since we include the headers that it generates from the clang side.

[JDevlieghere]

A few nits about naming, but otherwise this LGTM.

lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/lldb-expression-fuzzer.cpp

47 ↗

(On Diff #444761)

51–52 ↗

(On Diff #444761)

59 ↗

(On Diff #444761)

62 ↗

(On Diff #444761)

[mstorsjo]

This broke building of Clang, when it's set up by symlinking llvm-project/clang into llvm-project/llvm/tools. In that case, cmake configure errors out like this:

-- Configuring done 
CMake Error in tools/clang/tools/clang-fuzzer/handle-cxx/CMakeLists.txt:
  Target "clangHandleCXX" INTERFACE_INCLUDE_DIRECTORIES property contains
  path:
 
    "/home/martin/code/llvm-project/llvm/tools/clang/tools/clang-fuzzer/handle-cxx/." 

  which is prefixed in the source directory.

See e.g. https://stackoverflow.com/questions/25676277/cmake-target-include-directories-prints-an-error-when-i-try-to-add-the-source for some discussion on the issue.

Can we revert this commit until this has been sorted out?

[mib]

In D129377#3673204, @mstorsjo wrote:

This broke building of Clang, when it's set up by symlinking llvm-project/clang into llvm-project/llvm/tools. In that case, cmake configure errors out like this:

-- Configuring done 
CMake Error in tools/clang/tools/clang-fuzzer/handle-cxx/CMakeLists.txt:
  Target "clangHandleCXX" INTERFACE_INCLUDE_DIRECTORIES property contains
  path:
 
    "/home/martin/code/llvm-project/llvm/tools/clang/tools/clang-fuzzer/handle-cxx/." 

  which is prefixed in the source directory.

See e.g. https://stackoverflow.com/questions/25676277/cmake-target-include-directories-prints-an-error-when-i-try-to-add-the-source for some discussion on the issue.

Can we revert this commit until this has been sorted out?

@mstorsjo I reverted Chelsea's patch in b797834748f1, since she's offline now. Do you have a link to a bot failure that would help us investigate the issue ?

Thanks!

[mstorsjo]

In D129377#3673237, @mib wrote:

In D129377#3673204, @mstorsjo wrote:

This broke building of Clang, when it's set up by symlinking llvm-project/clang into llvm-project/llvm/tools. In that case, cmake configure errors out like this:

-- Configuring done 
CMake Error in tools/clang/tools/clang-fuzzer/handle-cxx/CMakeLists.txt:
  Target "clangHandleCXX" INTERFACE_INCLUDE_DIRECTORIES property contains
  path:
 
    "/home/martin/code/llvm-project/llvm/tools/clang/tools/clang-fuzzer/handle-cxx/." 

  which is prefixed in the source directory.

See e.g. https://stackoverflow.com/questions/25676277/cmake-target-include-directories-prints-an-error-when-i-try-to-add-the-source for some discussion on the issue.

Can we revert this commit until this has been sorted out?

@mstorsjo I reverted Chelsea's patch in b797834748f1, since she's offline now. Do you have a link to a bot failure that would help us investigate the issue ?

Thanks!

Thanks!

It’s not on a public buildbot but only in my local continuous builds unfortunately - but I’ll have look at the issue myself today - I was running out of time yesterday.

[mstorsjo]

In D129377#3673237, @mib wrote:

In D129377#3673204, @mstorsjo wrote:

This broke building of Clang, when it's set up by symlinking llvm-project/clang into llvm-project/llvm/tools. In that case, cmake configure errors out like this:

-- Configuring done 
CMake Error in tools/clang/tools/clang-fuzzer/handle-cxx/CMakeLists.txt:
  Target "clangHandleCXX" INTERFACE_INCLUDE_DIRECTORIES property contains
  path:
 
    "/home/martin/code/llvm-project/llvm/tools/clang/tools/clang-fuzzer/handle-cxx/." 

  which is prefixed in the source directory.

See e.g. https://stackoverflow.com/questions/25676277/cmake-target-include-directories-prints-an-error-when-i-try-to-add-the-source for some discussion on the issue.

Can we revert this commit until this has been sorted out?

@mstorsjo I reverted Chelsea's patch in b797834748f1, since she's offline now. Do you have a link to a bot failure that would help us investigate the issue ?

So in short, I think this would work if you'd simply remove the newly added target_include_directories(clangHandleCXX PUBLIC .) in clang/tools/clang-fuzzer/handle-cxx/CMakeLists.txt and target_include_directories(clangProtoToCXX PUBLIC .) and target_include_directories(clangLoopProtoToCXX PUBLIC .) in clang/tools/clang-fuzzer/proto-to-cxx/CMakeLists.txt.

To reproduce the issue - instead of configuring llvm+clang+lldb by passing -DLLVM_ENABLE_PROJECTS="clang;lldb" make symlinks in llvm-project/llvm/tools pointing to llvm-project/clang and llvm-project/lldb. (I.e., cd llvm-project/llvm/tools; ln -s ../../clang .; ln -s ../../lldb .) This slightly affects the effective path at where CMake sees the clang files, which causes CMake to report the unexpected path overlap issue here.

I've tried to build all these fuzzers - I haven't really managed to build it all, but I think I'm fairly close. Anyway, I tried removing those target_include_directories() in that setup, and it didn't change anything, but maybe it only makes a difference if one gets further than what I got. (Changing PUBLIC into PRIVATE in those lines avoided the issue too.)

clang/tools/clang-fuzzer/handle-cxx/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS ${LLVM_TARGETS_TO_BUILD} Support)
 
 add_clang_library(clangHandleCXX
   handle_cxx.cpp
 
   LINK_LIBS
   clangBasic
   clangCodeGen
   clangFrontend
   clangLex
   clangSerialization
   clangTooling
   )
+
+target_include_directories(clangHandleCXX PUBLIC .)

clang/tools/clang-fuzzer/proto-to-cxx/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS ${LLVM_TARGETS_TO_BUILD})
 set(CMAKE_CXX_FLAGS ${CXX_FLAGS_NOFUZZ})
 
 # Needed by LLVM's CMake checks because this file defines multiple targets.
 set(LLVM_OPTIONAL_SOURCES proto_to_cxx.cpp proto_to_cxx_main.cpp
                           loop_proto_to_cxx.cpp loop_proto_to_cxx_main.cpp)
 
 add_clang_library(clangProtoToCXX proto_to_cxx.cpp
                   DEPENDS clangCXXProto
                   LINK_LIBS clangCXXProto ${PROTOBUF_LIBRARIES}
                   )
 
 add_clang_library(clangLoopProtoToCXX loop_proto_to_cxx.cpp
                   DEPENDS clangCXXLoopProto
                   LINK_LIBS clangCXXLoopProto ${PROTOBUF_LIBRARIES}
                   )
+target_include_directories(clangProtoToCXX PUBLIC .)
+target_include_directories(clangLoopProtoToCXX PUBLIC .)
 
 add_clang_executable(clang-proto-to-cxx proto_to_cxx_main.cpp)
 add_clang_executable(clang-loop-proto-to-cxx loop_proto_to_cxx_main.cpp)
 
 target_link_libraries(clang-proto-to-cxx PRIVATE clangProtoToCXX)
 target_link_libraries(clang-loop-proto-to-cxx PRIVATE clangLoopProtoToCXX)

lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/CMakeLists.txt

+if(CLANG_ENABLE_PROTO_FUZZER)
   set(LLVM_LINK_COMPONENTS
     Support
     )
 
   add_llvm_fuzzer(lldb-expression-fuzzer
     EXCLUDE_FROM_ALL
     lldb-expression-fuzzer.cpp
     )
 
   if(TARGET lldb-expression-fuzzer)
     target_include_directories(lldb-expression-fuzzer PRIVATE ..)
 
-  # Generate the necessary source and header files for using protobufs
-  find_package(Protobuf REQUIRED)
-  add_definitions(-DGOOGLE_PROTOBUF_NO_RTTI)
-  include_directories(${PROTOBUF_INCLUDE_DIRS})
-  include_directories(${CMAKE_CURRENT_BINARY_DIR})
-  protobuf_generate_cpp(PROTO_SRCS PROTO_HDRS cxx_proto.proto)
-  protobuf_generate_cpp(LOOP_PROTO_SRCS LOOP_PROTO_HDRS cxx_loop_proto.proto)
-  set(LLVM_OPTIONAL_SOURCES ${LLVM_OPTIONAL_SOURCES} ${PROTO_SRCS})
-
-  # Place the source and header files into a library for use by LLDB's expression fuzzer
-  # FIXME: It would be better to use add_lldb_library, but using this will cause an error
-  # during cmake's file generation phase
-  add_library(lldbCXXProto
-    ${PROTO_SRCS}
-    ${PROTO_HDRS}
-    )
+    set(CLANG_CMAKE_MODULE_PATH
+      ${CMAKE_CURRENT_SOURCE_DIR}/../../../../clang/cmake/module)
+
+    set(CMAKE_MODULE_PATH
+      ${CMAKE_MODULE_PATH}
+      ${CLANG_CMAKE_MODULE_PATH})
 
-  # Build and include the libprotobuf-mutator repository
     include(ProtobufMutator)
     include_directories(${ProtobufMutator_INCLUDE_DIRS})
 
-  # Create a variable for the libraries generated by protobuf and protobuf mutator
-  set(COMMON_PROTO_FUZZ_LIBRARIES
-    ${ProtobufMutator_LIBRARIES}
-    ${PROTOBUF_LIBRARIES}
-    )
-
-  # Link the protobuf libraries as well as the clang libraries used to
-  # convert protobufs to C/C++
     target_link_libraries(lldb-expression-fuzzer
       PRIVATE
-    ${COMMON_PROTO_FUZZ_LIBRARIES}
+      ${ProtobufMutator_LIBRARIES}
+      ${LLVM_LIB_FUZZING_ENGINE}
       clangHandleCXX
-    lldbCXXProto
+      clangCXXProto
       clangProtoToCXX
       liblldb
       )
 
-  # The target for this fuzzer needs to depend on the protobuf mutator
-  # repository
-  add_dependencies(lldb-expression-fuzzer lldb_protobuf_mutator)
-
     add_custom_command(TARGET lldb-expression-fuzzer PRE_BUILD
-
-    # FIXME: Copying the source and header files is not the preferred way to implement these libraries
-    # on the LLDB side. It would be preferable to have the libraries for protobuf fuzzers be located
-    # in a more central location
-
-    # Create directories to store the files for handle-cxx and proto-to-cxx since the protobuf mutator
-    # depends on them
-    COMMAND ${CMAKE_COMMAND} -E make_directory ${CMAKE_CURRENT_BINARY_DIR}/handle-cxx
-    COMMAND ${CMAKE_COMMAND} -E make_directory ${CMAKE_CURRENT_BINARY_DIR}/proto-to-cxx
-
-    # Copy the header and source files for handle-cxx from clang
-    COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_SOURCE_DIR}/../../../../clang/tools/clang-fuzzer/handle-cxx/handle_cxx.h ${CMAKE_CURRENT_BINARY_DIR}/handle-cxx
-    COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_SOURCE_DIR}/../../../../clang/tools/clang-fuzzer/handle-cxx/handle_cxx.cpp ${CMAKE_CURRENT_BINARY_DIR}/handle-cxx
-
-    # Copy the header and source files for proto-to-cxx from clang
-    COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_SOURCE_DIR}/../../../../clang/tools/clang-fuzzer/proto-to-cxx/proto_to_cxx.h ${CMAKE_CURRENT_BINARY_DIR}/proto-to-cxx
-    COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_SOURCE_DIR}/../../../../clang/tools/clang-fuzzer/proto-to-cxx/proto_to_cxx.cpp ${CMAKE_CURRENT_BINARY_DIR}/proto-to-cxx
-    COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_SOURCE_DIR}/../../../../clang/tools/clang-fuzzer/proto-to-cxx/proto_to_cxx_main.cpp ${CMAKE_CURRENT_BINARY_DIR}/proto-to-cxx
-
       # Create and compile a simple C program using the command line. This is
       # needed because LLDB's expression evaluator needs a legitmate target
       # instead of a dummy target
       COMMAND echo 'int main (int argc, char** argv) { return 0\; }' | clang -o main.out -xc -
       )
 
     # Create a directory for storing the fuzzer's artifacts and run the fuzzer with arguments that will
     # not attempt to reduce the size of the inputs being generated
     add_custom_target(fuzz-lldb-expression
       COMMENT "Running the LLDB expression evaluator fuzzer..."
       WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/fuzzer-artifacts/expression-artifacts
       COMMAND $<TARGET_FILE:lldb-expression-fuzzer> -artifact_prefix=expression- -reduce_inputs=0
       USES_TERMINAL
       )
   endif()
+endif