Closed Bug 1464202 Opened 3 years ago Closed 3 years ago
Improve and centralize lib
Fuzzer flag management
59 bytes, text/x-review-board-request
The last libFuzzer upgrade to our code base has caused some problems about PC table generation (libFuzzer does no longer emit NEW_PC messages for example). After some digging and talking to :kcc, it turned out that with the current version of libFuzzer, we are supposed to use a different set of build flags. In general, he recommended using the combination flag -fsanitize=fuzzer(-no-link) which adds all the flags we need. However, this flag isn't supported in Clang <= 5. So this bug is about two things: 1) Centralize our libFuzzer flag management. Right now, we spread flags to every moz.build file that has libFuzzer target code. It would be much more desirable to have configuration files to include instead and have the flags in one place. 2) Improve the flags. Use -fsanitize=fuzzer-no-link if available. If it is not available, fall back to the old combination we used before. I was also told that we are supposed to always use the trace-cmp flag as well, in that case. The upcoming patch does all this by 1) Adding detection code for -fsanitize=fuzzer-no-link 2) Adding files in tools/fuzzer/ to include (one with the flag definitions and one having the typical CFLAGS/CXXFLAGS += directives that are almost always used). 3) Replacing all occurrences of the flags with the include. 4) special casing gyp. I wasn't able to change the code so we can simply pass the flag in. It works as it is right now in the patch, but duplicates the flags in the target gyp file which is exactly what we don't want in the long run. I'm open for suggestions, but I don't want this to block the overall refactoring.
Attachment #8980409 - Flags: feedback?(ted)
Comment on attachment 8980409 [details] Bug 1464202 - Improve and centralize libFuzzer flag management. https://reviewboard.mozilla.org/r/246580/#review252704 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: build/moz.configure/toolchain.configure:1635 (Diff revision 1) > + check_msg='whether the C compiler supports -fsanitize=fuzzer-no-link')) > +def have_libfuzzer_flag_fuzzer_no_link(value): > + if value: > + return True > + > +set_config('HAVE_LIBFUZZER_FLAG_FUZZER_NO_LINK', have_libfuzzer_flag_fuzzer_no_link) Error: Expected 2 blank lines after class or function definition, found 1 [flake8: E305]
Comment on attachment 8980409 [details] Bug 1464202 - Improve and centralize libFuzzer flag management. https://reviewboard.mozilla.org/r/246580/#review252964 ::: tools/fuzzing/libfuzzer-config.mozbuild:11 (Diff revision 1) > + > +include('libfuzzer-flags.mozbuild') > + > +if CONFIG['FUZZING']: > + if CONFIG['LIBFUZZER']: > + # Add trace-pc coverage for libfuzzer three space indent here ::: tools/fuzzing/libfuzzer-flags.mozbuild:10 (Diff revision 1) > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +libfuzzer_flags =  > + > +if CONFIG['HAVE_LIBFUZZER_FLAG_FUZZER_NO_LINK']: > + libfuzzer_flags += ['-fsanitize=fuzzer-no-link'] two space indents here, mozbuild files generally use 4 spaces
Comment on attachment 8980409 [details] Bug 1464202 - Improve and centralize libFuzzer flag management. https://reviewboard.mozilla.org/r/246580/#review254016 Please do fix the reviewbot comments before landing, or the Python lint job will burn.
Attachment #8980409 - Flags: review?(nfroyd) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/e8eb0005b50c Improve and centralize libFuzzer flag management. r=froydnj
You need to log in before you can comment on or make changes to this bug.