Teach the Swift front-end to generate code with sanitizer coverage #2527

delcypher · 2016-05-14T01:05:54Z

What's in this pull request?

Teach the Swift front-end to generate code with "Sanitizer Coverage" with a new command line flag
-sanitize-coverage=. This flag is analogous to Clang's -fsanitize-coverage=.

This instrumentation currently requires ASan or TSan to be enabled
because the module pass created by createSanitizerCoverageModulePass()
inserts calls into functions found in compiler-rt's "sanitizer_common".
"sanitizer_common" is not shipped as an individual library but instead
exists in several of the sanitizer runtime libraries so we have to
link with one of them to avoid linking errors.

The rationale between adding this feature is to allow experimentation
with libFuzzer which currently relies on "Sanitizer Coverage"
instrumentation.

CC: @AnnaZaks @devincoughlin

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
OS X platform	@swift-ci Please test OS X platform
Linux platform	@swift-ci Please test Linux platform

Note: Only members of the Apple organization can trigger swift-ci.

delcypher · 2016-05-14T03:23:51Z

@jrose-apple I suspect you will have opinions on this PR. Please take a look when you have time.

jrose-apple · 2016-05-18T16:15:55Z

lib/IRGen/IRGen.cpp

+                                     legacy::PassManagerBase &PM) {
+  SanitizerCoverageOptions opts;
+  opts.CoverageType = sanitizerCoverageType;
+  assert(opts.CoverageType != SanitizerCoverageOptions::SCK_None);


You should be able to make this a static_assert, since it's a template parameter.

jrose-apple · 2016-05-18T16:30:29Z

This looks really good, Dan. I peppered it with some minor comments but I don't see any real issues.

delcypher · 2016-05-18T18:13:27Z

@jrose-apple Thank you for the review. It turns out I need to extend the -sanitize-coverage= flag to support a few more options (namely trace-pc, `trace-cmpandindirect-calls``) to match what clang supports. So I think I'll try to fix your issues first and then adds the extra flags I need. These will be all separate commits. Once you're happy with them I can squash everything into a single commit to be merged.

jrose-apple · 2016-05-18T19:53:11Z

Sounds good, thanks!

delcypher · 2016-05-21T05:55:05Z

@jrose-apple You can probably take another look now.

Something I'm not very happy about is the introduction of the swift::SanitizerCoverageOpts struct which is basically the same as llvm::SanitizerCoverageOptions. It only exists to avoid doing #include "llvm/Transforms/Instrumentation.h" in IRGenOptions.h. In that header file it looks like there aren't any LLVM includes other than what's in the Support library and I don't want to introduce new includes.

AnnaZaks · 2016-05-22T06:37:33Z

Something I'm not very happy about is the introduction of the swift::SanitizerCoverageOpts struct

I am leaning toward including the definitions from LLVM rather than copying them over, especially, if what's defined in the LLVM header is identical to what you use. You could also consider splitting the LLVM header into 2. For example, one of them could contain only the definitions of the options.

delcypher · 2016-05-22T06:46:14Z

@AnnaZaks

I am leaning toward including the definitions from LLVM rather than copying them over, especially, if what's defined in the LLVM header is identical to what you use.

I should probably go for that then unless @jrose-apple has objections.

You could also consider splitting the LLVM header into 2. For example, one of them could contain only the definitions of the options.

Unfortunately I don't think that is practical. The change would need to be made upstream (I don't want this to be a swift only change to LLVM) which might get some push back and take a while to get accepted upstream which would delay getting this PR merged.

jrose-apple · 2016-05-23T16:37:31Z

include/swift/AST/DiagnosticsFrontend.def

+
+ERROR(error_option_missing_required_argument, none,
+  "option '%0' is missing a required argument. The required "
+  "argument %1", (StringRef, StringRef))


:-/ I'm still unhappy with one of the diagnostic arguments being a whole phrase. How about "option '%0' is missing a required argument (%1)"? That way there's less of a grammatical restriction, and it's also more concise.

SGTM. I will make this change.

jrose-apple · 2016-05-23T16:59:32Z

I'm torn about the LLVM header. That's not (transitively) a small header, but Anna's right that the duplication isn't worth it. Let's include it for now, but file a bug to split it up upstream as suggested.

delcypher · 2016-05-23T18:02:34Z

Okay. I've addressed @jrose-apple comments (apart from the LLVM header issue. I'm waiting on @rjmccall to give me the okay on that). Looks like this PR won't merge cleanly anymore so I'm going to rebase now and squash everything into a single commit.

AnnaZaks · 2016-05-23T19:01:41Z

For sanitizer tests that crash, please use the following option. This will ensure they do not crash, which is bad because it will generate a crash report.
env ASAN_OPTIONS=abort_on_error=0

delcypher · 2016-05-23T22:22:34Z

@AnnaZaks

For sanitizer tests that crash, please use the following option. This will ensure they do not crash, which is bad because it will generate a crash report.
env ASAN_OPTIONS=abort_on_error=0

Is that necessary here? The single test that use the sanitizer shouldn't be doing anything bad with addresses. The test/Sanitizers/sanitizer_coverage.swift is only using ASan because we need bits of the ASan compiler-rt linked in for the Sanitizer coverage to be linked in.

delcypher · 2016-05-23T22:28:09Z

@jrose-apple @AnnaZaks This PR has been rebased, modified to use the LLVM header and squashed into a single commit. When you have time please take a look.

AnnaZaks · 2016-05-23T22:55:52Z

You don't have to add that if the test is not reporting any issues. Though, we might want to have it for consistency.

jrose-apple · 2016-05-23T23:41:10Z

include/swift/AST/IRGenOptions.h

@@ -91,6 +94,9 @@ class IRGenOptions {
  /// Which sanitizer is turned on.
  SanitizerKind Sanitize : 2;

+  /// Which sanitizer coverage is turned on.
+  llvm::SanitizerCoverageOptions SanitizeCoverage;


Oops, nitpick: please move this out of the bitfields, so it doesn't stop them from getting merged.

delcypher · 2016-05-24T05:48:07Z

@AnnaZaks

You don't have to add that if the test is not reporting any issues. Though, we might want to have it for consistency.

Oh looks like you've added a test/Sanitizers directory since I last looked. I'm glad we picked the same name :). I've set the abort_on_error=0 ASAN_OPTIONS option although I didn't use env. Is that necessary. It doesn't seem to be on my local machine.

delcypher · 2016-05-24T05:48:33Z

@jrose-apple Sorry for the issues. I've tried to fix them in the latest version of the commit.

AnnaZaks · 2016-05-24T16:08:06Z

Oh looks like you've added a test/Sanitizers directory since I last looked. I'm glad we picked the same name :).

I've checked which directory you use in this patch before adding mine:)

I've set the abort_on_error=0 ASAN_OPTIONS option although I didn't use env. Is that necessary. It doesn't seem to be on my local machine.

I call %target-run %t_tsan-binary, which does not work without using 'env'.

jrose-apple · 2016-05-24T16:39:19Z

Looks good on my end. Thanks for bearing with my feedback!

delcypher · 2016-05-24T18:47:52Z

@jrose-apple Great. Is there anything left for me to do (e.g. triggering test runs)?

AnnaZaks · 2016-05-24T21:02:13Z

@swift-ci test

delcypher · 2016-05-24T21:37:54Z

@AnnaZaks I don't think the Linux build failure is to do with me. It looks like its in lldb.

AnnaZaks · 2016-05-25T02:18:42Z

@swift-ci please test and merge

AnnaZaks · 2016-05-27T01:02:45Z

@swift-ci please test and merge

jrose-apple · 2016-05-27T02:20:20Z

<unknown>:0: error: unsupported option '-sanitize=address' for target 'x86_64-unknown-linux-gnu'

Failing Tests (2):
    Swift :: Driver/sanitize_coverage.swift
    Swift :: IRGen/sanitize_coverage.swift

That looks like a real issue. Are these tests supposed to have REQUIRES guards?

jrose-apple · 2016-05-27T02:21:08Z

REQUIRES: asan_runtime, right? Although I wouldn't expect the Driver test to need it…

jrose-apple · 2016-05-27T02:22:07Z

test/IRGen/sanitize_coverage.swift

+// RUN: %target-swift-frontend -emit-ir -sanitize=address -sanitize-coverage=edge,indirect-calls %s | FileCheck %s -check-prefix=SANCOV -check-prefix=SANCOV_INDIRECT_CALLS
+// RUN: %target-swift-frontend -emit-ir -sanitize=address -sanitize-coverage=edge,8bit-counters %s | FileCheck %s -check-prefix=SANCOV -check-prefix=SANCOV_8BIT_COUNTERS
+
+import Foundation


Now that I think of it, please import Darwin instead of Foundation, to make it easier to port this test to Linux later.

Okay. I'm not sure how that makes porting easier but I'm not familiar enough with these modules to know any better.

It just means we won't accidentally start introducing dependencies on ObjC and/or Foundation.

delcypher · 2016-05-27T17:06:42Z

@jrose-apple

That looks like a real issue. Are these tests supposed to have REQUIRES guards?

Yes they should. I completely forgot about this. I can probably use REQUIRES: asan_runtime although for the driver test this a little misleading because it isn't really that the asan_runtime is required it is asan support in the frontend. Currently the driver will error out if the host is Linux and -sanitize=(address|thread). Should I just go ahead and use REQUIRES: asan_runtime or should I use XFAIL: linux?

AnnaZaks · 2016-05-27T17:11:14Z

You need XFAIL: linux. I suspect you do not need REQUIRES: asan_runtime for the driver tests. The failure above will be addressed by the XFAIL. The REQUIRES will make sure that the engineers that do not check out compiler RT will not run into test failures.

"Sanitizer Coverage" with a new flag ``-sanitize-coverage=``. This flag is analogous to Clang's ``-fsanitize-coverage=``. This instrumentation currently requires ASan or TSan to be enabled because the module pass created by ``createSanitizerCoverageModulePass()`` inserts calls into functions found in compiler-rt's "sanitizer_common". "sanitizer_common" is not shipped as an individual library but instead exists in several of the sanitizer runtime libraries so we have to link with one of them to avoid linking errors. The rationale between adding this feature is to allow experimentation with libFuzzer which currently relies on "Sanitizer Coverage" instrumentation.

delcypher · 2016-05-27T20:36:37Z

@AnnaZaks Okay I've made the changes. Could you re-run the tests?

AnnaZaks · 2016-05-27T20:39:54Z

@swift-ci please test and merge

delcypher · 2016-05-28T00:36:01Z

@jrose-apple @AnnaZaks
I don't think this build failure is related to this PR.

jrose-apple · 2016-05-28T00:51:06Z

Yeah, we're seeing this on master too. I think it's okay to merge.

delcypher · 2016-05-28T01:19:58Z

@jrose-apple Okay I'm merging then. Fingers crossed...

jrose-apple reviewed May 18, 2016
View reviewed changes

delcypher force-pushed the sanitizer_coverage branch from 2833a04 to 1f4d5dc Compare May 21, 2016 05:51

delcypher force-pushed the sanitizer_coverage branch from 1f4d5dc to a66ffc9 Compare May 21, 2016 05:59

jrose-apple reviewed May 23, 2016
View reviewed changes

delcypher force-pushed the sanitizer_coverage branch 2 times, most recently from 43e4439 to 8647614 Compare May 23, 2016 22:20

delcypher force-pushed the sanitizer_coverage branch from 8647614 to 601c31c Compare May 23, 2016 22:24

jrose-apple reviewed May 23, 2016
View reviewed changes

delcypher force-pushed the sanitizer_coverage branch 3 times, most recently from 35b1613 to bce30a0 Compare May 24, 2016 05:45

jrose-apple reviewed May 27, 2016
View reviewed changes

delcypher force-pushed the sanitizer_coverage branch from bce30a0 to 827f573 Compare May 27, 2016 20:35

delcypher merged commit 83da10b into swiftlang:master May 28, 2016

Teach the Swift front-end to generate code with sanitizer coverage #2527

Teach the Swift front-end to generate code with sanitizer coverage #2527

Uh oh!

Conversation

delcypher commented May 14, 2016

What's in this pull request?

Uh oh!

delcypher commented May 14, 2016

Uh oh!

jrose-apple May 18, 2016

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented May 18, 2016

Uh oh!

delcypher commented May 18, 2016

Uh oh!

jrose-apple commented May 18, 2016

Uh oh!

delcypher commented May 21, 2016

Uh oh!

AnnaZaks commented May 22, 2016

Uh oh!

delcypher commented May 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple May 23, 2016

Choose a reason for hiding this comment

Uh oh!

delcypher May 23, 2016

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented May 23, 2016

Uh oh!

delcypher commented May 23, 2016

Uh oh!

AnnaZaks commented May 23, 2016

Uh oh!

delcypher commented May 23, 2016

Uh oh!

delcypher commented May 23, 2016

Uh oh!

AnnaZaks commented May 23, 2016

Uh oh!

jrose-apple May 23, 2016

Choose a reason for hiding this comment

Uh oh!

delcypher commented May 24, 2016

Uh oh!

delcypher commented May 24, 2016

Uh oh!

AnnaZaks commented May 24, 2016

Uh oh!

jrose-apple commented May 24, 2016

Uh oh!

delcypher commented May 24, 2016

Uh oh!

AnnaZaks commented May 24, 2016

Uh oh!

delcypher commented May 24, 2016

Uh oh!

AnnaZaks commented May 25, 2016

Uh oh!

AnnaZaks commented May 27, 2016

Uh oh!

jrose-apple commented May 27, 2016

Uh oh!

jrose-apple commented May 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple May 27, 2016

Choose a reason for hiding this comment

Uh oh!

delcypher May 27, 2016

Choose a reason for hiding this comment

Uh oh!

jrose-apple May 27, 2016

Choose a reason for hiding this comment

Uh oh!

delcypher commented May 27, 2016

Uh oh!

delcypher commented May 22, 2016 •

edited

Loading

jrose-apple commented May 27, 2016 •

edited

Loading