Page MenuHomePhabricator

[Driver] Sanitizer support based on runtime library presence
ClosedPublic

Authored by george.karpenkov on Dec 4 2015, 3:39 AM.

Details

Summary

The runtime libraries of sanitizers are built in compiler-rt, and Clang can be built without compiler-rt, or compiler-rt can be configured to only build certain sanitizers. The driver should provide reasonable diagnostics and not a link-time error when a runtime library is missing.

This patch changes the driver for OS X to only support sanitizers of which we can find the runtime libraries. The discussion for this patch explains the rationale.

Diff Detail

Repository
rL LLVM

Event Timeline

kubamracek updated this revision to Diff 41862.Dec 4 2015, 3:39 AM
kubamracek retitled this revision from to [Driver] Sanitizer support based on runtime library presence.
kubamracek updated this object.
kubamracek added subscribers: llvm-commits, zaks.anna.
kubamracek edited subscribers, added: cfe-commits; removed: llvm-commits.Dec 4 2015, 7:43 AM
beanz edited edge metadata.Dec 4 2015, 1:31 PM

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.

lib/Driver/SanitizerArgs.cpp
204 ↗(On Diff #41862)

80 col

240 ↗(On Diff #41862)

Also 80 col violation.

Please clang-format.

lib/Driver/ToolChains.cpp
1243 ↗(On Diff #41862)

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.

1258 ↗(On Diff #41862)

Last time I'll comment this, please clang-format.

kubamracek updated this revision to Diff 42041.Dec 7 2015, 3:10 AM
kubamracek edited edge metadata.

Applying clang-format.

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?

samsonov added inline comments.Dec 7 2015, 11:06 AM
include/clang/Driver/ToolChain.h
410 ↗(On Diff #42041)

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.

lib/Driver/ToolChains.cpp
360 ↗(On Diff #42041)

Can we teach ToolChain (or at least Darwin toolchain) to return us OS name instead?

369 ↗(On Diff #42041)

Oh no, please don't return a StringRef which points to temporary.

test/Driver/fsanitize.c
14 ↗(On Diff #42041)

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.

33 ↗(On Diff #42041)

Nice catch, please commit this fix as a separate change.

214 ↗(On Diff #42041)

Why not resource_dir_darwin_sanitizers here and below?

221 ↗(On Diff #42041)

Again, I feel like we're lying to users here: -fsanitize=thread *is* supported for this target, it just requires building a runtime.

kubamracek added inline comments.Dec 7 2015, 11:20 AM
test/Driver/fsanitize.c
221 ↗(On Diff #42041)

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.

beanz added inline comments.Dec 8 2015, 10:30 AM
test/Driver/fsanitize.c
221 ↗(On Diff #42041)

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"

samsonov added inline comments.Dec 10 2015, 2:36 PM
test/Driver/fsanitize.c
221 ↗(On Diff #42041)

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.

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?

samsonov edited edge metadata.Jan 21 2016, 10:59 AM

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?

s/unsupported/unavailable? 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). Anyway, you're in much better position to make a judgement here, leaving this to you.

I believe, at least part of this patch will be superseded by http://reviews.llvm.org/D15624? Feel free to update this one when the latter lands.

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.

ygribov added inline comments.
test/Driver/fsanitize.c
221 ↗(On Diff #42041)

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.

kubamracek added inline comments.Jan 25 2016, 8:12 AM
test/Driver/fsanitize.c
221 ↗(On Diff #42041)

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.

ygribov added inline comments.Jan 25 2016, 9:34 AM
test/Driver/fsanitize.c
221 ↗(On Diff #42041)

Ah, for some strange reason I thought that you were searching runtime lib in sysroot rather than compiler root.

LGTM.

Any other comments to this?

beanz requested changes to this revision.Jun 27 2016, 4:20 PM
beanz edited edge metadata.

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

This revision now requires changes to proceed.Jun 27 2016, 4:20 PM

I missed that! Sorry about that.

george.karpenkov commandeered this revision.Jul 16 2018, 4:22 PM
george.karpenkov updated this revision to Diff 155785.
george.karpenkov added a reviewer: kubamracek.
george.karpenkov edited the summary of this revision. (Show Details)
george.karpenkov edited reviewers, added: delcypher; removed: bob.wilson, glider, t.p.northover, samsonov, beanz.

Attempt #2: reduced version of this patch, without ubsan support.

This looks great to me, but someone else should review this as well.

@delcypher Could you take a look?
@kcc Any objections?

delcypher requested changes to this revision.Jul 17 2018, 2:50 PM

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.

clang/lib/Driver/ToolChains/Darwin.h
134 ↗(On Diff #155785)

Why is this a SmallString<128> but in other places we're just using std::string?

427 ↗(On Diff #155785)

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?

clang/test/Driver/darwin-asan-nofortify.c
3 ↗(On Diff #155785)

Are these -resource-dir %S/Inputs/resource_dir needed because compiler-rt might not be present and now the driver checks for these?

This revision now requires changes to proceed.Jul 17 2018, 2:50 PM
delcypher added inline comments.Jul 17 2018, 3:00 PM
clang/lib/Driver/ToolChains/Darwin.cpp
2298 ↗(On Diff #155785)

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;
}

@delcypher sorry i don't agree with the change requests.
@mracek any comments?

clang/lib/Driver/ToolChains/Darwin.cpp
2298 ↗(On Diff #155785)

I'm not sure it would be worth crashing the driver.

clang/lib/Driver/ToolChains/Darwin.h
134 ↗(On Diff #155785)

For driver it does not matter which datastructure to use.
Here SmallString is used because that's what is more convenient to return.

427 ↗(On Diff #155785)

Then it's not clear whether the returned path is relative or absolute though.

clang/test/Driver/darwin-asan-nofortify.c
3 ↗(On Diff #155785)

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.

delcypher added inline comments.Jul 20 2018, 12:25 PM
clang/lib/Driver/ToolChains/Darwin.cpp
2298 ↗(On Diff #155785)

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.

clang/lib/Driver/ToolChains/Darwin.h
427 ↗(On Diff #155785)

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.

george.karpenkov marked 2 inline comments as done.
delcypher added inline comments.Jul 20 2018, 3:34 PM
clang/lib/Driver/ToolChains/Darwin.h
425 ↗(On Diff #156603)

You might want to update that comment to not mention "Relative path"

@george.karpenkov Other than the comment that probably needs updating, LGTM.

delcypher accepted this revision.Jul 20 2018, 3:34 PM
This revision is now accepted and ready to land.Jul 20 2018, 3:34 PM
This revision was automatically updated to reflect the committed changes.

+@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.

@rnk could you clarify how did it break the distributed asanified build? If the slave compiler supports asan it should have this runtime, right?

@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.

rnk added a comment.Jul 31 2018, 1:38 PM

@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.

@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.

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.

@rnk OK I see, we've accidentally posted comments nearly simultaneously.

@rnk changing subjects: will you be at the LLVM social on Thursday? Could we discuss it there?

rnk added a comment.Jul 31 2018, 2:03 PM

@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.

@rnk OK it's fine with me to revert it now and then we see what can be done.

@rnk As discussed, would it be acceptable for you to just have empty sanitizer runtime files in the resource directory?

rnk added a subscriber: beanz.Aug 7 2018, 1:47 PM

@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.

rnk added a comment.Aug 8 2018, 10:39 AM

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.