Skip to content

Conversation

@elbeno
Copy link
Contributor

@elbeno elbeno commented Sep 1, 2023

No description provided.

@elbeno
Copy link
Contributor Author

elbeno commented Sep 1, 2023

This is mostly formulaic, but the UB sanitizer turned up some issues in 64-bit tests.

@elbeno
Copy link
Contributor Author

elbeno commented Sep 1, 2023

There is also an issue with tests that don't have any "real" google tests, but only rapidcheck tests. They don't play well with gtest_discover_tests (I assume reporting no tests) - the result is that they will rebuild every build... adding at least one (possibly degenerate) "plain test" would fix this.

@lukevalenty
Copy link
Contributor

This is mostly formulaic, but the UB sanitizer turned up some issues in 64-bit tests.

Can you share which tests had this issue? Was it an issue in the test itself using 64-but primitives, or in safe arithmetic?

If a test hit 64-bit overflow for negation, that should fail the test because the big_integer should have the correct value while the 64-bit primitive will overflow or have other UB. Right?

@elbeno
Copy link
Contributor Author

elbeno commented Sep 1, 2023

It is a problem with the tests, not the library. And the test doesn't fail; it triggers UBSan. For example https://github.com/intel/safe-arithmetic/blob/main/test/safe/big_integer/detail/plus.cpp#L37 when the generated values are such that a + b (in regular C++ artihmetic) overflows. This is true for the plus, minus, and multiply tests, and the negate test. And it makes sense that rapidcheck should generate boundary conditions...

@lukevalenty
Copy link
Contributor

Looks like I need to update the branch protection rules on the repo. 😅

@elbeno
Copy link
Contributor Author

elbeno commented Sep 1, 2023

Should be building now... clang-format reordering headers produced a failure I guess.

@elbeno
Copy link
Contributor Author

elbeno commented Sep 1, 2023

Tests are "passing" (see ASan/TSan tests), but failing because of CTest interaction with rapidcheck & the compile-fail tests. And of course the UBSan tests have a couple of issues. I'm assuming this isn't holding up the actual merge though and we can fix this all up in future PRs?

Some tests cause integer overflow: they need to be guarded by `RC_PRE`.
This fixes signed 64-bit negation, addition and subtraction.
Multiplication still has this problem.
auto-merge was automatically disabled September 1, 2023 23:35

Head branch was pushed to by a user without write access

clang-tidy is temporarily turned off, to be turned on in a separate fix.
@lukevalenty lukevalenty added this pull request to the merge queue Sep 2, 2023
Merged via the queue into intel:main with commit 18b00c5 Sep 2, 2023
@elbeno elbeno deleted the update-ci branch September 2, 2023 21:39
@lukevalenty lukevalenty mentioned this pull request Sep 5, 2023
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.

2 participants