Add Undefined Behavior sanitizer to Swift Driver #18553

rahul-malik · 2018-08-07T22:17:48Z

What's in this pull request?

This PR adds the ability to use the undefined behavior sanitizer. It accomplishes this by adding --sanitize=undefined to the Swift driver and linking the runtime library (if available) when it is specified.

This change is a dependency of adding this feature to SPM as discussed in this thread

@jrose-apple @cheshire

jrose-apple · 2018-08-07T23:40:55Z

test/Driver/sanitizers.swift

+
+// RUN: %swiftc_driver -resource-dir %S/Inputs/fake-resource-dir/lib/swift/ -driver-print-jobs -sanitize=undefined -target x86_64-apple-macosx10.9 %s | %FileCheck -check-prefix=UBSAN -check-prefix=UBSAN_OSX %s
+// RUN: not %swiftc_driver -resource-dir %S/Inputs/fake-resource-dir/lib/swift/ -driver-print-jobs -sanitize=undefined -target x86-apple-macosx10.9 %s 2>&1 | %FileCheck -check-prefix=UBSAN_OSX_32 %s
+// RUN: not %swiftc_driver -resource-dir %S/Inputs/fake-resource-dir/lib/swift/ -driver-print-jobs -sanitize=undefined -target x86_64-apple-ios7.1 %s 2>&1 | %FileCheck -check-prefix=UBSAN_IOSSIM %s


Hm, this doesn't seem right. I'm pretty sure we support UBSan on all Apple platforms. @cheshire?

I think you’re right - I was largely basing these off the existing tsan tests and didn’t see the “not” prefix

Also one thing I’ve been trying to find an answer for is if ubsan is supported on Windows. If so I’ll update the windows toolchain as well

Looks like not yet. Even if we're wrong about that, we can always add it later.

Ok I've omitted it for now. Updated the tests, let me know if this looks right

rahul-malik · 2018-08-08T02:55:10Z

test/Driver/sanitizers.swift

+// RUN: %swiftc_driver -resource-dir %S/Inputs/fake-resource-dir/lib/swift/ -driver-print-jobs -sanitize=undefined -target arm64-apple-tvos9.0 %s 2>&1 | %FileCheck -check-prefix=UBSAN_tvOS %s
+// RUN: %swiftc_driver -resource-dir %S/Inputs/fake-resource-dir/lib/swift/ -driver-print-jobs -sanitize=undefined -target i386-apple-watchos2.0 %s 2>&1 | %FileCheck -check-prefix=UBSAN_watchOS_SIM %s
+// RUN: %swiftc_driver -resource-dir %S/Inputs/fake-resource-dir/lib/swift/ -driver-print-jobs -sanitize=undefined -target armv7k-apple-watchos2.0 %s 2>&1  | %FileCheck -check-prefix=UBSAN_watchOS %s
+// RUN: not %swiftc_driver -resource-dir %S/Inputs/fake-resource-dir/lib/swift/ -driver-print-jobs -sanitize=undefined -target x86_64-unknown-windows-msvc %s 2>&1 | %FileCheck -check-prefix=UBSAN_WINDOWS %s


TODO: Confirm windows doesn't support ubsan

I think UBSan is supported on Windows - Clang 5.0.1 installed clang_rt.ubsan_standalone-x86_64.lib and variants on my Windows installation.

jrose-apple · 2018-08-08T15:29:49Z

test/Driver/sanitizers.swift

+ */
+
+// RUN: %swiftc_driver -resource-dir %S/Inputs/fake-resource-dir/lib/swift/ -driver-print-jobs -sanitize=undefined -target x86_64-apple-macosx10.9 %s | %FileCheck -check-prefix=UBSAN -check-prefix=UBSAN_OSX %s
+// RUN: %swiftc_driver -resource-dir %S/Inputs/fake-resource-dir/lib/swift/ -driver-print-jobs -sanitize=undefined -target x86-apple-macosx10.9 %s 2>&1 | %FileCheck -check-prefix=UBSAN_OSX_32 %s


The positive checks should be checking both -check-prefix=UBSAN and -check-prefix=UBSAN_<OS>.

Fixing! Is there a reason the Address Sanitizer Linux / Windows positive checks don't specify -check-prefix=ASAN ?

I suspect it was just a copy/paste mistake, like yours were.

Ah, wait, no, the rpath command is Apple-platform-specific. I guess it's okay to leave those as they were.

Makes sense, I'll address and run the test suite locally before updating

rahul-malik · 2018-08-08T19:19:21Z

test/Driver/sanitizers.swift

