Implement SanitizerCoverage support (Refs. #6513) #6517

LebedevRI · 2021-12-25T14:49:25Z

Please refer to https://clang.llvm.org/docs/SanitizerCoverage.html

TLDR: ModuleSanitizerCoveragePass instruments the IR by inserting
calls to callbacks at certain constructs. What the callbacks should do
is up to the implementation. They are effectively required for fuzzing
to be effective, and are provided by e.g. libfuzzer.

One huge caveat is SanitizerCoverageOptions which controls
which which callbacks should actually be inserted.
I just don't know what to do about it. Right now i have hardcoded
the set that would have been enabled by -fsanitize=fuzzer-no-link,
because the alternative, due to halide unflexibility,
would be to introduce ~16 suboptions to control each one.

LebedevRI · 2021-12-29T23:00:37Z

Hmm, looks like the test needs to be less fragile. Will look into that.

Please refer to https://clang.llvm.org/docs/SanitizerCoverage.html TLDR: `ModuleSanitizerCoveragePass` instruments the IR by inserting calls to callbacks at certain constructs. What the callbacks should do is up to the implementation. They are effectively required for fuzzing to be effective, and are provided by e.g. libfuzzer. One huge caveat is `SanitizerCoverageOptions` which controls which which callbacks should actually be inserted. I just don't know what to do about it. Right now i have hardcoded the set that would have been enabled by `-fsanitize=fuzzer-no-link`, because the alternative, due to halide unflexibility, would be to introduce ~16 suboptions to control each one.

alexreinking · 2021-12-30T21:35:17Z

Just so you know, there's no need to force push... we always squash-merge our PRs and the history can be useful for review.

alexreinking · 2021-12-30T21:36:35Z

Requesting reviews from @steven-johnson since he's familiar with the sanitizer stuff and from @zvookin since he will have opinions on how to deal with passing options to this.

LebedevRI · 2021-12-30T21:48:44Z

Requesting reviews from @steven-johnson since he's familiar with the sanitizer stuff and from @zvookin since he will have opinions on how to deal with passing options to this.

Thanks!
Re options: we basically only have two choices:

just add a single best-guess meta-feature, like done here, that hardcodes the set of coverage to enable,
or add ~16 features to toggle each option in SanitizerCoverageOptions struct.

test/generator/sancov_aottest.cpp

steven-johnson · 2022-01-03T17:59:44Z

One huge caveat is SanitizerCoverageOptions which controls which which callbacks should actually be inserted. I just don't know what to do about it. Right now i have hardcoded the set that would have been enabled by -fsanitize=fuzzer-no-link, because the alternative, due to halide unflexibility, would be to introduce ~16 suboptions to control each one.

I think that's probably fine for the time being -- we can expand and add flexibility in the future as desired. That said, we should add a tracking issue here ("Target::SanitizerCoverage only supports equivalent of fuzzer-no-link") and add a link to it in comments at the relevant sites in the code.

steven-johnson

Some nits re: style and comments, otherwise LGTM pending green

steven-johnson · 2022-01-03T17:53:36Z

src/Module.cpp

            Target::MSAN,
            Target::NoRuntime,
            Target::TSAN,
+            Target::SANCOV,


annoying style nit: all-caps SANCOV seems out of place here as an all-caps abbreviation, despite MSAN, etc, since the latter are commonly referred to that way in LLVM docs but the former seems to be SanitizerCoverage. If we are using this only for testing purposes then I think we should use either SanitizerCoverage or sanitizer_coverage to fit in with existing patterns. (Tagging @abadams here for his opinion as well.)

IMO SanitizerCoverage to match the llvm docs, but I don't feel strongly about it

Do you want me to change all instances of SANCOV to SanitizerCoverage, or just some?

If we are using this only for testing purposes

I'm not sure what you mean by that? Roughly, the feature will be requested the way it is requested in the test.

change all instances of SANCOV to SanitizerCoverage, or just some?

I'd vote for all (except the ones in HalideRuntime.h, which should be sanitizer_coverage to match the conventions there)

Ok, let me know if i did it wrong.

test/generator/sancov_aottest.cpp

steven-johnson · 2022-01-03T18:37:41Z

src/Target.cpp

    {"rvv", Target::RVV},
    {"armv81a", Target::ARMv81a},
-    {"sancov", Target::SANCOV},
+    {"sanitizercoverage", Target::SanitizerCoverage},


"sanitizer_coverage", so that it matches the name in HalideRuntime.h

steven-johnson · 2022-01-03T18:37:59Z

src/Target.cpp

 #endif
 #if __has_feature(coverage_sanitizer)
-    host.set_feature(Target::SANCOV);
+    host.set_feature(Target::SANITIZERCOVERAGE);


This looks like it won't compile, did you mean Target::SanitizerCoverage

~~I did verify that this builds for me, starting with clean build dir.~~
Right, thanks, i didn't test-build halide with sancov.

steven-johnson · 2022-01-03T18:38:08Z

src/Target.cpp


    if ((features & matching_mask) != (other.features & matching_mask)) {
-        Internal::debug(1) << "runtime targets must agree on SoftFloatABI, Debug, TSAN, ASAN, MSAN, HVX, HexagonDma, HVX_shared_object, SANCOV\n"
+        Internal::debug(1) << "runtime targets must agree on SoftFloatABI, Debug, TSAN, ASAN, MSAN, HVX, HexagonDma, HVX_shared_object, SANITIZERCOVERAGE\n"


SanitizerCoverage

steven-johnson · 2022-01-03T18:38:56Z

test/generator/sanitizercoverage_generator.cpp

 namespace {

-class SANCOV : public Halide::Generator<SANCOV> {
+class SANITIZERCOVERAGE : public Halide::Generator<SANITIZERCOVERAGE> {


s/SANITIZERCOVERAGE/SanitizerCoverage/ or really anything that isn't ALLCAPS

steven-johnson · 2022-01-03T18:39:13Z

test/generator/sanitizercoverage_generator.cpp


    void generate() {
-        // Currently the test just exercises Target::SANCOV
+        // Currently the test just exercises Target::SANITIZERCOVERAGE


Target::SanitizerCoverage

steven-johnson · 2022-01-03T18:39:18Z

test/generator/sanitizercoverage_generator.cpp


 private:
-    // Currently the test just exercises Target::SANCOV
+    // Currently the test just exercises Target::SANITIZERCOVERAGE


Target::SanitizerCoverage

steven-johnson

Thanks for the fixes!

LebedevRI · 2022-01-04T16:41:11Z

@alexreinking @steven-johnson Thank you!

LebedevRI force-pushed the sancov branch from c907e23 to 8334ce5 Compare December 25, 2021 14:58

alexreinking requested a review from halidebuildbots December 28, 2021 22:45

alexreinking added enhancement New user-visible features or improvements to existing features. release_notes For changes that may warrant a note in README for official releases. usability labels Dec 28, 2021

LebedevRI force-pushed the sancov branch 6 times, most recently from ab8fc5d to b312eee Compare December 30, 2021 20:49

LebedevRI force-pushed the sancov branch from b312eee to ef16a46 Compare December 30, 2021 21:29

alexreinking requested review from steven-johnson and zvookin December 30, 2021 21:35

abadams reviewed Dec 31, 2021

View reviewed changes

test/generator/sancov_aottest.cpp Outdated Show resolved Hide resolved

Simplify test

4c0f3b6

steven-johnson requested changes Jan 3, 2022

View reviewed changes

sancov test: avoid potential signedness warnings.

300e119

LebedevRI mentioned this pull request Jan 3, 2022

Target::SanitizerCoverage only supports equivalent of fuzzer-no-link #6528

Closed

Rename all instances of sancov to sanitizecoverage

63e7113

steven-johnson requested changes Jan 3, 2022

View reviewed changes

LebedevRI added 3 commits January 3, 2022 21:52

Adjust spelling of "SanitizerCoverage" in some places

fdf5656

Actually adjust the feature name in build system for the test

a52451e

Hopefully fix Makefile build

b0b95d9

Merge branch 'master' into pr/6517

b6cb534

steven-johnson approved these changes Jan 4, 2022

View reviewed changes

steven-johnson merged commit f11d820 into halide:master Jan 4, 2022

LebedevRI deleted the sancov branch January 4, 2022 16:56

AtilaSaraiva mentioned this pull request Jul 3, 2022

halide: 10.0.0 -> 14.0.0 NixOS/nixpkgs#180015

Merged

13 tasks

Implement SanitizerCoverage support (Refs. #6513) #6517

Implement SanitizerCoverage support (Refs. #6513) #6517

Uh oh!

Conversation

LebedevRI commented Dec 25, 2021

Uh oh!

LebedevRI commented Dec 29, 2021

Uh oh!

alexreinking commented Dec 30, 2021

Uh oh!

alexreinking commented Dec 30, 2021

Uh oh!

LebedevRI commented Dec 30, 2021

Uh oh!

Uh oh!

steven-johnson commented Jan 3, 2022

Uh oh!

steven-johnson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LebedevRI Jan 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steven-johnson left a comment

Choose a reason for hiding this comment

Uh oh!

LebedevRI commented Jan 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LebedevRI Jan 3, 2022 •

edited

Loading