[PATCH] D56194: [gn build] Add fuzzers in llvm/tools that are needed for check-llvm

[Nico Weber via Phabricator via llvm-commits]

thakis marked an inline comment as done.
thakis added inline comments.


================
Comment at: llvm/utils/gn/build/fuzzer.gni:29
+template("fuzzer") {
+  assert(defined(invoker.dummy_main))
+  assert(defined(invoker.sources))
----------------
phosek wrote:
> thakis wrote:
> > phosek wrote:
> > > I checked the LLVM `add_llvm_fuzzer` function and `DUMMY_MAIN` is just an optional argument which is only included in `LLVM_OPTIONAL_SOURCES` for IDE but otherwise ignored if `llvm_use_sanitize_coverage` or `llvm_use_sanitize_coverage` is set. Shouldn't this behave the same?
> > This template also only uses dummy_main if `llvm_lib_fuzzing_engine` and `llvm_use_sanitize_coverage` both aren't set  (see the two `not_needed`s) -- it tries to do the same. Maybe I got it wrong?
> If I'm reading `add_llvm_fuzzer`, it's not going to give you an error if you don't pass `DUMMY_MAIN` while this template will. I think it's fine since you don't want the behavior of this target differ based on whether `llvm_use_sanitize_coverage` or `llvm_use_sanitize_coverage` is set or not, but I want to point out the difference. In this case it'd be probably better to change CMake to make those arguments required as well.
> 
> Can you please also provide an error message for this assertion and the one below?
Ah, I see what you mean. I agree it'd be good to always require it on the cmake side too :-)

Landing with error messages added for the asserts.


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

https://reviews.llvm.org/D56194