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

Enable address sanitizer #1821

Closed
wants to merge 8 commits into from

Conversation

asavonic
Copy link
Contributor

@asavonic asavonic commented Jun 5, 2020

No description provided.

@@ -169,6 +169,11 @@ install(DIRECTORY ${OPENCL_INCLUDE}/CL
option(SYCL_BUILD_PI_CUDA
"Enables the CUDA backend for the Plugin Interface" OFF)

option(SYCL_USE_SANITIZER "Use address sanitizer" OFF)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it maybe make sense to use LLVM_USE_SANITIZER value to set the default for SYCL_USE_SANITIZER.

Also we probably should align possible values with LLVM_USE_SANITIZER:

Define the sanitizer used to build LLVM binaries and tests. Possible values are Address, Memory, MemoryWithOrigins, Undefined, Thread, DataFlow, and Address;Undefined. Defaults to empty string.

SYCL_INCLUDE_TESTS uses similar logic - it's default value is LLVM project default, but user can override SYCL specific variable.

@@ -74,7 +74,8 @@ def do_configure(args):
"-DLLVM_ENABLE_DOXYGEN={}".format(llvm_enable_doxygen),
"-DLLVM_ENABLE_SPHINX={}".format(llvm_enable_sphinx),
"-DBUILD_SHARED_LIBS={}".format(llvm_build_shared_libs),
"-DSYCL_ENABLE_XPTI_TRACING=ON" # Explicitly turn on XPTI tracing
"-DSYCL_ENABLE_XPTI_TRACING=ON", # Explicitly turn on XPTI tracing
"-DSYCL_USE_SANITIZER=ON"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, sanitation brings some overhead and using this script is recommended in get started guide, which making "ON" a default value for almost everyone who is building the library from sources.

I think we should have two separate configuration modes for the project:

  • regular (user mode) - assuming that CI status is "green" just build me a SYCL compiler toolchain I can use in my daily development.
  • CI (developers mode) - enable as much checks as possible

I think it's more reasonable to use "regular" mode by default, so that users following get started guide won't be surprised with unexpected overhead or checks.
It should be easy for developers to enable "CI" mode as well.

One of the possible implementations can be:

  • we add -ci option to buildbot scripts, which is off by default and set by CI checks and can be used by developers for the development and/or to reproduce CI behavior.

@tfzhu, does it sounds reasonable to you? If so we can add this option ASAP and gradually separate "user" and "developer" modes.

"Enable -Werror flag" is another option in addition to sanitizer, which should be disabled in "user" mode. Users have different C++ toolchains on their systems and we have a number of bug reports about failed builds due to new warnings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with adding -ci option

Andrew Savonichev added 8 commits July 6, 2020 18:17
Signed-off-by: Andrew Savonichev <[email protected]>
This patch fixes:

ERROR: AddressSanitizer: alloc-dealloc-mismatch (operator new [] vs
operator delete)

Signed-off-by: Andrew Savonichev <[email protected]>
This patch fixes:

ERROR: AddressSanitizer: alloc-dealloc-mismatch (operator new [] vs
operator delete) on 0x604000008690

Signed-off-by: Andrew Savonichev <[email protected]>
Use-after-free happens only for non-production code.

Signed-off-by: Andrew Savonichev <[email protected]>
Signed-off-by: Andrew Savonichev <[email protected]>
Signed-off-by: Andrew Savonichev <[email protected]>
Signed-off-by: Andrew Savonichev <[email protected]>
@asavonic asavonic force-pushed the private/asavonic/asan-gcc branch from 39989a8 to 489ea15 Compare July 7, 2020 11:05
@asavonic asavonic closed this Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants