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.

Sign up for GitHub

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

Jump to bottom

Avoid ASAN for release builds, use w/ GCC or clang in debug builds #425

Merged
merged 3 commits into from
Apr 19, 2024
Merged

Avoid ASAN for release builds, use w/ GCC or clang in debug builds #425

merged 3 commits into from
Apr 19, 2024

Conversation

cpu
Copy link
Member

@cpu cpu commented Apr 19, 2024

This branch resolves #423 by:

  • Making sure we only use ASAN with debug builds, and switching most of CI to run tests with that profile. Valgrind is left using release builds because ASAN + Valgrind conflict without more care.
  • Enabling ASAN for GCC in addition to Clang. GCC has also had ASAN support built-in for a long time. We might as well use it for more builds.
  • Adding simple checks in CI that debug builds are using ASAN and release builds aren't. Presently this isn't comprehensive, but a quick safety check on Linux hosts using nm.

cpu added 2 commits April 19, 2024 15:01
@cpu
Makefile: only use ASAN for debug+clang
b3c78e3 
Previously if `$CC` was `clang` the `Makefile` and `Makefile.pkg-config`
make files would configure `CFLAGS` and `LDFLAGS` with address sanitizer
(ASAN) enabled.

We only want this to happen for _debug_ builds that are using `clang`,
since it's generally considered bad practice to ship sanitizer builds as
release artifacts.

This commit updates the logic to require `PROFILE=debug`, and then
switches CI to use that profile when running tests. The Valgrind test
continues to use `PROFILE=release` because ASAN and Valgrind will
conflict, raising the error "ASan runtime does not come first in initial
library list;". It may be possible to use both with more
care/configuration, but for now we'll just use valgrind with a release
build.
@cpu
Makefile: enable ASAN for GCC
a097fa1 
Previously `Makefile` and `Makefile.pkg-config` conditionally enabled
address sanitizer (ASAN) based on whether the compiler was `clang`.
Since GCC has supported address sanitizer since GCC 4.8[0] we can remove
this check and always use ASAN for both supported compilers.

The only snag is that I observed linker errors about undefined
references to `__ubsan_handle_type_mismatch` unless I fix the `LDFLAGS`
to also include `-fsanitize=undefined`. This was already included in the
`CFLAGS`.

[0]: https://github.com/google/sanitizers/wiki/AddressSanitizer#getting-addresssanitizer
@cpu cpu self-assigned this Apr 19, 2024
@cpu cpu changed the title Avoid ASAN for prod builds, use w/ GCC or clang in debug builds Avoid ASAN for release builds, use w/ GCC or clang in debug builds Apr 19, 2024
cpu
cpu commented Apr 19, 2024
View reviewed changes
Makefile Outdated Show resolved Hide resolved
@cpu
ci: verify ASAN presence/absence as appropriate
4afce73 
Presently only done on Linux since the test relies on `nm` being
available.
@cpu cpu merged commit 0e8e673 into rustls:main Apr 19, 2024
23 checks passed
@cpu cpu deleted the cpu-423-asan-tests-only branch April 19, 2024 21:19
@cpu cpu mentioned this pull request Apr 23, 2024
Merged
@cpu cpu mentioned this pull request Aug 7, 2024
Merged
@chenrui333 chenrui333 mentioned this pull request Sep 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsha jsha jsha approved these changes

Assignees

@cpu cpu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use -fsanitize=address for tests only
2 participants
@cpu @jsha