Skip to content
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

Closed

Conversation

oliverlee
Copy link
Contributor

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.

@google-cla
Copy link

google-cla bot commented Dec 28, 2022

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.

@sgowroji sgowroji added team-Rules-CPP Issues for C++ rules awaiting-user-response Awaiting a response from the author labels Dec 28, 2022
@fmeum
Copy link
Collaborator

fmeum commented Dec 28, 2022

@oliverlee
Copy link
Contributor Author

Is there a suggestion for handling platforms without sanitizer libraries to link against?

https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/01855fff-0611-4e48-957c-fd2a745c57f7/src/test/shell/bazel/cc_integration_test/shard_3_of_5/test.log

ld.gold: error: cannot find -lasan"

@fmeum
Copy link
Collaborator

fmeum commented Dec 30, 2022

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

@sgowroji sgowroji added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Jan 2, 2023
@oliverlee
Copy link
Contributor Author

@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 volatile in the asan example. Let me know if you would prefer the sample you provided.

Copy link
Collaborator

@fmeum fmeum left a 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?

oliverlee added a commit to oliverlee/bazel-toolchain that referenced this pull request Jan 6, 2023
Create a local copy of unix_cc_toolchain_config.bzl.

This commit should be reverted once bazelbuild/bazel#17083 is merged.
oliverlee added a commit to oliverlee/bazel-toolchain that referenced this pull request Jan 6, 2023
Defines asan, tsan, ubsan features.

This commit should be reverted once bazelbuild/bazel#17083 is merged.
@oquenchil oquenchil added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jan 9, 2023
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jan 11, 2023
@oliverlee
Copy link
Contributor Author

Thanks for reviewing and especially @fmeum for all your help.

@fmeum
Copy link
Collaborator

fmeum commented Jan 12, 2023

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jan 12, 2023
@ShreeM01
Copy link
Contributor

@bazel-io fork 6.1.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jan 19, 2023
ShreeM01 added a commit that referenced this pull request Jan 20, 2023
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 <oliverzlee@gmail.com>
hvadehra pushed a commit that referenced this pull request Feb 14, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants