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

Fix asan and undefined behavior error [10916] #1845

Merged
merged 3 commits into from
Apr 6, 2021
Merged

Fix asan and undefined behavior error [10916] #1845

merged 3 commits into from
Apr 6, 2021

Conversation

princeward
Copy link
Contributor

@princeward princeward commented Mar 23, 2021 •
edited
Loading

Fix two error reported by AddressSanitizer and UndefinedBehaviorSanitizer.

The AddressSanitizer error is trivial.

For the UndefinedBehaviorSanitizer error, left shifting a uint64 by 64 bits is considered undefined behavior. See this article for reference.

This is take 2 of an earlier PR: #1834

@princeward
Fix asan and undefined behavior error, take 2
c270b67 
Signed-off-by: Zijian Wang <zjwang@fb.com>
@princeward princeward mentioned this pull request Mar 23, 2021
Closed
@princeward
Copy link
Contributor Author

princeward commented Mar 23, 2021 •
edited
Loading

Hi @MiguelCompany , thanks for reviewing in an previous version of this PR: #1834. I opened this new PR since I messed up with DCO signoff in the previous one.

I applied your suggestion and verified that the new change will pass both Address and UndefinedBehavior sanitizer now :D

Please let me what else is needed to merge this PR. Thank you!

MiguelCompany
MiguelCompany previously approved these changes Mar 23, 2021
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

LGTM

@MiguelCompany MiguelCompany changed the title Fix asan and undefined behavior error, take 2 Fix asan and undefined behavior error [10916] Mar 23, 2021
@princeward
Copy link
Contributor Author

Hi @MiguelCompany , thanks for approving. I see that some CI tests are failing. Are these legitimate concerns and actually caused by my PR?

@MiguelCompany
Copy link
Member

I have to check, but there are two tests that are failing on all platforms: BlackboxTests_DDS_PIM.DDSDataSharing.BestEffortDirtyPayloads and BlackboxTests_DDS_PIM.DDSDataSharing.ReliableDirtyPayloads which I have never seen fail. Will keep you posted

@MiguelCompany
Copy link
Member

@richiprosima Please test this

@IkerLuengo
Centralise the generation of the default DataSharing domain ID
cf8c162 
The code to generate the default DataSharing domain ID has been
extracted to a common place to avoid code duplication and maintenance.

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>
@IkerLuengo
Copy link
Contributor

The piece of code making the bit shifting was used in several places, but only some of them were modified on this PR, breaking the logic and making the tests fail. Commit cf8c162 centralizes this code in a single place and solved the testing errors.

@IkerLuengo
Resolve test linker errors
1b122e8 
Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>
MiguelCompany
MiguelCompany requested changes Apr 6, 2021
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

LGTM with green CI.

@MiguelCompany
Copy link
Member

Thank you @princeward and @IkerLuengo

@MiguelCompany MiguelCompany merged commit 4d067d0 into eProsima:master Apr 6, 2021
@IkerLuengo IkerLuengo mentioned this pull request Apr 7, 2021
Closed
IkerLuengo added a commit that referenced this pull request Apr 7, 2021
@IkerLuengo
Fix asan and undefined behavior error
01916d8 
This is a port of #1845 from <master> to <2.2.x>

* Fix asan and undefined behavior error, take 2

Signed-off-by: Zijian Wang <zjwang@fb.com>

* Centralise the generation of the default DataSharing domain ID

The code to generate the default DataSharing domain ID has been
extracted to a common place to avoid code duplication and maintenance.

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Resolve test linker errors

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

Co-authored-by: Iker Luengo <ikerluengo@eprosima.com>
Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>
@IkerLuengo IkerLuengo mentioned this pull request Apr 7, 2021
Merged
MiguelCompany pushed a commit that referenced this pull request Apr 8, 2021
@IkerLuengo
Fix asan and undefined behavior error (#1889)
56d51e9 
This is a port of #1845 from <master> to <2.2.x>

* Fix asan and undefined behavior error, take 2

Signed-off-by: Zijian Wang <zjwang@fb.com>

* Centralise the generation of the default DataSharing domain ID

The code to generate the default DataSharing domain ID has been
extracted to a common place to avoid code duplication and maintenance.

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Resolve test linker errors

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

Co-authored-by: Iker Luengo <ikerluengo@eprosima.com>
Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MiguelCompany MiguelCompany MiguelCompany approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@princeward @MiguelCompany @IkerLuengo