On 2021.03.04 14:48, Junio C Hamano wrote: > "Andrzej Hunt via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > > > From: Andrzej Hunt <ajrhunt@xxxxxxxxxx> > > > Subject: Re: [PATCH v2] Update 'make fuzz-all' docs to reflect modern clang > > I'd retitte it to > > Makefile: update 'make fuzz-all' docs to reflect modern clang > > > Clang no longer produces a libFuzzer.a, instead you can include > > libFuzzer by using -fsanitize=fuzzer. > > Do we see two sentences here? IOW, s/, instead/. Instead/ is needed? > > > Therefore we should use > > that in the example command for building fuzzers. > > > > We also add -fsanitize=fuzzer-no-link to ensure that all the required > > instrumentation is added when compiling git [1], and remove > > -fsanitize-coverage=trace-pc-guard as it is deprecated. > > Without something like s/add/add to CFLAGS/, I found this a bit > cryptic and failed to read what it wanted to do without looking at > the patch text itself. > > > I happen to have tested with LLVM 11 - however -fsanitize=fuzzer appears to > > work in a wide range of reasonably modern clangs. > > > > (On my system: what used to be libFuzzer.a now lives under the following path, > > which is tricky albeit not impossible for a novice such as myself to find: > > /usr/lib64/clang/11.0.0/lib/linux/libclang_rt.fuzzer-x86_64.a ) > > All nice things to have in the log message. > > > Makefile | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index dd08b4ced01c..c7248ac6057b 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -3292,11 +3292,11 @@ cover_db_html: cover_db > > # are not necessarily appropriate for general builds, and that vary greatly > > # depending on the compiler version used. > > # > > -# An example command to build against libFuzzer from LLVM 4.0.0: > > +# An example command to build against libFuzzer from LLVM 11.0.0: > > # > > # make CC=clang CXX=clang++ \ > > -# CFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ > > -# LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \ > > +# CFLAGS="-fsanitize=fuzzer-no-link,address" \ > > +# LIB_FUZZING_ENGINE="-fsanitize=fuzzer" \ > > # fuzz-all > > # > > FUZZ_CXXFLAGS ?= $(CFLAGS) > > LIB_FUZZING_ENGINE is used this way in the Makefile: > > $(FUZZ_PROGRAMS): all > $(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) $(LIB_OBJS) $(BUILTIN_OBJS) \ > $(XDIFF_OBJS) $(EXTLIBS) git.o $@.o $(LIB_FUZZING_ENGINE) -o $@ > > and it is somewhat annoying to see a compiler/linker option that > late on the command line, where readers would expect an object file > or a library archive would appear. Yes, it appears that clang has changed how the fuzzing engine is selected, as this used to be just a library path (as you see in the diff). We might as well move this option up with the rest of the flags. > It makes me wonder if we should > instead be doing something along the following line: > > - empty LIB_FUZZING_ENGINE by default > - add -fsanitize=fuzzer names to FUZZ_CXXFLAGS > > i.e. > > diff --git c/Makefile w/Makefile > index 4128b457e1..b5df76b33b 100644 > --- c/Makefile > +++ w/Makefile > @@ -3306,14 +3306,15 @@ cover_db_html: cover_db > # are not necessarily appropriate for general builds, and that vary greatly > # depending on the compiler version used. > # > -# An example command to build against libFuzzer from LLVM 4.0.0: > +# An example command to build against libFuzzer from LLVM 11.0.0: > # > # make CC=clang CXX=clang++ \ > -# CFLAGS="-fsanitize-coverage=trace-pc-guard -fsanitize=address" \ > -# LIB_FUZZING_ENGINE=/usr/lib/llvm-4.0/lib/libFuzzer.a \ > +# CFLAGS="-fsanitize=fuzzer-no-link,address" \ > # fuzz-all > # > FUZZ_CXXFLAGS ?= $(CFLAGS) > +FUZZ_CXXFLAGS += -fsanitize=fuzzer > +LIB_FUZZING_ENGINE = I don't think we want to mess with FUZZ_CXXFLAGS, as oss-fuzz may be adding conflicting -fsanitize args here. Having LIB_FUZZING_ENGINE default to empty should be fine though. > > .PHONY: fuzz-all > > > In the meantime, I'll queue the version you sent as-is (modulo the > retitling). > > Thanks. > >
