Change driver logic for sanitizers support. #10232
Conversation
|
@swift-ci Please smoke test |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
jrose-apple
left a comment
jrose-apple
left a comment
There was a problem hiding this comment.
Nice improvement for those who might not have compiler-rt checked out. A lot of small comments and some testing concerns.
include/swift/Driver/ToolChain.h
Outdated
There was a problem hiding this comment.
Nitpick: funny indentation here.
There was a problem hiding this comment.
Nitpick: Please use llvm::function_ref for callbacks that are not persisted.
lib/Driver/Driver.cpp
Outdated
There was a problem hiding this comment.
Nitpick: No need for const.
lib/Driver/ToolChain.cpp
Outdated
There was a problem hiding this comment.
Please drop this comment. You can say something like "Assume no sanitizers are supported on a particular platform by default" if you want.
lib/Frontend/CompilerInvocation.cpp
Outdated
There was a problem hiding this comment.
Typo: "existence". Also, should this be a default argument to parseSanitizerArgValues then?
There was a problem hiding this comment.
I believe so, it's the confusing two-stage check. If the library wasn't there, we would have errorred out by now.
lib/Option/SanitizerOptions.cpp
Outdated
There was a problem hiding this comment.
Nitpick: funny indentation here.
lib/Option/SanitizerOptions.cpp
Outdated
There was a problem hiding this comment.
Nitpick: This isn't your fault, but if you can't align parameters then the preference is to double-indent them and leave it, not right-align the longest parameter and then match the others.
test/Sanitizers/tsan.swift
Outdated
There was a problem hiding this comment.
Please put input files in an Inputs/ folder rather than giving them a dummy RUN line.
test/Sanitizers/tsan_macos.swift
Outdated
There was a problem hiding this comment.
Why split this file up, though?
test/Driver/sanitizers.swift
Outdated
There was a problem hiding this comment.
Please don't drop these tests. We still need to make sure we get these right for all the other Apple platforms.
|
@swift-ci please test |
test/Sanitizers/tsan.swift
Outdated
There was a problem hiding this comment.
Is this comment no longer relevant?
test/Driver/sanitizers.swift
Outdated
There was a problem hiding this comment.
No need for this here since nothing's actually getting run and everything has an explicit target.
lib/Option/SanitizerOptions.cpp
Outdated
There was a problem hiding this comment.
Missed a function_ref.
lib/Frontend/CompilerInvocation.cpp
Outdated
There was a problem hiding this comment.
Nitpick: no const and no need to capture anything.
lib/Driver/ToolChain.cpp
Outdated
There was a problem hiding this comment.
Nitpick: headers are out of order. (They should be alphabetical within the project.)
|
Build failed |
|
Build failed |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
jrose-apple
left a comment
jrose-apple
left a comment
There was a problem hiding this comment.
Looks good. Thanks for sticking with all my comments!
cheshire
changed the title
cheshire
changed the title
|
@swift-ci please smoke test |
With this patch different sanitizers (tsan/asan) will be enabled or disabled on the driver level on a particular OS depending on whether the required library is present. The current patch only supports Darwin architectures, but Linux support should not be hard to add.
in order not to make them depend on the platform they are running on.
|
@swift-ci please smoke test |
cheshire
force-pushed
the
sanitizer_driver_detection
branch
from
69e4f0e to
5956aa9
Compare
|
@swift-ci please smoke test linux |
3 participants
With this patch different sanitizers (tsan/asan) will be enabled or
disabled on the driver level on a particular OS depending on whether
the required library is present.
The current patch only supports Darwin architectures, but Linux support
should not be hard to add.