-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add sanitizer features to unix_cc_toolchain_config #17083
Add sanitizer features to unix_cc_toolchain_config #17083
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
https://cs.opensource.google/bazel/bazel/+/3748bcc8bf0c45e34295583fafaf1f52449114b8:src/test/shell/bazel/cc_integration_test.sh;l=0 runs on Linux and macOS, I think that tests would fit in there. |
3966604
to
8ecc178
Compare
Is there a suggestion for handling platforms without sanitizer libraries to link against?
|
8ecc178
to
b12f7d8
Compare
@oliverlee Looks like the failure is with gcc on an old centos, which would be rather difficult to match with Bazel platform constraints. Instead, since the feature is opt-in anyway, you could check for gcc and its version in the integration test and skip the test if it isn't supported. The ASan test failure on macOS may be missing due to compiler optimizations as the example code doesn't really do anything. We have found these examples trigger ASan and UBSan relatively reliably across platforms. |
b12f7d8
to
30c6b07
Compare
30c6b07
to
de65b91
Compare
@fmeum I've updated tests to check for lib*san on Linux since is seems that centos is using gcc 10. It also looks like you're right about compiler optimizations so I've locally disabled optimizations with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but this needs a team member's approval. @oquenchil, would you be available to review this PR?
Create a local copy of unix_cc_toolchain_config.bzl. This commit should be reverted once bazelbuild/bazel#17083 is merged.
Defines asan, tsan, ubsan features. This commit should be reverted once bazelbuild/bazel#17083 is merged.
Thanks for reviewing and especially @fmeum for all your help. |
@bazel-io flag |
@bazel-io fork 6.1.0 |
There was some discussion here about adding `asan`, `tsan`, and `ubsan` features to the unix toolchains to match macos. bazel-contrib/toolchains_llvm#90 (comment) I've taken my changes local to that project and copied it into Bazel as suggested by @fmeum. I've written some tests but I'm not sure where to place them or if it makes sense to depend on the error messages from asan/tsan/ubsan. Closes #17083. PiperOrigin-RevId: 501213060 Change-Id: I9d973ebe35e4fa2804d2e91df9f700a285f7b404 Co-authored-by: Oliver Lee <[email protected]>
There was some discussion here about adding `asan`, `tsan`, and `ubsan` features to the unix toolchains to match macos. bazel-contrib/toolchains_llvm#90 (comment) I've taken my changes local to that project and copied it into Bazel as suggested by @fmeum. I've written some tests but I'm not sure where to place them or if it makes sense to depend on the error messages from asan/tsan/ubsan. Closes #17083. PiperOrigin-RevId: 501213060 Change-Id: I9d973ebe35e4fa2804d2e91df9f700a285f7b404
There was some discussion here about adding
asan
,tsan
, andubsan
features to the unix toolchains to match macos. bazel-contrib/toolchains_llvm#90 (comment)I've taken my changes local to that project and copied it into Bazel as suggested by @fmeum. I've written some tests but I'm not sure where to place them or if it makes sense to depend on the error messages from asan/tsan/ubsan.