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 sanitizers only on 64bit machines #149

Merged
merged 2 commits into from
Sep 19, 2019
Merged

Conversation

Karsten1987
Copy link
Collaborator

addresses #142

Signed-off-by: Karsten Knese karsten@openrobotics.org

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987 Karsten1987 self-assigned this Aug 21, 2019
@Karsten1987 Karsten1987 mentioned this pull request Aug 21, 2019
@piraka9011
Copy link
Contributor

@tfoote @dirk-thomas please review and merge. Required for Tier2 Armhf support on Dashing.

@Karsten1987
Copy link
Collaborator Author

@piraka9011 An immediate workaround for this is to change the Armhf build configuration to explicitly pass in DISABLE_SANITIZERS=ON as an cmake argument.
See comment here: #142 (comment)

I would like to run a PR job on an armhf build machine first to verify the result of this PR. I currently don't know how to do that though.

@tfoote
Copy link
Contributor

tfoote commented Sep 17, 2019

@Karsten1987 Here's an armhf CI run: build upto rosbag2 test select rosbag2

Build Status

@Karsten1987
Copy link
Collaborator Author

Thanks @tfoote for the build. I somehow must have been ignoring the fact that ci.ros2.org is provisioned with a 32bit armhf job 😇

The referenced build job explicitly disabled asan by passing in the flag. I've triggered another one which uses the logic of this PR.
Build Status

Also, a regular CI job:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Karsten1987
Copy link
Collaborator Author

This is up for review.

@@ -16,6 +16,11 @@ if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
endif()

option(DISABLE_SANITIZERS "disables the use of gcc saniztizers")
# Only enable sanitizers on x64 architectures
# https://github.com/google/sanitizers/issues/794
if(NOT "${CMAKE_SIZEOF_VOID_P}" EQUAL "8")
Copy link
Contributor

Choose a reason for hiding this comment

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

qq: Were you unable to get this running without the quotes? Ideally it would be int not str comaprison.

if(NOT CMAKE_SIZEOF_VOID_P EQUAL 8)
   set(DISABLE_SANITIZERS ON)
endif()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

absolutely right. 3c1ab0d addresses this.

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987
Copy link
Collaborator Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

armhf32:
Build Status

@Karsten1987 Karsten1987 merged commit 37e68ca into master Sep 19, 2019
@Karsten1987 Karsten1987 deleted the fix_142 branch September 19, 2019 03:01
Karsten1987 added a commit that referenced this pull request Sep 19, 2019
* enable address sanitizers only on 64bit machines

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* remove quotes to compare integers

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Karsten1987 added a commit that referenced this pull request Sep 20, 2019
* enable address sanitizers only on 64bit machines

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* remove quotes to compare integers

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@tfoote tfoote added this to Released in Dashing Patch Release 4 Sep 20, 2019
@tfoote tfoote removed this from Released in Dashing Patch Release 4 Sep 20, 2019
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.

None yet

3 participants