[Lldb-commits] [PATCH] D129377: [lldb/Fuzzer] Add fuzzer for expression evaluator

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: clang/cmake/modules/ProtobufMutator.cmake:4-5
+  set (PBM_PREFIX clang_protobuf_mutator)
+elseif(${CMAKE_CURRENT_SOURCE_DIR} MATCHES "lldb")
+  set (PBM_PREFIX lldb_protobuf_mutator)
+endif()
----------------
cassanova wrote:
> mib wrote:
> > 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
> 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
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129377/new/

https://reviews.llvm.org/D129377