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

iox-#459 enable UndefinedBehaviorSanitizer for iceoryx #461

Merged

Conversation

prasannabhat
Copy link
Contributor

@prasannabhat prasannabhat commented Dec 21, 2020

Signed-off-by: Prasanna Bhat [email protected]

Pre-Review Checklist for the PR Author

  1. Branch follows the naming format (iox-#123-this-is-a-branch)
  2. Commits messages are according to this guideline
    • Commit messages have the issue ID (iox-#123 commit text)
    • Commit messages are signed (git commit -s)
    • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
  3. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  4. Relevant issues are linked
  5. Add sensible notes for the reviewer
  6. All checks have passed
  7. Assign PR to reviewer

Notes for Reviewer

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

Post-review Checklist for the Eclipse Committer

  1. All checkboxes in the PR checklist are checked or crossed out
  2. Merge

References

@dkroenke dkroenke self-requested a review December 22, 2020 07:56
@dkroenke dkroenke added the tooling All iceoryx related tooling (scripts etc.) label Dec 22, 2020
@dkroenke dkroenke linked an issue Dec 22, 2020 that may be closed by this pull request
@dkroenke
Copy link
Member

@prasannabhat Can you please merge the latest master and rebuild again?

@prasannabhat prasannabhat changed the title iox-#459 enable UndefinedBehaviorSanitizer for iceoryx core components iox-#459 enable UndefinedBehaviorSanitizer for iceoryx Dec 23, 2020
Copy link
Member

@dkroenke dkroenke left a comment

Choose a reason for hiding this comment

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

Looks quite good!
One last point from my side: Can you add some words about the UB Sanitizer in https://github.com/eclipse/iceoryx/blob/master/doc/website/getting-started/installation.md#use-sanitizer-scan ?

@@ -58,6 +58,8 @@ function(iox_create_asan_compile_time_blacklist BLACKLIST_FILE_PATH)
file(WRITE ${BLACKLIST_FILE_PATH} "# This file is auto-generated from iceoryx_utils/cmake/IceoryxPlatform.cmake\n")
file(APPEND ${BLACKLIST_FILE_PATH} "# src:*file_name.cpp*\n")
file(APPEND ${BLACKLIST_FILE_PATH} "# fun:*Test_Name*\n")
file(APPEND ${BLACKLIST_FILE_PATH} "# posh_runtime.cpp:70:12: runtime error: reference binding to null pointer of type 'iox::runtime::PoshRuntime'\n")
file(APPEND ${BLACKLIST_FILE_PATH} "src:*posh_runtime.cpp\n")
Copy link
Member

Choose a reason for hiding this comment

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

Do you have more information what fails? Maybe we can fix it with this PR

Copy link
Member

Choose a reason for hiding this comment

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

I would keep that separate because it does not belong into this issue: #423

Copy link
Member

Choose a reason for hiding this comment

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

you're right, but maybe it's a minor thing which could be solved within a few minutes.

Copy link
Contributor Author

@prasannabhat prasannabhat Jan 7, 2021

Choose a reason for hiding this comment

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

Here you go
AFAIK there is only one error from UBSan (I think this is a very positve indication of the code quality 👍 )
(Dont know how long these build logs are persisted)

Copy link
Member

Choose a reason for hiding this comment

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

That's really weird. The error doesn't make sense. I tried to reproduce it on my machine, but I just got the reference binding to null pointer error.
I fixed that and removed the runtime from the blacklist. Let's see what happens

Copy link
Member

Choose a reason for hiding this comment

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

Let's see what happens
... well, it's broken :D

@dkroenke the tests for setting the runtime factory are broken. We need a RouDi for this tests, but cannot use the RouDiEnvironment, since it sets the factory itself and when we override that in the test, we break the RouDiEnvironment. I'm wondering why it doesn't crash.

I disabled the test with a note to re-enable them with #449

Copy link
Member

Choose a reason for hiding this comment

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

okay, windows is broken and I have no idea why :/

Copy link
Member

Choose a reason for hiding this comment

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

it builds now ... but I have only slightly an idea why :D

@prasannabhat
Copy link
Contributor Author

Looks quite good!
One last point from my side: Can you add some words about the UB Sanitizer in https://github.com/eclipse/iceoryx/blob/master/doc/website/getting-started/installation.md#use-sanitizer-scan ?

@dkroenke Thanks for introducing the documentation here. I updated UB Sanitizer content.

@dkroenke dkroenke self-requested a review January 7, 2021 08:36
dkroenke
dkroenke previously approved these changes Jan 7, 2021
@dkroenke dkroenke merged commit c0e7ed8 into eclipse-iceoryx:master Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling All iceoryx related tooling (scripts etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable UndefinedBehaviorSanitizer
3 participants