This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by thakis on Jan 1 2019, 6:48 PM.

Details

Summary

Also add a fuzzer() template for defining fuzzers that's similar to add_llvm_fuzzer in the CMake build, and a build file for dependency llvm/lib/FuzzMutate.

Diff Detail

Repository
rL LLVM

Event Timeline

thakis created this revision.Jan 1 2019, 6:48 PM
phosek added inline comments.Jan 1 2019, 7:50 PM
llvm/utils/gn/build/fuzzer.gni
29 ↗(On Diff #179812)

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?

thakis marked an inline comment as done.Jan 1 2019, 8:06 PM
thakis added inline comments.
llvm/utils/gn/build/fuzzer.gni
29 ↗(On Diff #179812)

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_neededs) -- it tries to do the same. Maybe I got it wrong?

phosek accepted this revision.Jan 2 2019, 9:38 AM

LGTM

llvm/utils/gn/build/fuzzer.gni
29 ↗(On Diff #179812)

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?

This revision is now accepted and ready to land.Jan 2 2019, 9:38 AM
thakis marked an inline comment as done.Jan 2 2019, 10:14 AM
thakis added inline comments.
llvm/utils/gn/build/fuzzer.gni
29 ↗(On Diff #179812)

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.

This revision was automatically updated to reflect the committed changes.