+// UBSAN_watchOS_SIM: lib/swift/clang/lib/darwin/libclang_rt.ubsan_watchossim_dynamic.dylib
+// UBSAN_watchOS: lib/swift/clang/lib/darwin/libclang_rt.ubsan_watchos_dynamic.dylib
+// UBSAN_LINUX: lib/swift/clang/lib/linux/libclang_rt.ubsan-x86_64.a
+// UBSAN_WINDOWS: lib/swift/clang/lib/windows/clang_rt.ubsan_standalone-x86_64.lib


This lib name is based off @troughton earlier comment but I believe the logic in the windows driver will look for clang_rt.ubsan-x86_64.lib so this might require an update

Ah, you should be able to run the tests locally to find out! docs/Testing.md has information on how to run just the one test.

rahul-malik · 2018-08-08T19:19:50Z

@jrose-apple @troughton - Thanks for the comments, updated!

jrose-apple · 2018-08-10T15:38:45Z

@swift-ci Please test

rahul-malik · 2018-08-10T17:45:57Z

@jrose-apple - I ran into the same test failure locally but it doesn't quite make sense to me why -sanitize=undefined is not found given that if i pass -sanitize=undefined to the swiftc executable built for testing it works fine.

The test error:

<unknown>:0: error: unsupported option '-sanitize=undefined' for target 'x86_64-apple-macosx10.9'

This error occurs if you pass an unsupported value for -sanitize or if we are unable to locate the dylib. I'm assuming this error is the latter but I am unsure how to debug this further without knowing the actual folder hierarchy and contents in the CI environment.

jrose-apple · 2018-08-10T18:27:54Z

Maybe we just don't build the UBSan support by default. @cheshire, @Rostepher?

cheshire · 2018-08-13T21:49:38Z

@rahul-malik Thanks! The source looks fine.
If the error is reproducible locally for you, could you try to build a debug version of the swift compiler, and use debugger to figure out what goes wrong?
IIRC the ubsan should be build by default.

The driver uses a somewhat convoluted logic to mark the sanitizer as unavailable if the file for it is not found in resource directory. Could you try putting a breakpoint there and seeing what happens?
If the file is not found, is the path wrong? In my build directory, I have a file "lib/clang/6.0.0/lib/darwin/libclang_rt.ubsan_osx.a" so it seems it should be found.

rahul-malik · 2018-08-14T00:43:36Z

@cheshire - Here is the current local state. I'll keep debugging but just wanted to provide more information.

~/dev/swift-sources/build/Ninja-DebugAssert/llvm-macosx-x86_64/lib/clang/6.0.0
❯ ls lib/darwin/ | grep ubsan
libclang_rt.ubsan_minimal_osx.a
libclang_rt.ubsan_minimal_osx_dynamic.dylib
libclang_rt.ubsan_osx.a
libclang_rt.ubsan_osx_dynamic.dylib

The dylib is referenced from this method in DarwinToolchain.cpp

std::string toolchains::Darwin::sanitizerRuntimeLibName(StringRef Sanitizer,
                                                        bool shared) const {
  return (Twine("libclang_rt.") + Sanitizer + "_" +
          getDarwinLibraryNameSuffixForTriple(this->getTriple()) +
          (shared ? "_dynamic.dylib" : ".a"))
      .str();
}

Given that I would expect a lookup of libclang_rt.ubsan_osx_dynamic.dylib which should make it available.

cheshire · 2018-08-14T02:01:32Z

@rahul-malik yeah it's weird. What if you change the lookup to use the archive file, does everything work then?

rahul-malik · 2018-08-17T02:36:37Z

@cheshire - Thanks! Haven't been able to get back to this for a few days but will look into your suggestion tonight.

rahul-malik · 2018-08-21T17:21:32Z

@cheshire - Changing to the static lib (.a) didn't fix the test for me locally. Any chance you can patch and help me investigate?

cheshire · 2018-10-08T18:59:47Z

@rahul-malik Sorry, I am still somewhat confused on what exactly the problem is and to which degree is it isolated.

Is it correct to say that the test is failing, and you could not quite understand why?
If yes, then it could be a matter of just adding an empty (stub) file to test/Driver/Inputs/fake-resource-dir/lib/swift/clang/lib, alongside many others (e.g. check out test/Driver/Inputs/fake-resource-dir/lib/swift/clang/lib/darwin).
During the test, the flag -resource-dir %S/Inputs/fake-resource-dir/lib/swift/ is passed to the driver, and that's why the look up is performed in that folder.

cheshire

Seems you need to add empty stub files for ubsan into test/Driver/Inputs/fake-resource-dir

rahul-malik · 2018-10-08T20:51:54Z

Thank you! Yes I was unclear how to go about resolving the missing file stub issue. I’ll update the PR next week with this change

cheshire · 2018-10-08T20:53:44Z

@rahul-malik NP! In general, I find that such issues are best resolved with debuggers: bisection narrows it down to the file not being found, and then debugger would let you know that it searches in a stub location.

This change allows the swift driver to link the ubsan runtime if `-sanitize=undefined` is specified.

