| Message ID | pull.889.v2.git.1614871707845.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e82816998ff1b9228a968a8513bfb58198ecdbb1 |
| Headers | show |
| Series | [v2] Update 'make fuzz-all' docs to reflect modern clang | expand |
"Andrzej Hunt via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Andrzej Hunt <ajrhunt@google.com> > 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. 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 = .PHONY: fuzz-all In the meantime, I'll queue the version you sent as-is (modulo the retitling). Thanks.
On 04/03/2021 23:48, Junio C Hamano wrote:> > 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. 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 This sounds sensible to me, and will certainly simplify the use of "make fuzz-all" by beginners - although I'm not sure just how useful the change would be since my understanding is that this target is almost exclusively used by oss-fuzz. However I would prefer to wait for Josh's feedback before making such a change, as he is the owner of oss-fuzz's git integration [1], and as such is most likely to be affected by any changes to this target. In the meantime I'll prepare an updated patch with a fixed commit message! [1] https://github.com/google/oss-fuzz/blob/c41e46ffc8bc409bdfde0c0d2c97e1305f0c4106/projects/git/project.yaml#L3
Andrzej Hunt <andrzej@ahunt.org> writes: > However I would prefer to wait for Josh's feedback before making such > a change, as he is the owner of oss-fuzz's git integration [1], and as > such is most likely to be affected by any changes to this target. > > > In the meantime I'll prepare an updated patch with a fixed commit message! Makes sense. Let's wait and see what Josh says before going forward.
On 2021.03.04 14:48, Junio C Hamano wrote: > "Andrzej Hunt via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Andrzej Hunt <ajrhunt@google.com> > > > 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. > >
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)