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

Automate Address & Thread sanitization #2782

Merged
merged 12 commits into from
Aug 8, 2017
Merged

Automate Address & Thread sanitization #2782

merged 12 commits into from
Aug 8, 2017

Conversation

ironage
Copy link
Contributor

@ironage ironage commented Aug 3, 2017

  • Add support for building with thread and address sanitizers via cmake
  • Automate these build modes so CI will run them for each PR
  • Fix several errors/races in unit tests reported in these modes
  • Track which unit tests are run by enabling UNITTEST_PROGRESS when testing

@ironage ironage self-assigned this Aug 3, 2017
@ironage ironage force-pushed the js/ci-improvements branch from f8a650d to c502e23 Compare August 3, 2017 18:52
@realm-ci
Copy link
Contributor

realm-ci commented Aug 3, 2017

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2782/3/Performance_Report

@realm-ci
Copy link
Contributor

realm-ci commented Aug 3, 2017

Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2782/3/Diff_Coverage

@realm-ci
Copy link
Contributor

realm-ci commented Aug 3, 2017

Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2782/4/Diff_Coverage

@realm-ci
Copy link
Contributor

realm-ci commented Aug 3, 2017

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2782/4/Performance_Report

emanuelez
emanuelez reviewed Aug 3, 2017
View reviewed changes
Copy link
Contributor

@emanuelez emanuelez left a comment

Choose a reason for hiding this comment

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

I think this PR needs to be rethought a bit in light of the fact that we already have a cmake toolchain for many specialty builds.

CMakeLists.txt Outdated
@@ -104,12 +104,20 @@ option(REALM_ENABLE_ASSERTIONS "Enable assertions in release mode." OFF)
option(REALM_ENABLE_ALLOC_SET_ZERO "Zero all allocations." OFF)
option(REALM_ENABLE_ENCRYPTION "Enable encryption." ON)
option(REALM_ENABLE_MEMDEBUG "Add additional memory checks" OFF)
option(REALM_SANITIZE_ADDRESS "Enable the GCC/Clang 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.

No need for this. There is already a toolchain for specialty builds: https://github.com/realm/realm-core/blob/master/tools/cmake/SpecialtyBuilds.cmake

CMakeLists.txt Outdated
set(REALM_MAX_BPNODE_SIZE "1000" CACHE STRING "Max B+ tree node size.")

if (CMAKE_SYSTEM_NAME MATCHES "^Windows")
set(REALM_ENABLE_ENCRYPTION OFF)
endif()

if(REALM_SANITIZE_THREAD)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed either

@ironage
Copy link
Contributor Author

ironage commented Aug 3, 2017

@emanuelez thanks for pointing out the specialty builds file, I forgot about it! I changed to use those built in flags now.

@realm-ci
Copy link
Contributor

realm-ci commented Aug 3, 2017

Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2782/5/Diff_Coverage

@realm-ci
Copy link
Contributor

realm-ci commented Aug 3, 2017

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2782/5/Performance_Report

@realm-ci
Copy link
Contributor

realm-ci commented Aug 3, 2017

Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2782/6/Diff_Coverage

@realm-ci
Copy link
Contributor

realm-ci commented Aug 3, 2017

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2782/6/Performance_Report

emanuelez
emanuelez approved these changes Aug 4, 2017
View reviewed changes
Copy link
Contributor

@emanuelez emanuelez left a comment

Choose a reason for hiding this comment

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

Just a minor comment. Good job!

Jenkinsfile Outdated
@@ -151,24 +152,29 @@ def buildDockerEnv(name) {
return docker.image(name)
}

def doBuildInDocker(String buildType) {
def doBuildInDocker(String buildType, String sanitizeMode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you used a default value for the parameter you could avoid passing an empty string when calling this function:

def doBuildInDocker(String buildType, String sanitizeMode='') {

@realm-ci
Copy link
Contributor

realm-ci commented Aug 4, 2017

Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2782/7/Diff_Coverage

@realm-ci
Copy link
Contributor

realm-ci commented Aug 4, 2017

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2782/7/Performance_Report

@realm-ci
Copy link
Contributor

realm-ci commented Aug 4, 2017

Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2782/8/Diff_Coverage

@realm-ci
Copy link
Contributor

realm-ci commented Aug 4, 2017

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2782/8/Performance_Report

finnschiermer
finnschiermer approved these changes Aug 7, 2017
View reviewed changes
Copy link
Contributor

@finnschiermer finnschiermer left a comment

Choose a reason for hiding this comment

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

Nice!

@emanuelez
Copy link
Contributor

How about the memory sanitizer? Is it in the plans?

@ironage ironage merged commit 3985339 into master Aug 8, 2017
@ironage ironage removed the S:Review label Aug 8, 2017
@ironage ironage deleted the js/ci-improvements branch August 8, 2017 20:32
@ironage
Copy link
Contributor Author

ironage commented Aug 8, 2017

@emanuelez though it may have value to run a memory sanitizer on each PR, I think our valgrind job is probably sufficient for uninitialised reads.

@ironage ironage mentioned this pull request Aug 10, 2017
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@finnschiermer finnschiermer finnschiermer approved these changes

@emanuelez emanuelez emanuelez approved these changes

Assignees

@ironage ironage

Labels
T-Test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ironage @realm-ci @emanuelez @finnschiermer