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

[Don't merge] Add cron jobs for sanitizers and libc++ debug mode #1624

Closed
wants to merge 1 commit into from

Conversation

reuk
Copy link
Contributor

@reuk reuk commented Nov 24, 2017

This PR adds stages for address and undefined-behavior sanitizer, and for the libc++ debug mode. These stages should only run during a cron job.

Obviously don't merge this until we know that the stages work properly...

I think the input of @chrisr-diffblue, @owen-jones-diffblue, @zemanlx would be especially useful on this PR.

Copy link
Contributor

@owen-mc-diffblue owen-mc-diffblue left a comment

Choose a reason for hiding this comment

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

How about one with USE_DSTRING set to false? Or is that not working at the moment?

@reuk
Copy link
Contributor Author

reuk commented Nov 27, 2017

I tried this out the other day and I couldn't get it working completely. @thk123 might have continued working on this though.

.travis.yml Outdated
'-DCMAKE_CXX_COMPILER=clang++' \
'-DCMAKE_CXX_FLAGS=-fsanitize=address,undefined'
- cmake --build build -- -j4
script: (cd build; ctest -V -L CORE)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably run ctest with -j2 to get a bit of parallelisation of tests.

@reuk
Copy link
Contributor Author

reuk commented Nov 27, 2017

@zemanlx Could you enable a nightly run of CBMC's CI?
I'm going to push a commit which removes the if: type = cron lines so that we can check the builds actually work, but I'll drop the commit once the builds work, the cron job is enabled, and this is ready to merge.

.travis.yml Outdated
- NAME="sanitized build"
if: type = cron
os: linux
sudo: required
Copy link
Contributor

Choose a reason for hiding this comment

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

Is sudo really required? I don't see what you are doing that does need that, and I think adding the requirement for sudo means we get slower job startup? (i.e. sudo requires a full VM that travis estimate as a 30-50 second startup, where as sudo not required just means firing up a container which is 1-6s). Only other consideration is that sudo-required machines get more RAM allocation, according to: https://docs.travis-ci.com/user/reference/overview/

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'll try without sudo and see how it goes

.travis.yml Outdated
- NAME="libc++ debug build'
if: type = cron
os: linux
sudo: required
Copy link
Contributor

Choose a reason for hiding this comment

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

Same query regarding sudo here.

Copy link
Contributor

@chrisr-diffblue chrisr-diffblue left a comment

Choose a reason for hiding this comment

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

Would it be possible to add an additional CMakeFiles build configuration (like we have ReleaseWithDebug) for the sanitised builds, and for the libcpp debug builds, and then just build/test those targets in these travis jobs? The only reason I ask is that would then give people a (possibly) nicer way to run sanitizer/debug builds locally.

@reuk reuk force-pushed the reuk/cron-jobs branch 2 times, most recently from 9810ce9 to f7aa39d Compare November 27, 2017 12:59
@reuk
Copy link
Contributor Author

reuk commented Nov 27, 2017

I had a go at this. There's now Sanitized and LibCPPDebug configurations. The sanitized build enables asan and ubsan, while the libcppdebug config passes the debug flags for libc++ and libstdc++. These configs should work on (modern versions of) both g++ and clang++, so we don't lock the configs to specific compilers.

Also worth mentioning: I put the new configurations inside a conditional block, so that superprojects can define their own configurations without having them overridden by CBMC.

@tautschnig
Copy link
Collaborator

@reuk is the "Don't merge" in the subject still accurate? Arguably I can't yet be merged as the clang "sanitized" build fails with "argument unused during compilation: ..." -- will you be looking into this?

@tautschnig
Copy link
Collaborator

Also, would you mind adding "Fixes: #832" to one of the commit messages (if my assertion that that issue would be fixed is actually true).

@reuk
Copy link
Contributor Author

reuk commented Dec 1, 2017

"Don't merge" is accurate until I've managed to get the extra checks to pass CI, and the cron job has been enabled in Travis. For the time being, I'm just going to disable the 'unused command line argument' error, as I can't reproduce it locally. The argument could be made for being more conservative with the include paths we use for each library, but I can't fix this unless I can reproduce the build locally.

@reuk reuk force-pushed the reuk/cron-jobs branch 8 times, most recently from 0ddb679 to d8f4de0 Compare December 1, 2017 15:44
@reuk reuk force-pushed the reuk/cron-jobs branch 2 times, most recently from 6bca041 to ea147a0 Compare December 1, 2017 16:14
@reuk reuk closed this Dec 1, 2017
@owen-mc-diffblue
Copy link
Contributor

I gather this has been superseded by #1642

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.

5 participants