[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
rnk added a comment. I got some actual data. The way we package clang today, the extracted installation is 134.83M, and lib/clang/7.0.0/lib/darwin/* is 13M, so it's a 10% increase. It would be worth it to us to replace these with empty files if this change does land, but again, I'd rather not go this direction, which would require special logic just for the darwin parts of compiler-rt. Repository: rL LLVM https://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
rnk added a subscriber: beanz. rnk added a comment. In https://reviews.llvm.org/D15225#1191304, @george.karpenkov wrote: > @rnk As discussed, would it be acceptable for you to just have empty > sanitizer runtime files in the resource directory? I was talking to @beanz, and he suggested adding a cmake flag, something like CLANG_UNSUPPORTED_SANITIZERS=asan;ubsan;tsan;msan etc to control this. Alternatively, it could be a positive list of supported sanitizers, whatever is preferable. I think my main objection to this is that while it's convenient from a packaging perspective, it ascribes too much significance to the existence or non-existence of some library files that aren't needed during compilation in the first place. Changing the wording from "not supported" to "not available" doesn't seem that helpful. It would still lead someone down the path of needing to read the clang source code to understand that some library files are missing, whereas a link error would be more obvious. It's also easy to imagine scenarios where the user has a slightly non-standard link setup and the runtime library ultimately doesn't come from the resource directory. For example, users checking out compiler-rt and building these libraries themselves, perhaps with additional instrumentation. Overall, I feel like this is too tight coupling. Repository: rL LLVM https://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
george.karpenkov added a comment. @rnk As discussed, would it be acceptable for you to just have empty sanitizer runtime files in the resource directory? Repository: rL LLVM https://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
george.karpenkov added a comment. @rnk OK it's fine with me to revert it now and then we see what can be done. Repository: rL LLVM https://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
rnk added a comment. In https://reviews.llvm.org/D15225#1183318, @george.karpenkov wrote: > @rnk changing subjects: will you be at the LLVM social on Thursday? Could we > discuss it there? Yes, we can and should, but I'd like update chromium's copy of clang before then. Repository: rL LLVM https://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
george.karpenkov added a comment. @rnk changing subjects: will you be at the LLVM social on Thursday? Could we discuss it there? Repository: rL LLVM https://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
george.karpenkov added a comment. @rnk OK I see, we've accidentally posted comments nearly simultaneously. Repository: rL LLVM https://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
george.karpenkov added a comment. > it's just that the runtime library didn't happen to exist on the system > performing compilation But then conceptually, the compiler toolchain on the system performing compilation is not fully supporting asan, right? It works for producing asanified object files, but fails with a link error if you wanted to produce an asanified executable. Repository: rL LLVM https://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
rnk added a comment. In https://reviews.llvm.org/D15225#1183132, @george.karpenkov wrote: > @rnk could you clarify how did it break the distributed asanified build? If > the slave compiler supports asan it should have this runtime, right? Most open source distributed build systems wrap only compilation steps, not link steps. There's no reason to ship the runtime library just for compilation. In https://reviews.llvm.org/D15225#1183135, @george.karpenkov wrote: > @rnk Regarding the error message, would it be possible to compromise on > something like "sanitizer runtime not available"? > I understand that the exact error message would be very useful for you, but > it's just confusing for a user getting a toolchain with Xcode, since they > can't just add the required file into the toolchain. I think you are underestimating the likelihood that users will repackage and redistribute the C/C++ toolchain parts of the XCode as part of their normal build processes. I think in principle it's better for clang to remain self-contained, for it to just "know" which platforms have working sanitizer libraries. It feels like this change is trying to check whether the link will succeed during compilation, which is kind of putting the cart before the horse. Repository: rL LLVM https://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
george.karpenkov added a comment. @rnk Regarding the error message, would it be possible to compromise on something like "sanitizer runtime not available"? I understand that the exact error message would be very useful for you, but it's just confusing for a user getting a toolchain with Xcode, since they can't just add the required file into the toolchain. Repository: rL LLVM https://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
george.karpenkov added a comment. @rnk could you clarify how did it break the distributed asanified build? If the slave compiler supports asan it should have this runtime, right? Repository: rL LLVM https://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
rnk added subscribers: hans, thakis, samsonov, rnk. rnk added a comment. +@thakis @hans I think this broke Chromium's distributed Mac ASan build. I personally found the error message confusing for the reasons that @samsonov mentioned back in 2015. It's not that ASan wasn't supported on the platform, it's just that the runtime library didn't happen to exist on the system performing compilation. If this is going to be based on a filesystem test, the diagnostic should be more mechanical in nature, i.e. actually tell the user that some required file doesn't exist. In the meantime, I would like to revert this. I don't think our distributed build use case is that exotic, and I expect other users of distcc and icecreamcc will have similar issues with this change. Repository: rL LLVM https://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
This revision was automatically updated to reflect the committed changes.
Closed by commit rL337635: [Driver] Sanitizer support based on runtime library
presence (authored by george.karpenkov, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D15225?vs=156603=156639#toc
Repository:
rL LLVM
https://reviews.llvm.org/D15225
Files:
cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
cfe/trunk/lib/Driver/ToolChains/Darwin.h
cfe/trunk/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_ios_dynamic.dylib
cfe/trunk/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_iossim_dynamic.dylib
cfe/trunk/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_osx_dynamic.dylib
cfe/trunk/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_tvos_dynamic.dylib
cfe/trunk/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_tvossim_dynamic.dylib
cfe/trunk/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_watchos_dynamic.dylib
cfe/trunk/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_watchossim_dynamic.dylib
cfe/trunk/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.fuzzer_osx.a
cfe/trunk/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.lsan_ios_dynamic.dylib
cfe/trunk/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.lsan_iossim_dynamic.dylib
cfe/trunk/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.lsan_osx_dynamic.dylib
cfe/trunk/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.lsan_tvossim_dynamic.dylib
cfe/trunk/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.tsan_iossim_dynamic.dylib
cfe/trunk/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.tsan_osx_dynamic.dylib
cfe/trunk/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.tsan_tvossim_dynamic.dylib
cfe/trunk/test/Driver/darwin-asan-nofortify.c
cfe/trunk/test/Driver/darwin-sanitizer-ld.c
cfe/trunk/test/Driver/fsanitize.c
cfe/trunk/test/Driver/fuzzer.c
cfe/trunk/test/Driver/sanitizer-ld.c
Index: cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
@@ -916,13 +916,26 @@
return 4;
}
+SmallString<128> MachO::runtimeLibDir(bool IsEmbedded) const {
+ SmallString<128> Dir(getDriver().ResourceDir);
+ llvm::sys::path::append(
+ Dir, "lib", IsEmbedded ? "macho_embedded" : "darwin");
+ return Dir;
+}
+
+std::string Darwin::getFileNameForSanitizerLib(StringRef SanitizerName,
+ bool Shared) const {
+ return (Twine("libclang_rt.") + SanitizerName + "_" +
+ getOSLibraryNameSuffix() +
+ (Shared ? "_dynamic.dylib" : ".a")).str();
+
+}
+
void MachO::AddLinkRuntimeLib(const ArgList , ArgStringList ,
StringRef DarwinLibName,
RuntimeLinkOptions Opts) const {
- SmallString<128> Dir(getDriver().ResourceDir);
- llvm::sys::path::append(
- Dir, "lib", (Opts & RLO_IsEmbedded) ? "macho_embedded" : "darwin");
+ SmallString<128> Dir = runtimeLibDir(Opts & RLO_IsEmbedded);
SmallString<128> P(Dir);
llvm::sys::path::append(P, DarwinLibName);
@@ -1042,12 +1055,9 @@
StringRef Sanitizer,
bool Shared) const {
auto RLO = RuntimeLinkOptions(RLO_AlwaysLink | (Shared ? RLO_AddRPath : 0U));
- AddLinkRuntimeLib(Args, CmdArgs,
-(Twine("libclang_rt.") + Sanitizer + "_" +
- getOSLibraryNameSuffix() +
- (Shared ? "_dynamic.dylib" : ".a"))
-.str(),
-RLO);
+ std::string SanitizerRelFilename =
+ getFileNameForSanitizerLib(Sanitizer, Shared);
+ AddLinkRuntimeLib(Args, CmdArgs, SanitizerRelFilename, RLO);
}
ToolChain::RuntimeLibType DarwinClang::GetRuntimeLibType(
@@ -2285,24 +2295,43 @@
SanitizerMask Darwin::getSupportedSanitizers() const {
const bool IsX86_64 = getTriple().getArch() == llvm::Triple::x86_64;
SanitizerMask Res = ToolChain::getSupportedSanitizers();
- Res |= SanitizerKind::Address;
- Res |= SanitizerKind::Leak;
- Res |= SanitizerKind::Fuzzer;
- Res |= SanitizerKind::FuzzerNoLink;
+
+ {
+using namespace SanitizerKind;
+assert(!(Res & (Address | Leak | Fuzzer | FuzzerNoLink | Thread)) &&
+ "Sanitizer is already registered as supported");
+ }
+
+ if (sanitizerRuntimeExists("asan"))
+Res |= SanitizerKind::Address;
+ if (sanitizerRuntimeExists("lsan"))
+Res |= SanitizerKind::Leak;
+ if (sanitizerRuntimeExists("fuzzer", /*Shared=*/false)) {
+Res |= SanitizerKind::Fuzzer;
+Res |= SanitizerKind::FuzzerNoLink;
+ }
Res |= SanitizerKind::Function;
- if (isTargetMacOS()) {
-if (!isMacosxVersionLT(10, 9))
- Res |= SanitizerKind::Vptr;
+ if
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
delcypher added a comment. @george.karpenkov Other than the comment that probably needs updating, LGTM. https://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
delcypher added inline comments.
Comment at: clang/lib/Driver/ToolChains/Darwin.h:425
+ /// \return Relative path to the filename for the library
+ /// containing the sanitizer {@code SanitizerName}.
You might want to update that comment to not mention "Relative path"
https://reviews.llvm.org/D15225
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
george.karpenkov updated this revision to Diff 156603.
https://reviews.llvm.org/D15225
Files:
clang/lib/Driver/ToolChains/Darwin.cpp
clang/lib/Driver/ToolChains/Darwin.h
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_ios_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_iossim_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_osx_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_tvos_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_tvossim_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_watchos_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_watchossim_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.fuzzer_osx.a
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.lsan_ios_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.lsan_iossim_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.lsan_osx_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.lsan_tvossim_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.tsan_iossim_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.tsan_osx_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.tsan_tvossim_dynamic.dylib
clang/test/Driver/darwin-asan-nofortify.c
clang/test/Driver/darwin-sanitizer-ld.c
clang/test/Driver/fsanitize.c
clang/test/Driver/fuzzer.c
clang/test/Driver/sanitizer-ld.c
Index: clang/test/Driver/sanitizer-ld.c
===
--- clang/test/Driver/sanitizer-ld.c
+++ clang/test/Driver/sanitizer-ld.c
@@ -530,6 +530,7 @@
// RUN: %clangxx -fsanitize=address %s -### -o %t.o 2>&1 \
// RUN: -mmacosx-version-min=10.6 \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
// RUN: -target x86_64-apple-darwin13.4.0 -fuse-ld=ld -stdlib=platform \
// RUN: --sysroot=%S/Inputs/basic_linux_tree \
// RUN: | FileCheck --check-prefix=CHECK-ASAN-DARWIN106-CXX %s
@@ -539,6 +540,7 @@
// RUN: %clangxx -fsanitize=leak %s -### -o %t.o 2>&1 \
// RUN: -mmacosx-version-min=10.6 \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
// RUN: -target x86_64-apple-darwin13.4.0 -fuse-ld=ld -stdlib=platform \
// RUN: --sysroot=%S/Inputs/basic_linux_tree \
// RUN: | FileCheck --check-prefix=CHECK-LSAN-DARWIN106-CXX %s
@@ -598,6 +600,7 @@
// RUN: %clang -fsanitize=cfi -fsanitize-stats %s -### -o %t.o 2>&1 \
// RUN: -target x86_64-apple-darwin -fuse-ld=ld \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
// RUN: --sysroot=%S/Inputs/basic_linux_tree \
// RUN: | FileCheck --check-prefix=CHECK-CFI-STATS-DARWIN %s
// CHECK-CFI-STATS-DARWIN: "{{.*}}ld{{(.exe)?}}"
Index: clang/test/Driver/fuzzer.c
===
--- clang/test/Driver/fuzzer.c
+++ clang/test/Driver/fuzzer.c
@@ -1,28 +1,28 @@
// Test flags inserted by -fsanitize=fuzzer.
-// RUN: %clang -fsanitize=fuzzer %s -target x86_64-apple-darwin14 -### 2>&1 | FileCheck --check-prefixes=CHECK-FUZZER-LIB,CHECK-COVERAGE-FLAGS %s
+// RUN: %clang -fsanitize=fuzzer %s -target x86_64-apple-darwin14 -resource-dir %S/Inputs/resource_dir -### 2>&1 | FileCheck --check-prefixes=CHECK-FUZZER-LIB,CHECK-COVERAGE-FLAGS %s
//
// CHECK-FUZZER-LIB: libclang_rt.fuzzer
// CHECK-COVERAGE: -fsanitize-coverage-inline-8bit-counters
// CHECK-COVERAGE-SAME: -fsanitize-coverage-indirect-calls
// CHECK-COVERAGE-SAME: -fsanitize-coverage-trace-cmp
// CHECK-COVERAGE-SAME: -fsanitize-coverage-pc-table
-// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=platform %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBCXX-LINUX %s
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=platform -resource-dir %S/Inputs/resource_dir %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBCXX-LINUX %s
//
// CHECK-LIBCXX-LINUX: -lstdc++
-// RUN: %clang -target x86_64-apple-darwin14 -fsanitize=fuzzer %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBCXX-DARWIN %s
+// RUN: %clang -target x86_64-apple-darwin14 -fsanitize=fuzzer -resource-dir %S/Inputs/resource_dir %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBCXX-DARWIN %s
//
// CHECK-LIBCXX-DARWIN: -lc++
// Check that we don't link in libFuzzer.a when producing a shared object.
-// RUN: %clang -fsanitize=fuzzer %s -shared -o %t.so -### 2>&1 | FileCheck --check-prefixes=CHECK-NOLIB-SO %s
+// RUN: %clang -fsanitize=fuzzer %s -shared -o %t.so -resource-dir %S/Inputs/resource_dir -### 2>&1 | FileCheck --check-prefixes=CHECK-NOLIB-SO %s
// CHECK-NOLIB-SO-NOT: libclang_rt.libfuzzer
// Check that we don't link in libFuzzer when compiling with -fsanitize=fuzzer-no-link.
-// RUN: %clang
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
delcypher added inline comments.
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2298
SanitizerMask Res = ToolChain::getSupportedSanitizers();
- Res |= SanitizerKind::Address;
- Res |= SanitizerKind::Leak;
- Res |= SanitizerKind::Fuzzer;
- Res |= SanitizerKind::FuzzerNoLink;
+
+ if (sanitizerRuntimeExists("asan"))
george.karpenkov wrote:
> delcypher wrote:
> > I feel that we should assert that `Res` doesn't already contain the
> > SanitizerKind we are decided whether or not to set.
> > E.g.
> >
> > ```
> > assert(!(Res & SanitizerKind::Address));
> > if (sanitizerRuntimeExists("asan")) {
> > Res |= SanitizerKind::Address;
> > }
> > ```
> I'm not sure it would be worth crashing the driver.
Asserts are usually off in shipping compilers. If an assumption is being
violated, as a developer it's better than you know about it during debugging.
Comment at: clang/lib/Driver/ToolChains/Darwin.h:427
+ /// containing the sanitizer {@code SanitizerName}.
+ std::string sanitizerToRelativeLibName(StringRef SanitizerName,
+ bool Shared = true) const;
george.karpenkov wrote:
> delcypher wrote:
> > I don't like this name very much. Given that the path is relative to the
> > directory containing the library, what this function really does is given
> > the **file name** for a sanitizer library. Mentioning "relative" is just
> > confusing.
> >
> > Wouldn't something like `getSanitizerLibName()` or
> > `getNameForSanitizerLib()` be much clearer?
> Then it's not clear whether the returned path is relative or absolute though.
The name implies that it's a file name and not a path. It could be
`getSanitizerLibFileName()` or `getFileNameForSanitizerLib()` but that seemed a
little verbose.
https://reviews.llvm.org/D15225
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
george.karpenkov added a comment.
@delcypher sorry i don't agree with the change requests.
@mracek any comments?
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2298
SanitizerMask Res = ToolChain::getSupportedSanitizers();
- Res |= SanitizerKind::Address;
- Res |= SanitizerKind::Leak;
- Res |= SanitizerKind::Fuzzer;
- Res |= SanitizerKind::FuzzerNoLink;
+
+ if (sanitizerRuntimeExists("asan"))
delcypher wrote:
> I feel that we should assert that `Res` doesn't already contain the
> SanitizerKind we are decided whether or not to set.
> E.g.
>
> ```
> assert(!(Res & SanitizerKind::Address));
> if (sanitizerRuntimeExists("asan")) {
> Res |= SanitizerKind::Address;
> }
> ```
I'm not sure it would be worth crashing the driver.
Comment at: clang/lib/Driver/ToolChains/Darwin.h:134
+ /// \return Directory to find the runtime library in.
+ SmallString<128> runtimeLibDir(bool IsEmbedded=false) const;
+
delcypher wrote:
> Why is this a `SmallString<128>` but in other places we're just using
> `std::string`?
For driver it does not matter which datastructure to use.
Here SmallString is used because that's what is more convenient to return.
Comment at: clang/lib/Driver/ToolChains/Darwin.h:427
+ /// containing the sanitizer {@code SanitizerName}.
+ std::string sanitizerToRelativeLibName(StringRef SanitizerName,
+ bool Shared = true) const;
delcypher wrote:
> I don't like this name very much. Given that the path is relative to the
> directory containing the library, what this function really does is given the
> **file name** for a sanitizer library. Mentioning "relative" is just
> confusing.
>
> Wouldn't something like `getSanitizerLibName()` or `getNameForSanitizerLib()`
> be much clearer?
Then it's not clear whether the returned path is relative or absolute though.
Comment at: clang/test/Driver/darwin-asan-nofortify.c:3
-// RUN: %clang -fsanitize=address %s -E -dM -target x86_64-darwin | FileCheck
%s
+// RUN: %clang -fsanitize=address %s -E -dM -target x86_64-darwin
-resource-dir %S/Inputs/resource_dir | FileCheck %s
delcypher wrote:
> Are these `-resource-dir %S/Inputs/resource_dir` needed because compiler-rt
> might not be present and now the driver checks for these?
By default -resource-dir points to the resource dir in the "bin" directory. The
contents of that is sort of random, and depends on what ninja commands have you
ran before.
https://reviews.llvm.org/D15225
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
delcypher added inline comments.
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2298
SanitizerMask Res = ToolChain::getSupportedSanitizers();
- Res |= SanitizerKind::Address;
- Res |= SanitizerKind::Leak;
- Res |= SanitizerKind::Fuzzer;
- Res |= SanitizerKind::FuzzerNoLink;
+
+ if (sanitizerRuntimeExists("asan"))
I feel that we should assert that `Res` doesn't already contain the
SanitizerKind we are decided whether or not to set.
E.g.
```
assert(!(Res & SanitizerKind::Address));
if (sanitizerRuntimeExists("asan")) {
Res |= SanitizerKind::Address;
}
```
https://reviews.llvm.org/D15225
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
delcypher requested changes to this revision.
delcypher added a comment.
This revision now requires changes to proceed.
Seems mostly fine apart from some minor nits.
If I'm honest I don't see any reason why this should be Darwin specific. Sure
the naming convention and location of the runtime libraries is different for
other platforms but other than that the same logic used here is applicable.
I don't feel that strongly about it and we could refactor later if other
platforms need this if you really don't want to support this for other
platforms.
Comment at: clang/lib/Driver/ToolChains/Darwin.h:134
+ /// \return Directory to find the runtime library in.
+ SmallString<128> runtimeLibDir(bool IsEmbedded=false) const;
+
Why is this a `SmallString<128>` but in other places we're just using
`std::string`?
Comment at: clang/lib/Driver/ToolChains/Darwin.h:427
+ /// containing the sanitizer {@code SanitizerName}.
+ std::string sanitizerToRelativeLibName(StringRef SanitizerName,
+ bool Shared = true) const;
I don't like this name very much. Given that the path is relative to the
directory containing the library, what this function really does is given the
**file name** for a sanitizer library. Mentioning "relative" is just confusing.
Wouldn't something like `getSanitizerLibName()` or `getNameForSanitizerLib()`
be much clearer?
Comment at: clang/test/Driver/darwin-asan-nofortify.c:3
-// RUN: %clang -fsanitize=address %s -E -dM -target x86_64-darwin | FileCheck
%s
+// RUN: %clang -fsanitize=address %s -E -dM -target x86_64-darwin
-resource-dir %S/Inputs/resource_dir | FileCheck %s
Are these `-resource-dir %S/Inputs/resource_dir` needed because compiler-rt
might not be present and now the driver checks for these?
https://reviews.llvm.org/D15225
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
george.karpenkov added a comment. @delcypher Could you take a look? @kcc Any objections? https://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
kubamracek added a comment. This looks great to me, but someone else should review this as well. https://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
george.karpenkov updated this revision to Diff 155785.
george.karpenkov edited the summary of this revision.
george.karpenkov edited reviewers, added: delcypher; removed: bob.wilson,
glider, t.p.northover, samsonov, beanz.
george.karpenkov added a comment.
Attempt #2: reduced version of this patch, without ubsan support.
https://reviews.llvm.org/D15225
Files:
clang/lib/Driver/ToolChains/Darwin.cpp
clang/lib/Driver/ToolChains/Darwin.h
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_ios_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_iossim_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_osx_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_tvos_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_tvossim_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_watchos_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_watchossim_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.fuzzer_osx.a
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.lsan_ios_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.lsan_iossim_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.lsan_osx_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.lsan_tvossim_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.tsan_iossim_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.tsan_osx_dynamic.dylib
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.tsan_tvossim_dynamic.dylib
clang/test/Driver/darwin-asan-nofortify.c
clang/test/Driver/darwin-sanitizer-ld.c
clang/test/Driver/fsanitize.c
clang/test/Driver/fuzzer.c
clang/test/Driver/sanitizer-ld.c
Index: clang/test/Driver/sanitizer-ld.c
===
--- clang/test/Driver/sanitizer-ld.c
+++ clang/test/Driver/sanitizer-ld.c
@@ -530,6 +530,7 @@
// RUN: %clangxx -fsanitize=address %s -### -o %t.o 2>&1 \
// RUN: -mmacosx-version-min=10.6 \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
// RUN: -target x86_64-apple-darwin13.4.0 -fuse-ld=ld -stdlib=platform \
// RUN: --sysroot=%S/Inputs/basic_linux_tree \
// RUN: | FileCheck --check-prefix=CHECK-ASAN-DARWIN106-CXX %s
@@ -539,6 +540,7 @@
// RUN: %clangxx -fsanitize=leak %s -### -o %t.o 2>&1 \
// RUN: -mmacosx-version-min=10.6 \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
// RUN: -target x86_64-apple-darwin13.4.0 -fuse-ld=ld -stdlib=platform \
// RUN: --sysroot=%S/Inputs/basic_linux_tree \
// RUN: | FileCheck --check-prefix=CHECK-LSAN-DARWIN106-CXX %s
@@ -598,6 +600,7 @@
// RUN: %clang -fsanitize=cfi -fsanitize-stats %s -### -o %t.o 2>&1 \
// RUN: -target x86_64-apple-darwin -fuse-ld=ld \
+// RUN: -resource-dir=%S/Inputs/resource_dir \
// RUN: --sysroot=%S/Inputs/basic_linux_tree \
// RUN: | FileCheck --check-prefix=CHECK-CFI-STATS-DARWIN %s
// CHECK-CFI-STATS-DARWIN: "{{.*}}ld{{(.exe)?}}"
Index: clang/test/Driver/fuzzer.c
===
--- clang/test/Driver/fuzzer.c
+++ clang/test/Driver/fuzzer.c
@@ -1,28 +1,28 @@
// Test flags inserted by -fsanitize=fuzzer.
-// RUN: %clang -fsanitize=fuzzer %s -target x86_64-apple-darwin14 -### 2>&1 | FileCheck --check-prefixes=CHECK-FUZZER-LIB,CHECK-COVERAGE-FLAGS %s
+// RUN: %clang -fsanitize=fuzzer %s -target x86_64-apple-darwin14 -resource-dir %S/Inputs/resource_dir -### 2>&1 | FileCheck --check-prefixes=CHECK-FUZZER-LIB,CHECK-COVERAGE-FLAGS %s
//
// CHECK-FUZZER-LIB: libclang_rt.fuzzer
// CHECK-COVERAGE: -fsanitize-coverage-inline-8bit-counters
// CHECK-COVERAGE-SAME: -fsanitize-coverage-indirect-calls
// CHECK-COVERAGE-SAME: -fsanitize-coverage-trace-cmp
// CHECK-COVERAGE-SAME: -fsanitize-coverage-pc-table
-// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=platform %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBCXX-LINUX %s
+// RUN: %clang -fsanitize=fuzzer -target i386-unknown-linux -stdlib=platform -resource-dir %S/Inputs/resource_dir %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBCXX-LINUX %s
//
// CHECK-LIBCXX-LINUX: -lstdc++
-// RUN: %clang -target x86_64-apple-darwin14 -fsanitize=fuzzer %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBCXX-DARWIN %s
+// RUN: %clang -target x86_64-apple-darwin14 -fsanitize=fuzzer -resource-dir %S/Inputs/resource_dir %s -### 2>&1 | FileCheck --check-prefixes=CHECK-LIBCXX-DARWIN %s
//
// CHECK-LIBCXX-DARWIN: -lc++
// Check that we don't link in libFuzzer.a when producing a shared object.
-// RUN: %clang -fsanitize=fuzzer %s -shared -o %t.so -### 2>&1 | FileCheck --check-prefixes=CHECK-NOLIB-SO %s
+// RUN: %clang -fsanitize=fuzzer
Re: [PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
beanz requested changes to this revision. beanz added a comment. This revision now requires changes to proceed. It looks like @samsonov had quite a few valid review comments that are still unresolved. I know he hasn't been active lately, but I think those points deserve discussion. -Chris http://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
zaks.anna added a comment. LGTM. Any other comments to this? http://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
ygribov added a subscriber: ygribov. Comment at: test/Driver/fsanitize.c:221 @@ +220,3 @@ +// RUN: %clang -target x86_64-apple-darwin10 -resource-dir=%S/Inputs/resource_dir -fsanitize=memory -fsanitize=thread,memory %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-MSAN-TSAN-MSAN-DARWIN1 +// CHECK-MSAN-TSAN-MSAN-DARWIN1: unsupported option '-fsanitize=thread,memory' for target 'x86_64-apple-darwin10' +// CHECK-MSAN-TSAN-MSAN-DARWIN1-NOT: unsupported option samsonov wrote: > beanz wrote: > > kubabrecka wrote: > > > samsonov wrote: > > > > Again, I feel like we're lying to users here: `-fsanitize=thread` *is* > > > > supported for this target, it just requires building a runtime. > > > I'd like to see this from the point-of-view of a binary distribution. If > > > the binary distribution (e.g. the one from llvm.org or Apple's Clang in > > > Xcode) doesn't contain a runtime library, then the sanitizer is *not* > > > supported in that distribution. Also, see > > > http://reviews.llvm.org/D14846, we'd like to have CMake options to select > > > which runtimes will be built. If you deliberately choose not to build > > > ThreadSanitizer, then that sanitizer is *not* supported in your version > > > of Clang. If you're experimenting and porting a runtime to a new > > > platform, then this sanitizer *is* supported in your version of Clang. > > Maybe the point is we should have a different error message for cases where > > the runtime is just missing. Something like "runtime components for > > '-fsanitize=thread' not available" > I see, so essentially you want to use a different approach for determining > sanitizer availability (on OS X for now): if the library is present, then we > support sanitizer, otherwise we don't: i.e. the binary distribution is the > source of truth, not the list of sanitizers hardcoded into Clang driver > source code. I'm fine with that, and see why it would make sense. > > It's just that error message looks misleading: the problem is not TSan is > unsupported for target, it's just unavailable in this distribution for one > reason or another. > the binary distribution is the source of truth, not the list of sanitizers > hardcoded into Clang driver source code. This will not work for cross-compilers. It _may_ be ok for OSX but not for other platforms. http://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
kubabrecka added inline comments. Comment at: test/Driver/fsanitize.c:221 @@ +220,3 @@ +// RUN: %clang -target x86_64-apple-darwin10 -resource-dir=%S/Inputs/resource_dir -fsanitize=memory -fsanitize=thread,memory %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-MSAN-TSAN-MSAN-DARWIN1 +// CHECK-MSAN-TSAN-MSAN-DARWIN1: unsupported option '-fsanitize=thread,memory' for target 'x86_64-apple-darwin10' +// CHECK-MSAN-TSAN-MSAN-DARWIN1-NOT: unsupported option ygribov wrote: > samsonov wrote: > > beanz wrote: > > > kubabrecka wrote: > > > > samsonov wrote: > > > > > Again, I feel like we're lying to users here: `-fsanitize=thread` > > > > > *is* supported for this target, it just requires building a runtime. > > > > I'd like to see this from the point-of-view of a binary distribution. > > > > If the binary distribution (e.g. the one from llvm.org or Apple's Clang > > > > in Xcode) doesn't contain a runtime library, then the sanitizer is > > > > *not* supported in that distribution. Also, see > > > > http://reviews.llvm.org/D14846, we'd like to have CMake options to > > > > select which runtimes will be built. If you deliberately choose not to > > > > build ThreadSanitizer, then that sanitizer is *not* supported in your > > > > version of Clang. If you're experimenting and porting a runtime to a > > > > new platform, then this sanitizer *is* supported in your version of > > > > Clang. > > > Maybe the point is we should have a different error message for cases > > > where the runtime is just missing. Something like "runtime components for > > > '-fsanitize=thread' not available" > > I see, so essentially you want to use a different approach for determining > > sanitizer availability (on OS X for now): if the library is present, then > > we support sanitizer, otherwise we don't: i.e. the binary distribution is > > the source of truth, not the list of sanitizers hardcoded into Clang driver > > source code. I'm fine with that, and see why it would make sense. > > > > It's just that error message looks misleading: the problem is not TSan is > > unsupported for target, it's just unavailable in this distribution for one > > reason or another. > > the binary distribution is the source of truth, not the list of sanitizers > > hardcoded into Clang driver source code. > > This will not work for cross-compilers. It _may_ be ok for OSX but not for > other platforms. Why not? On Linux, there are statically-linked sanitizers, if you want to cross-compile, you need to have the runtime libraries for the target. And dynamic libraries are a similar story – they're version-tied to the compiler and your compiler should really have the libraries of the sanitizers that it supports. http://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
ygribov added inline comments. Comment at: test/Driver/fsanitize.c:221 @@ +220,3 @@ +// RUN: %clang -target x86_64-apple-darwin10 -resource-dir=%S/Inputs/resource_dir -fsanitize=memory -fsanitize=thread,memory %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-MSAN-TSAN-MSAN-DARWIN1 +// CHECK-MSAN-TSAN-MSAN-DARWIN1: unsupported option '-fsanitize=thread,memory' for target 'x86_64-apple-darwin10' +// CHECK-MSAN-TSAN-MSAN-DARWIN1-NOT: unsupported option kubabrecka wrote: > ygribov wrote: > > samsonov wrote: > > > beanz wrote: > > > > kubabrecka wrote: > > > > > samsonov wrote: > > > > > > Again, I feel like we're lying to users here: `-fsanitize=thread` > > > > > > *is* supported for this target, it just requires building a runtime. > > > > > I'd like to see this from the point-of-view of a binary distribution. > > > > > If the binary distribution (e.g. the one from llvm.org or Apple's > > > > > Clang in Xcode) doesn't contain a runtime library, then the sanitizer > > > > > is *not* supported in that distribution. Also, see > > > > > http://reviews.llvm.org/D14846, we'd like to have CMake options to > > > > > select which runtimes will be built. If you deliberately choose not > > > > > to build ThreadSanitizer, then that sanitizer is *not* supported in > > > > > your version of Clang. If you're experimenting and porting a runtime > > > > > to a new platform, then this sanitizer *is* supported in your version > > > > > of Clang. > > > > Maybe the point is we should have a different error message for cases > > > > where the runtime is just missing. Something like "runtime components > > > > for '-fsanitize=thread' not available" > > > I see, so essentially you want to use a different approach for > > > determining sanitizer availability (on OS X for now): if the library is > > > present, then we support sanitizer, otherwise we don't: i.e. the binary > > > distribution is the source of truth, not the list of sanitizers hardcoded > > > into Clang driver source code. I'm fine with that, and see why it would > > > make sense. > > > > > > It's just that error message looks misleading: the problem is not TSan is > > > unsupported for target, it's just unavailable in this distribution for > > > one reason or another. > > > the binary distribution is the source of truth, not the list of > > > sanitizers hardcoded into Clang driver source code. > > > > This will not work for cross-compilers. It _may_ be ok for OSX but not for > > other platforms. > Why not? On Linux, there are statically-linked sanitizers, if you want to > cross-compile, you need to have the runtime libraries for the target. And > dynamic libraries are a similar story – they're version-tied to the compiler > and your compiler should really have the libraries of the sanitizers that it > supports. Ah, for some strange reason I thought that you were searching runtime lib in sysroot rather than compiler root. http://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
zaks.anna added a comment. > I don't know, is there a way to install runtime components for ASan if your > distribution doesn't happen to have one (that must be tricky, as the version > of ASan should match the version of the compiler). Correct, there is no recommended way of installing the libraries on top of the existing toolchain. http://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
zaks.anna added a comment. > I see, so essentially you want to use a different approach for determining > sanitizer availability (on OS X for now): if the library is present, then we > support > sanitizer, otherwise we don't: i.e. the binary distribution is the source of > truth, not the list of sanitizers hardcoded into Clang driver source code. > I'm fine with > that, and see why it would make sense. Correct. > It's just that error message looks misleading: the problem is not TSan is > unsupported for target, it's just unavailable in this distribution for one > reason or > another. The main advantage of the error message Kuba has right now is that it is user friendly. A sanitizer IS unsupported for the given target in the given distribution if the library is missing. Saying something along the lines of "runtime components for '-fsanitize=thread' not available" is vague. For example, does it mean that the user needs to install the runtime components in some other way? http://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
samsonov added inline comments. Comment at: test/Driver/fsanitize.c:221 @@ +220,3 @@ +// RUN: %clang -target x86_64-apple-darwin10 -resource-dir=%S/Inputs/resource_dir -fsanitize=memory -fsanitize=thread,memory %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-MSAN-TSAN-MSAN-DARWIN1 +// CHECK-MSAN-TSAN-MSAN-DARWIN1: unsupported option '-fsanitize=thread,memory' for target 'x86_64-apple-darwin10' +// CHECK-MSAN-TSAN-MSAN-DARWIN1-NOT: unsupported option beanz wrote: > kubabrecka wrote: > > samsonov wrote: > > > Again, I feel like we're lying to users here: `-fsanitize=thread` *is* > > > supported for this target, it just requires building a runtime. > > I'd like to see this from the point-of-view of a binary distribution. If > > the binary distribution (e.g. the one from llvm.org or Apple's Clang in > > Xcode) doesn't contain a runtime library, then the sanitizer is *not* > > supported in that distribution. Also, see http://reviews.llvm.org/D14846, > > we'd like to have CMake options to select which runtimes will be built. If > > you deliberately choose not to build ThreadSanitizer, then that sanitizer > > is *not* supported in your version of Clang. If you're experimenting and > > porting a runtime to a new platform, then this sanitizer *is* supported in > > your version of Clang. > Maybe the point is we should have a different error message for cases where > the runtime is just missing. Something like "runtime components for > '-fsanitize=thread' not available" I see, so essentially you want to use a different approach for determining sanitizer availability (on OS X for now): if the library is present, then we support sanitizer, otherwise we don't: i.e. the binary distribution is the source of truth, not the list of sanitizers hardcoded into Clang driver source code. I'm fine with that, and see why it would make sense. It's just that error message looks misleading: the problem is not TSan is unsupported for target, it's just unavailable in this distribution for one reason or another. http://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
beanz added inline comments. Comment at: test/Driver/fsanitize.c:221 @@ +220,3 @@ +// RUN: %clang -target x86_64-apple-darwin10 -resource-dir=%S/Inputs/resource_dir -fsanitize=memory -fsanitize=thread,memory %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-MSAN-TSAN-MSAN-DARWIN1 +// CHECK-MSAN-TSAN-MSAN-DARWIN1: unsupported option '-fsanitize=thread,memory' for target 'x86_64-apple-darwin10' +// CHECK-MSAN-TSAN-MSAN-DARWIN1-NOT: unsupported option kubabrecka wrote: > samsonov wrote: > > Again, I feel like we're lying to users here: `-fsanitize=thread` *is* > > supported for this target, it just requires building a runtime. > I'd like to see this from the point-of-view of a binary distribution. If the > binary distribution (e.g. the one from llvm.org or Apple's Clang in Xcode) > doesn't contain a runtime library, then the sanitizer is *not* supported in > that distribution. Also, see http://reviews.llvm.org/D14846, we'd like to > have CMake options to select which runtimes will be built. If you > deliberately choose not to build ThreadSanitizer, then that sanitizer is > *not* supported in your version of Clang. If you're experimenting and > porting a runtime to a new platform, then this sanitizer *is* supported in > your version of Clang. Maybe the point is we should have a different error message for cases where the runtime is just missing. Something like "runtime components for '-fsanitize=thread' not available" http://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
kubabrecka added a comment. > The one thing I'm not sure about on this is, do we really want this to be > darwin-only? I kinda think it might be nice if we unified this behavior > across all platforms. > ... I wonder if this can't be better abstracted. > I'd really like to see all operating systems have the same behavior for > this, so I think much of the code can be shared. > If the runtime library exists for a sanitizer (any sanitizer) I don't see > any reason clang shouldn't then support the flags. On Darwin, the situation is easier, because all the supported sanitizers are dylibs. The code to link the runtimes is very different on every platform (Darwin, Linux, FreeBSD, Windows). Alexey, can you comment on this? Does this change make sense for Linux as well? http://reviews.llvm.org/D15225 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
kubabrecka updated this revision to Diff 42041.
kubabrecka added a comment.
Applying clang-format.
http://reviews.llvm.org/D15225
Files:
include/clang/Basic/DiagnosticDriverKinds.td
include/clang/Driver/ToolChain.h
lib/Driver/SanitizerArgs.cpp
lib/Driver/ToolChain.cpp
lib/Driver/ToolChains.cpp
lib/Driver/ToolChains.h
test/Driver/Inputs/resource_dir_darwin_sanitizers/lib/darwin/libclang_rt.asan_iossim_dynamic.dylib
test/Driver/Inputs/resource_dir_darwin_sanitizers/lib/darwin/libclang_rt.asan_osx_dynamic.dylib
test/Driver/Inputs/resource_dir_darwin_sanitizers/lib/darwin/libclang_rt.tsan_osx_dynamic.dylib
test/Driver/Inputs/resource_dir_darwin_sanitizers/lib/darwin/libclang_rt.ubsan_iossim_dynamic.dylib
test/Driver/Inputs/resource_dir_darwin_sanitizers/lib/darwin/libclang_rt.ubsan_osx_dynamic.dylib
test/Driver/darwin-asan-nofortify.c
test/Driver/darwin-sanitizer-ld.c
test/Driver/fsanitize.c
test/Driver/sanitizer-ld.c
Index: test/Driver/sanitizer-ld.c
===
--- test/Driver/sanitizer-ld.c
+++ test/Driver/sanitizer-ld.c
@@ -295,6 +295,7 @@
// RUN: -mmacosx-version-min=10.6 \
// RUN: -target x86_64-apple-darwin13.4.0 \
// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN: -resource-dir=%S/Inputs/resource_dir_darwin_sanitizers \
// RUN: | FileCheck --check-prefix=CHECK-ASAN-DARWIN106-CXX %s
// CHECK-ASAN-DARWIN106-CXX: "{{.*}}ld{{(.exe)?}}"
// CHECK-ASAN-DARWIN106-CXX: libclang_rt.asan_osx_dynamic.dylib
Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -10,8 +10,10 @@
// RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED
// CHECK-UNDEFINED: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|function|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|vptr|object-size|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){19}"}}
-// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-DARWIN
-// CHECK-UNDEFINED-DARWIN: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|object-size|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
+// RUN: %clang -target x86_64-apple-darwin10 -resource-dir=%S/Inputs/resource_dir -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-DARWIN1
+// CHECK-UNDEFINED-DARWIN1: error: '-fsanitize-trap=undefined' required with '-fsanitize=undefined' option
+// RUN: %clang -target x86_64-apple-darwin10 -resource-dir=%S/Inputs/resource_dir_darwin_sanitizers -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-DARWIN2
+// CHECK-UNDEFINED-DARWIN2: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|object-size|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
// RUN: %clang -target i386-unknown-openbsd -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-OPENBSD
// CHECK-UNDEFINED-OPENBSD: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|object-size|float-cast-overflow|array-bounds|enum|bool|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
@@ -28,9 +30,15 @@
// RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER
// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
-// RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
// CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
+// RUN: %clang -target x86_64-apple-darwin10 -resource-dir=%S/Inputs/resource_dir -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS-DARWIN1
+// CHECK-BOUNDS-DARWIN1: error: '-fsanitize-trap=undefined' required with '-fsanitize=bounds' option
+
+// RUN: %clang -target x86_64-apple-darwin10 -resource-dir=%S/Inputs/resource_dir_darwin_sanitizers -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS-DARWIN2
+// CHECK-BOUNDS-DARWIN2: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
+
// RUN: %clang -target x86_64-linux-gnu -fsanitize=all %s -### 2>&1 | FileCheck %s
Re: [PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
samsonov added inline comments.
Comment at: include/clang/Driver/ToolChain.h:410
@@ -407,1 +409,3 @@
+ /// (don't require runtime library).
+ virtual SanitizerMask getSanitizersRequiringTrap() const;
};
I don't think this is relevant to ToolChain at all. `SanitizerArgs` has
`TrappingSupported` mask, which is what you should need: if you don't have
runtimes for all sanitizers you're enabling, and *some* of these sanitizers
support trapping, driver may advise to use this flag.
Comment at: lib/Driver/ToolChains.cpp:360
@@ +359,3 @@
+ StringRef OS = "";
+ if (isTargetMacOS()) OS = "osx";
+ if (isTargetWatchOSSimulator()) OS = "watchossim";
Can we teach ToolChain (or at least Darwin toolchain) to return us OS name
instead?
Comment at: lib/Driver/ToolChains.cpp:369
@@ +368,3 @@
+
+ return (Twine("libclang_rt.") + Sanitizer + "_" + OS + "_dynamic.dylib")
+ .str();
Oh no, please don't return a `StringRef` which points to temporary.
Comment at: test/Driver/fsanitize.c:14
@@ +13,3 @@
+// RUN: %clang -target x86_64-apple-darwin10
-resource-dir=%S/Inputs/resource_dir -fsanitize=undefined %s -### 2>&1 |
FileCheck %s --check-prefix=CHECK-UNDEFINED-DARWIN1
+// CHECK-UNDEFINED-DARWIN1: error: '-fsanitize-trap=undefined' required with
'-fsanitize=undefined' option
+// RUN: %clang -target x86_64-apple-darwin10
-resource-dir=%S/Inputs/resource_dir_darwin_sanitizers -fsanitize=undefined %s
-### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-DARWIN2
I find this diagnostic misleading :-/ It's not the case that
`-fsanitize=undefined` *requires* you to additionally provide
`-fsanitize-trap=undefined`. It requires having a UBSan runtime library
available. Or, if you can't provide it (it's not available on your system), you
can workaround this by using `-fsanitize-trap`. The latter should be a
suggestion, really.
Comment at: test/Driver/fsanitize.c:33
@@ -30,3 +32,3 @@
-// RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s
--check-prefix=CHECK-BOUNDS
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=bounds -### -fsyntax-only
%s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
// CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
Nice catch, please commit this fix as a separate change.
Comment at: test/Driver/fsanitize.c:214
@@ -205,3 +213,3 @@
-// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=memory %s -### 2>&1 |
FileCheck %s --check-prefix=CHECK-MSAN-DARWIN
+// RUN: %clang -target x86_64-apple-darwin10
-resource-dir=%S/Inputs/resource_dir -fsanitize=memory %s -### 2>&1 | FileCheck
%s --check-prefix=CHECK-MSAN-DARWIN
// CHECK-MSAN-DARWIN: unsupported option '-fsanitize=memory' for target
'x86_64-apple-darwin10'
Why not resource_dir_darwin_sanitizers here and below?
Comment at: test/Driver/fsanitize.c:221
@@ +220,3 @@
+// RUN: %clang -target x86_64-apple-darwin10
-resource-dir=%S/Inputs/resource_dir -fsanitize=memory -fsanitize=thread,memory
%s -### 2>&1 | FileCheck %s --check-prefix=CHECK-MSAN-TSAN-MSAN-DARWIN1
+// CHECK-MSAN-TSAN-MSAN-DARWIN1: unsupported option '-fsanitize=thread,memory'
for target 'x86_64-apple-darwin10'
+// CHECK-MSAN-TSAN-MSAN-DARWIN1-NOT: unsupported option
Again, I feel like we're lying to users here: `-fsanitize=thread` *is*
supported for this target, it just requires building a runtime.
http://reviews.llvm.org/D15225
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15225: [Driver] Sanitizer support based on runtime library presence
beanz added a comment.
Mostly formatting comments. I think for the most part this is the right way to
go, but I don't really feel qualified to approve this. The one thing I'm not
sure about on this is, do we really want this to be darwin-only? I kinda think
it might be nice if we unified this behavior across all platforms.
Comment at: lib/Driver/SanitizerArgs.cpp:204
@@ -203,2 +203,3 @@
const SanitizerMask Supported = setGroupBits(TC.getSupportedSanitizers());
+ const SanitizerMask RequiresTrap =
setGroupBits(TC.getSanitizersRequiringTrap());
ToolChain::RTTIMode RTTIMode = TC.getRTTIMode();
80 col
Comment at: lib/Driver/SanitizerArgs.cpp:240
@@ +239,3 @@
+ if ((Add & TrappingKinds) == 0) {
+if (SanitizerMask KindsToDiagnose = Add & RequiresTrap &
~DiagnosedKinds) {
+ std::string Desc = describeSanitizeArg(*I, KindsToDiagnose);
Also 80 col violation.
Please clang-format.
Comment at: lib/Driver/ToolChains.cpp:1243
@@ -1214,2 +1242,3 @@
+
SanitizerMask Darwin::getSupportedSanitizers() const {
SanitizerMask Res = ToolChain::getSupportedSanitizers();
I'm not super familiar with the driver code, but I wonder if this can't be
better abstracted.
I'd really like to see all operating systems have the same behavior for this,
so I think much of the code can be shared.
If the runtime library exists for a sanitizer (any sanitizer) I don't see any
reason clang shouldn't then support the flags.
Comment at: lib/Driver/ToolChains.cpp:1258
@@ -1223,1 +1257,3 @@
+ if (!SanitizerRuntimeExists("ubsan")) {
+Res |= (SanitizerKind::Undefined & ~SanitizerKind::Vptr &
~SanitizerKind::Function);
}
Last time I'll comment this, please clang-format.
http://reviews.llvm.org/D15225
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
