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 -fsanitizer=address for debug builds in travis #2653

Merged
merged 3 commits into from
Oct 19, 2016
Merged

Conversation

TheMarex
Copy link
Member

@TheMarex TheMarex commented Jul 13, 2016

@@ -54,6 +54,7 @@ option(ENABLE_ASSERTIONS OFF)
option(COVERAGE OFF)
option(SANITIZER OFF)
option(ENABLE_LTO "Use LTO if available" ON)
option(ENABLE_SANITIZER "Use memory sanitizer for Debug build" OFF)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of hardcoding options for asan / msan / tsan / ubsan / ... it's probably cleaner to just set the env var

CXXFLAGS=-fsanitize=X

and let CMake pick it up automatically.

@TheMarex TheMarex force-pushed the build/add-sanitize branch from 77d4e27 to 4418102 Compare July 19, 2016 17:10
@TheMarex TheMarex force-pushed the build/add-sanitize branch from 4418102 to 61fa142 Compare August 19, 2016 10:54
@TheMarex TheMarex changed the title Add -fsanitizer=address for debug builds in travis [WIP] Add -fsanitizer=address for debug builds in travis Aug 19, 2016
@TheMarex
Copy link
Member Author

This fails because osrm-extract is triggers the sanitizer https://travis-ci.org/Project-OSRM/osrm-backend/jobs/153525915#L1824-L1882 which is tracked #2788

@MoKob MoKob changed the title [WIP] Add -fsanitizer=address for debug builds in travis Add -fsanitizer=address for debug builds in travis Sep 2, 2016
@TheMarex TheMarex force-pushed the build/add-sanitize branch 6 times, most recently from 0f7fa0f to 3ea4cc4 Compare October 5, 2016 10:49
@MoKob MoKob force-pushed the build/add-sanitize branch from 3ea4cc4 to 47d064c Compare October 5, 2016 11:34
@TheMarex TheMarex force-pushed the build/add-sanitize branch from 47d064c to 102491f Compare October 5, 2016 11:35
@TheMarex
Copy link
Member Author

TheMarex commented Oct 5, 2016

We are currently hitting http://stackoverflow.com/questions/37603238/fsanitize-not-using-gold-linker-in-gcc-6-1 with GCC. Switching to clang now and see if that works.

@TheMarex TheMarex force-pushed the build/add-sanitize branch from 32e337a to b57eb27 Compare October 5, 2016 15:34
@oxidase
Copy link
Contributor

oxidase commented Oct 5, 2016

@TheMarex the problem was in linking example without gold against OSRM libraries built with gold. It is only ubuntu gcc 6.1 issue. gcc 6.2 works fine. I have committed to my repository https://github.com/oxidase/osrm-backend/tree/build/add-sanitize to avoid forced push to upstream.

There were 2 problems:

If it looks ok, i can squash commits and push into upstream.

@TheMarex
Copy link
Member Author

TheMarex commented Oct 6, 2016

@oxidase thanks for looking into this! 👍 I added some comments on your branch. You can pull in the changes here if you think my comments are resolved.

@oxidase
Copy link
Contributor

oxidase commented Oct 6, 2016

The issue /usr/bin/ld: unrecognized option '--push-state' is due to missing '-fuse-ld=gold' in the example that is built with gcc-6.2 from the toolchain PPA and ld 2.24 from trusty that are not fully compatible. There is a problem with binutils 2.24 that do not support the option introduced by asan.

I have added a binutils PPA that bumps version to 2.27 for the debug asan build. It can be removed when binutils in the toolchain PPA will be updated to at least 2.26 or travis will switch to xenial.

@TheMarex
Copy link
Member Author

@oxidase thanks for looking into this again. Great to see this working finally. 💯

I'm going to do the following things here:

  • Split the commit up into different commit for build system/travis changes
  • Split the code changes from the commit into an own commit

If we wait for #3015 the shared memory fixes are obsolete anyway since the whole ownership mode got reworked.

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

Successfully merging this pull request may close these issues.

4 participants