- Update positive asan tests that don't contain `-check-prefix=ASAN` - Add ubsan to windows toolchain

rahul-malik · 2018-11-12T23:37:43Z

@cheshire @jrose-apple - Got around to updating with new inputs and rebasing the latest sources. Should be good to go 👍

cheshire · 2018-11-12T23:42:36Z

@swift-ci please test

swift-ci · 2018-11-13T00:38:05Z

Build failed
Swift Test Linux Platform
Git Sha - 1bdab0c

swift-ci · 2018-11-13T01:07:30Z

Build failed
Swift Test OS X Platform
Git Sha - 1bdab0c

cheshire · 2018-11-13T01:49:25Z

There are tests failing, but none of them are the result of your patch.

jrose-apple · 2018-11-13T01:58:39Z

I just merged Bruno's fix for this, so let's try again.

@swift-ci Please test

rahul-malik · 2018-11-13T19:14:58Z

@jrose-apple @cheshire - Yay! Tests passed :) Is this PR ready to accept / merge?

cheshire · 2018-11-13T19:24:06Z

OK, Thanks!

cheshire · 2018-11-13T19:25:02Z

Should I merge it, or do you want to do it? You would need to use the "squash and merge" option.

jrose-apple · 2018-11-13T20:10:15Z

Ah, external contributors don't have commit privileges by default, so you should be the one to merge it, George.

rahul-malik mentioned this pull request Aug 7, 2018

Add UBSan support for C family languages swiftlang/swift-package-manager#1733

Merged

jrose-apple reviewed Aug 7, 2018

View reviewed changes

rahul-malik force-pushed the rmalik-swift-driver-ubsan branch from 3cfd43b to 05575f6 Compare August 8, 2018 02:53

rahul-malik commented Aug 8, 2018

View reviewed changes

jrose-apple reviewed Aug 8, 2018

View reviewed changes

rahul-malik commented Aug 8, 2018

View reviewed changes

jrose-apple self-assigned this Aug 10, 2018

jrose-apple requested a review from cheshire August 10, 2018 15:39

swiftlang deleted a comment from swift-ci Aug 10, 2018

rahul-malik closed this Aug 14, 2018

rahul-malik reopened this Aug 14, 2018

cheshire suggested changes Oct 8, 2018

View reviewed changes

rahul-malik added 4 commits November 12, 2018 15:28

Add Undefined Behavior sanitizer to Swift Driver

ad18ff1

This change allows the swift driver to link the ubsan runtime if `-sanitize=undefined` is specified.

- Update positive tests for ubsan

9a466f9

- Update positive asan tests that don't contain `-check-prefix=ASAN` - Add ubsan to windows toolchain

Remove darwin only check from linux / windows test cases

9fcec16

Add test fake inputs, rebase latest swift master

27fa64f

rahul-malik force-pushed the rmalik-swift-driver-ubsan branch from c3e3ac1 to 27fa64f Compare November 12, 2018 23:33

Add remaining ubsan stubs

1bdab0c

cheshire approved these changes Nov 13, 2018

View reviewed changes

cheshire merged commit d3cc043 into swiftlang:master Nov 13, 2018

rahul-malik deleted the rmalik-swift-driver-ubsan branch November 13, 2018 21:23

Add Undefined Behavior sanitizer to Swift Driver #18553

Add Undefined Behavior sanitizer to Swift Driver #18553

Uh oh!

Conversation

rahul-malik commented Aug 7, 2018

What's in this pull request?

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!

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!

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!

rahul-malik commented Aug 8, 2018

Uh oh!

jrose-apple commented Aug 10, 2018

Uh oh!

rahul-malik commented Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Aug 10, 2018

Uh oh!

cheshire commented Aug 13, 2018

Uh oh!

rahul-malik commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cheshire commented Aug 14, 2018

Uh oh!

rahul-malik commented Aug 17, 2018

Uh oh!

rahul-malik commented Aug 21, 2018

Uh oh!

cheshire commented Oct 8, 2018

Uh oh!

cheshire left a comment

Choose a reason for hiding this comment

Uh oh!

rahul-malik commented Oct 8, 2018

Uh oh!

cheshire commented Oct 8, 2018

Uh oh!

rahul-malik commented Nov 12, 2018

Uh oh!

cheshire commented Nov 12, 2018

Uh oh!

swift-ci commented Nov 13, 2018

Uh oh!

swift-ci commented Nov 13, 2018

Uh oh!

cheshire commented Nov 13, 2018

Uh oh!

jrose-apple commented Nov 13, 2018

Uh oh!

rahul-malik commented Nov 13, 2018

Uh oh!

cheshire commented Nov 13, 2018

Uh oh!

rahul-malik commented Aug 10, 2018 •

edited

Loading

rahul-malik commented Aug 14, 2018 •

edited

Loading