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

Test 'UnsignedComparisons' and 'FitsIn' failed #347

Closed
simon28li opened this issue May 28, 2021 · 9 comments
Closed

Test 'UnsignedComparisons' and 'FitsIn' failed #347

simon28li opened this issue May 28, 2021 · 9 comments
Assignees

Comments

@simon28li
Copy link

Hi,The error in the execution of sealtest is as follows.
Can you help locate the cause of the error?
Thanks a lot.

[ RUN      ] Common.Constants
[       OK ] Common.Constants (0 ms)
[ RUN      ] Common.UnsignedComparisons
/home/enable/SEAL/native/tests/seal/util/common.cpp:53: Failure
Value of: unsigned_eq(neg_c, pos_ull_max)
  Actual: false
Expected: true
[  FAILED  ] Common.UnsignedComparisons (0 ms)
[ RUN      ] Common.SafeArithmetic
[       OK ] Common.SafeArithmetic (0 ms)
[ RUN      ] Common.FitsIn
/home/enable/SEAL/native/tests/seal/util/common.cpp:109: Failure
Value of: fits_in<char>(pos_uc_max)
  Actual: true
Expected: false
[  FAILED  ] Common.FitsIn (0 ms)
[ RUN      ] Common.DivideRoundUp


 ...


[----------] Global test environment tear-down
[==========] 255 tests from 41 test suites ran. (983 ms total)
[  PASSED  ] 253 tests.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] Common.UnsignedComparisons
[  FAILED  ] Common.FitsIn

 2 FAILED TESTS

Steps to reproduce the issue

$ cmake -S . -B build -DSEAL_BUILD_TESTS=ON -DSEAL_BUILD_BENCH=ON -DSEAL_BUILD_EXAMPLES=ON
$ cmake --build build -j16
$ cd build/bin
$ ./sealtest

Information on your system

$ uname -a
Linux localhost.localdomain 4.18.0-193.el8.aarch64 #1 SMP Fri May 8 11:05:12 UTC 2020 aarch64 aarch64 aarch64 GNU/Linux
[root@localhost bin]# cat /etc/os-release
NAME="CentOS Linux"
VERSION="8 (Core)"
ID="centos"
ID_LIKE="rhel fedora"
VERSION_ID="8"
PLATFORM_ID="platform:el8"
PRETTY_NAME="CentOS Linux 8 (Core)"
ANSI_COLOR="0;31"
CPE_NAME="cpe:/o:centos:centos:8"
HOME_URL="https://www.centos.org/"
BUG_REPORT_URL="https://bugs.centos.org/"

CENTOS_MANTISBT_PROJECT="CentOS-8"
CENTOS_MANTISBT_PROJECT_VERSION="8"
REDHAT_SUPPORT_PRODUCT="centos"
REDHAT_SUPPORT_PRODUCT_VERSION="8"


@WeiDaiWD
Copy link
Contributor

WeiDaiWD commented May 28, 2021

On ARM64, char is a 8-bit unsigned type. The first failure is because that on other architectures neg_c is -1 (equal to pos_ull_max once converted to unsigned long long) while on ARM64 neg_c is 255 (equals to 255 once converted to unsigned long long). The second failure is that because on other architectures char is -128 ~ 127 (pos_uc_max does not fit in this) while on ARM64 char is 0 ~ 255 (pos_uc_max does fit in this).

SEAL still supports ARM64. Please ignore these two failures for now. I've just checked all appearances of char in SEAL, and they are all safe. I'll remove these two failed tests in the next minor release. Thank you very much for reporting this!

@WeiDaiWD WeiDaiWD self-assigned this May 28, 2021
@WeiDaiWD
Copy link
Contributor

Actually, I might just make these tests architecture-dependent. Please keep this issue open.

@simon28li
Copy link
Author

On ARM64, char is a 8-bit unsigned type. The first failure is because that on other architectures neg_c is -1 (equal to pos_ull_max once converted to unsigned long long) while on ARM64 neg_c is 255 (equals to 255 once converted to unsigned long long). The second failure is that because on other architectures char is -128 ~ 127 (pos_uc_max does not fit in this) while on ARM64 char is 0 ~ 255 (pos_uc_max does fit in this).

SEAL still supports ARM64. Please ignore these two failures for now. I've just checked all appearances of char in SEAL, and they are all safe. I'll remove these two failed tests in the next minor release. Thank you very much for reporting this!

Thank you very much!

@WeiDaiWD
Copy link
Contributor

WeiDaiWD commented Jun 8, 2021

I've fixed this internally. It will be release in 3.6.6. Thanks again!

@WeiDaiWD WeiDaiWD closed this as completed Jun 8, 2021
@WeiDaiWD
Copy link
Contributor

WeiDaiWD commented Jun 9, 2021

@simon28li Would you kindly test this fix for me? I don't have access to ARM64. The branch is fix-arm64. Thank you so much!

@simon28li
Copy link
Author

@WeiDaiWD It seems that there is still a use case that fails.

[----------] 10 tests from Common
[ RUN      ] Common.Constants
[       OK ] Common.Constants (0 ms)
[ RUN      ] Common.UnsignedComparisons
/home/enable/SEAL-fix_arm64/SEAL/native/tests/seal/util/common.cpp:55: Failure
Value of: unsigned_lt(pos_uc_max, neg_c)
  Actual: false
Expected: true
[  FAILED  ] Common.UnsignedComparisons (0 ms)

...

[----------] Global test environment tear-down
[==========] 255 tests from 41 test suites ran. (1002 ms total)
[  PASSED  ] 254 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] Common.UnsignedComparisons

 1 FAILED TEST

@WeiDaiWD
Copy link
Contributor

WeiDaiWD commented Jun 9, 2021

I've just pushed to the same branch. Please test again. Much appreciated!

@simon28li
Copy link
Author

@WeiDaiWD
It's great. It all worked.

[       OK ] UIntCore.CompareUInt (0 ms)
[ RUN      ] UIntCore.GetPowerOfTwo
[       OK ] UIntCore.GetPowerOfTwo (0 ms)
[ RUN      ] UIntCore.DuplicateUIntIfNeeded
[       OK ] UIntCore.DuplicateUIntIfNeeded (0 ms)
[----------] 18 tests from UIntCore (0 ms total)

[----------] Global test environment tear-down
[==========] 255 tests from 41 test suites ran. (979 ms total)
[  PASSED  ] 255 tests.

@WeiDaiWD
Copy link
Contributor

Great. Thank you!

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

No branches or pull requests

2 participants