Skip to content
This repository has been archived by the owner on Jan 2, 2021. It is now read-only.

Make the unit tests ASAN-clean and run on Travis #131

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sas
Copy link
Contributor

@sas sas commented Sep 30, 2016

No description provided.

#set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fsanitize=address")
#set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address")
if (CMAKE_CXX_COMPILER_ID MATCHES "GNU" OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
# Global options (these apply for other subprojects like JSObjects) and must
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 this is from ds2, but doesn't apply here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. Let me fix this.

set(COMMON_FLAGS "${COMMON_FLAGS} -g -fno-omit-frame-pointer")
endif ()

if (SANITIZER STREQUAL "asan")
Copy link
Contributor

@grp grp Sep 30, 2016

Choose a reason for hiding this comment

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

Why use "asan" here? With -DSANITIZE=address it could just be -fsanitize=${SANITIZE}, much less code. Or even just pass it via CMAKE_CXX_FLAGS directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do it this way because "asan" is the name people know about, and because for other sanitizers (e.g.: ubsan), we can't just take the cmake argument and pass it down.

# Global options (these apply for other subprojects like JSObjects) and must
# be set before we include subdirectories.
if (SANITIZER)
set(COMMON_FLAGS "${COMMON_FLAGS} -g -fno-omit-frame-pointer")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be replaced by -DCMAKE_BUILD_TYPE=Debug in the Makefile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want the cmake to be usable standalone. Also, ASAN != debug; you could build release (with optimizations) and have asan enabled and generate debug info to print backtraces.

@sas sas force-pushed the asan branch 2 times, most recently from 9acdf19 to 5ea53b0 Compare October 3, 2016 20:07
@grp
Copy link
Contributor

grp commented Oct 4, 2016

Weird error:

/usr/bin/ld: ThirdParty/googletest/googletest/libgtest.a(gtest-all.cc.o): undefined reference to symbol 'pthread_key_delete@@GLIBC_2.2.5'
//lib/x86_64-linux-gnu/libpthread.so.0: error adding symbols: DSO missing from command line

Also, is there a way that Travis could check all of the sanitizers at once?

@grp
Copy link
Contributor

grp commented Oct 4, 2016

(Google suggests the error means a missing -lpthread on the command line?)

This will make it easier to enable these sanitizers with a simple
command-line switch, and potentially enable them on Travis.
This should prevent checking in some code with memory errors.
@facebook-github-bot
Copy link

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email [email protected] with your details so we can update your status.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants