Page MenuHomePhabricator

[SanitizerCoverage] Add `__sanitizer_cov_bool_flag_init` as the weak interface functions.
ClosedPublic

Authored by pratyai on Apr 9 2020, 9:57 PM.

Details

Summary

Following up the discussion on D77638 (and following rGd6cfed6060c283dc4a6bf9ca294dcd732e8b9f72 as example), defining __sanitizer_cov_bool_flag_init as the weak interface functions in various compiler-rt/ files.

[Please note that 2 clang-tidy checks fail. I believe clang-tidy is mistaken in this case.]

Not included here:

  • compiler-rt/lib/fuzzer/FuzzerTracePC.cpp [need to implement the handler function]
  • compiler-rt/lib/fuzzer/FuzzerTracePC.h
  • clang/lib/Driver/SanitizerArgs.cpp [tied to FuzzerTracePC.cpp]
  • compiler-rt/lib/fuzzer/CMakeLists.txt
  • clang/test/Driver/fuzzer.c
  • compiler-rt/lib/sanitizer_common/sanitizer_coverage_win_sections.cpp [this file is about windows specific tricks]

I'll put all the fuzzer related changes together in another diff.

Diff Detail

Event Timeline

pratyai created this revision.Apr 9 2020, 9:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2020, 9:57 PM
Herald added subscribers: Restricted Project, dberris. · View Herald Transcript
pratyai edited the summary of this revision. (Show Details)Apr 9 2020, 10:05 PM
pratyai edited the summary of this revision. (Show Details)Apr 9 2020, 10:07 PM
pratyai edited the summary of this revision. (Show Details)
pratyai edited the summary of this revision. (Show Details)Apr 10 2020, 7:16 AM
pratyai updated this revision to Diff 256589.Apr 10 2020, 9:25 AM
pratyai edited the summary of this revision. (Show Details)
pratyai updated this revision to Diff 256651.Apr 10 2020, 1:41 PM
pratyai retitled this revision from [SanitizerCoverage] Add `__sanitizer_cov_bool_flag_init` as the weak interface functions in various compiler-rt/ files. to [SanitizerCoverage] Add `__sanitizer_cov_bool_flag_init` as the weak interface functions..
pratyai edited the summary of this revision. (Show Details)Apr 10 2020, 2:56 PM
pratyai edited the summary of this revision. (Show Details)
pratyai added a reviewer: vitalybuka.

could you try to add libfuzzer test?

vitalybuka accepted this revision.Apr 14 2020, 4:42 PM
This revision is now accepted and ready to land.Apr 14 2020, 4:42 PM

could you try to add libfuzzer test?

Will add a test in compiler-rt/test/fuzzer/.

pratyai updated this revision to Diff 257653.Apr 15 2020, 3:29 AM

Added LinkTest.cpp and link.test, just to verify that the weak interfaces would allow to combine the -fsanitize-coverage=inline-bool-flag with -fsanitize=fuzzer without linker errors.

Verified by dropping the weak interfaces and keeping the tests: the test would complain that __sanitizer_cov_bool_flag_init has undefined reference.

pratyai updated this revision to Diff 258228.Apr 16 2020, 8:46 PM

I'll take an attempt of landing it today if it's alright, since it's accepted. Was waiting in case the newly added test files were problematic. Thank!

pratyai updated this revision to Diff 259682.Apr 23 2020, 12:49 PM
vitalybuka accepted this revision.Apr 23 2020, 5:38 PM

Hmm, it seems I cannot do arc land (permission denied, 403). In that case, would you please land the diff? Thanks!

[there are a bunch of python failure but they seem unrelated and have been failing for other diffs too; the clang-tidy errors still false alarm]

Sure. I wiil.

compiler-rt/test/fuzzer/LinkTest.cpp
5

I guess you can remove this file and just compile any existing file

pratyai marked an inline comment as done.Apr 24 2020, 12:09 AM
pratyai added inline comments.
compiler-rt/test/fuzzer/LinkTest.cpp
5

Will do; will update the diff after running the tests.

pratyai updated this revision to Diff 259824.Apr 24 2020, 12:50 AM

I removed the test. After some consideration is not a good place, but even in sanitizer_common it's not very useful as it's hard to get new problems like this in future.

I'll put all the fuzzer related changes together in another diff.

What kind of changes?

This revision was automatically updated to reflect the committed changes.

I removed the test. After some consideration is not a good place, but even in sanitizer_common it's not very useful as it's hard to get new problems like this in future.

I'll put all the fuzzer related changes together in another diff.

What kind of changes?

Fuzzer seems to automatically include other sanitizer coverage features,:
https://reviews.llvm.org/source/llvm-github/browse/master/clang/lib/Driver/SanitizerArgs.cpp$406
And have callbacks for them:
https://reviews.llvm.org/source/llvm-github/browse/master/compiler-rt/lib/fuzzer/FuzzerTracePC.cpp$38
If the new bool flag feature should also be in there, I can add it. If it's not necessarily the case, I can leave it for future decisions.

Then there is
https://reviews.llvm.org/source/llvm-github/browse/master/compiler-rt/lib/sanitizer_common/sanitizer_coverage_win_sections.cpp$35
which seems to be necessary for the new flag to work on windows systems? I'm trying to figure out a way to test it before sending any diffs. Please let me know if that change is unnecessary too.

I couldn't find anything else that might need change other than what's listed in the description-- at least not from looking at how the inline 8bit counters is used in the codebase.

I removed the test. After some consideration is not a good place, but even in sanitizer_common it's not very useful as it's hard to get new problems like this in future.

I'll put all the fuzzer related changes together in another diff.

What kind of changes?

Fuzzer seems to automatically include other sanitizer coverage features,:
https://reviews.llvm.org/source/llvm-github/browse/master/clang/lib/Driver/SanitizerArgs.cpp$406
And have callbacks for them:
https://reviews.llvm.org/source/llvm-github/browse/master/compiler-rt/lib/fuzzer/FuzzerTracePC.cpp$38
If the new bool flag feature should also be in there, I can add it. If it's not necessarily the case, I can leave it for future decisions.

Then there is
https://reviews.llvm.org/source/llvm-github/browse/master/compiler-rt/lib/sanitizer_common/sanitizer_coverage_win_sections.cpp$35
which seems to be necessary for the new flag to work on windows systems? I'm trying to figure out a way to test it before sending any diffs. Please let me know if that change is unnecessary too.

I couldn't find anything else that might need change other than what's listed in the description-- at least not from looking at how the inline 8bit counters is used in the codebase.

I think this is unnecessary, even if some other counter are used by libFuzzer. In the current state bool flags does not break anything and we add them into libFuzzer only when needed.
But I don't mind to review your changes if you like to continue.