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

Fix msgpack_cpp11 tests on 32-bit architectures #829

Merged
merged 2 commits into from
Jan 6, 2020

Conversation

jamessan
Copy link
Contributor

@jamessan jamessan commented Jan 3, 2020

I recently updated the Debian package for msgpack-c to 3.2.1 and found that there were a number of build failures. It appears that all of the 32-bit architectures (except m68k) failed with this error:

[20/122] /usr/bin/c++   -I../build-gtest/install/include -I../include -Iinclude -DMSGPACK_DEFAULT_API_VERSION=3 -std=c++11 -g -O2 -fdebug-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2    -Wall -Wextra -Wconversion -MD -MT test/CMakeFiles/msgpack_cpp11.dir/msgpack_cpp11.cpp.o -MF test/CMakeFiles/msgpack_cpp11.dir/msgpack_cpp11.cpp.o.d -o test/CMakeFiles/msgpack_cpp11.dir/msgpack_cpp11.cpp.o -c ../test/msgpack_cpp11.cpp
FAILED: test/CMakeFiles/msgpack_cpp11.dir/msgpack_cpp11.cpp.o 
/usr/bin/c++   -I../build-gtest/install/include -I../include -Iinclude -DMSGPACK_DEFAULT_API_VERSION=3 -std=c++11 -g -O2 -fdebug-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2    -Wall -Wextra -Wconversion -MD -MT test/CMakeFiles/msgpack_cpp11.dir/msgpack_cpp11.cpp.o -MF test/CMakeFiles/msgpack_cpp11.dir/msgpack_cpp11.cpp.o.d -o test/CMakeFiles/msgpack_cpp11.dir/msgpack_cpp11.cpp.o -c ../test/msgpack_cpp11.cpp
../test/msgpack_cpp11.cpp: In member function ‘virtual void MSGPACK_TIMESPEC_timespec_pack_convert_32bit_sec_Test::TestBody()’:
../test/msgpack_cpp11.cpp:1081:36: error: narrowing conversion of ‘4294967295’ from ‘long unsigned int’ to ‘__time_t’ {aka ‘long int’} [-Wnarrowing]
 1081 |     timespec val1{ 0xffffffffUL, 0 };
      |                                    ^
../test/msgpack_cpp11.cpp: In member function ‘virtual void MSGPACK_TIMESPEC_timespec_object_with_zone_32bit_sec_Test::TestBody()’:
../test/msgpack_cpp11.cpp:1097:36: error: narrowing conversion of ‘4294967295’ from ‘long unsigned int’ to ‘__time_t’ {aka ‘long int’} [-Wnarrowing]
 1097 |     timespec val1{ 0xffffffffUL, 0 };
      |                                    ^

Since this test is only run as part of the c++11 suite, I created this PR to ensure there's c++11 coverage (both 64 and 32-bit) for the CI runs.

Please let me know if you'd rather add two new jobs to the matrix rather than re-purposing existing ones.

@jamessan
Copy link
Contributor Author

jamessan commented Jan 3, 2020

CI / Linux (1) shows the same failure now:

2020-01-03T02:11:52.2175428Z /home/runner/work/msgpack-c/msgpack-c/test/msgpack_cpp11.cpp:1085:20: error: constant expression evaluates to 4294967295 which cannot be narrowed to type '__time_t' (aka 'long') [-Wc++11-narrowing]
2020-01-03T02:11:52.2176133Z     timespec val1{ 0xffffffffUL, 0 };
2020-01-03T02:11:52.2176418Z                    ^~~~~~~~~~~~
2020-01-03T02:11:52.2177087Z /home/runner/work/msgpack-c/msgpack-c/test/msgpack_cpp11.cpp:1085:20: note: insert an explicit cast to silence this issue
2020-01-03T02:11:52.2177477Z     timespec val1{ 0xffffffffUL, 0 };
2020-01-03T02:11:52.2177736Z                    ^~~~~~~~~~~~
2020-01-03T02:11:52.2177974Z                    static_cast<__time_t>( )
2020-01-03T02:11:52.2178646Z /home/runner/work/msgpack-c/msgpack-c/test/msgpack_cpp11.cpp:1085:20: warning: implicit conversion changes signedness: 'unsigned long' to '__time_t' (aka 'long') [-Wsign-conversion]
2020-01-03T02:11:52.2178973Z     timespec val1{ 0xffffffffUL, 0 };
2020-01-03T02:11:52.2179210Z                  ~ ^~~~~~~~~~~~

@jamessan
Copy link
Contributor Author

jamessan commented Jan 3, 2020

Feel free to push to my branch if you'd like to fix the issues in this PR. I added some commits to address the issues that came up.

@jamessan jamessan force-pushed the cxx11-32bit branch 5 times, most recently from 42cbc3d to 36c2032 Compare January 3, 2020 13:22
@redboltz
Copy link
Contributor

redboltz commented Jan 6, 2020

You added export CXX11="ON" to two of CI matrix.
But they are for C++03 test. So I don't want to add export CXX11="ON" there.

I expected that if I define CXX17="ON" in the CI matrix, then C++11 and C++17 tests are enabled. But it only enables C++17 tests.

I will apply your fix for test/msgpack_cpp11.cpp.
And will update from IF (MSGPACK_CXX11) to IF (MSGPACK_CXX11 OR MSGPACK_CXX17) at

IF (MSGPACK_CXX11)

instead of applying your .github/workflows/gha.yml fix.

This covers 64bit CXX11 at the pattern 2 and 32bit CXX11 at the pattern 3.

It that OK?

@jamessan
Copy link
Contributor Author

jamessan commented Jan 6, 2020

You added export CXX11="ON" to two of CI matrix.
But they are for C++03 test. So I don't want to add export CXX11="ON" there.

Yeah, I was trying to avoid expanding the matrix more. I can understand not wanting to take that change.

It that OK?

Sounds good. I'll remove the gha.yml change from this branch.

@jamessan jamessan changed the title Add CI testing for CXX11 Fix msgpack_cpp11 tests on 32-bit architectures Jan 6, 2020
On 32-bit unix platforms, 0xffffffffUL is a 32-bit value so the compiler
complains about converting it to a signed value.

    /home/runner/work/msgpack-c/msgpack-c/test/msgpack_cpp11.cpp:1085:20: error: constant expression evaluates to 4294967295 which cannot be narrowed to type '__time_t' (aka 'long') [-Wc++11-narrowing]
        timespec val1{ 0xffffffffUL, 0 };
                       ^~~~~~~~~~~~
    /home/runner/work/msgpack-c/msgpack-c/test/msgpack_cpp11.cpp:1085:20: note: insert an explicit cast to silence this issue
        timespec val1{ 0xffffffffUL, 0 };
                       ^~~~~~~~~~~~
                       static_cast<__time_t>( )
    /home/runner/work/msgpack-c/msgpack-c/test/msgpack_cpp11.cpp:1085:20: warning: implicit conversion changes signedness: 'unsigned long' to '__time_t' (aka 'long') [-Wsign-conversion]
        timespec val1{ 0xffffffffUL, 0 };
                     ~ ^~~~~~~~~~~~

Since we're trying to test how the maximum 32-bit value that fits in
timespec.tv_sec is handled, directly use the maximum 32-bit value for
the appropriate (un)signed type used for timespec.tv_sec.

We don't just cast to the value, as the compiler suggests, because that
would result in an extremely negative value.
@redboltz redboltz merged commit f8b0ad1 into msgpack:master Jan 6, 2020
@redboltz
Copy link
Contributor

redboltz commented Jan 6, 2020

@jamessan , thank you. merged.

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