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 endianness bug in write_digit2_separated #2699

Merged
merged 3 commits into from
Jan 4, 2022
Merged

Conversation

phprus
Copy link
Contributor

@phprus phprus commented Jan 4, 2022

Fix issue #2698

Need testing on big-endian platform.

CC @xvitaly

@xvitaly
Copy link
Contributor

xvitaly commented Jan 4, 2022

I can confirm this PR fixes the issue:

+ /usr/bin/ctest --output-on-failure --force-new-ctest-process -j3
Test project /builddir/build/BUILD/fmt-8.1.0/redhat-linux-build
      Start  1: args-test
      Start  2: assert-test
      Start  3: chrono-test
 1/19 Test  #1: args-test ........................   Passed    0.00 sec
      Start  4: color-test
 2/19 Test  #2: assert-test ......................   Passed    0.00 sec
      Start  5: core-test
 3/19 Test  #4: color-test .......................   Passed    0.00 sec
      Start  6: gtest-extra-test
 4/19 Test  #5: core-test ........................   Passed    0.00 sec
      Start  7: format-test
 5/19 Test  #6: gtest-extra-test .................   Passed    0.00 sec
      Start  8: format-impl-test
 6/19 Test  #8: format-impl-test .................   Passed    0.01 sec
      Start  9: ostream-test
 7/19 Test  #7: format-test ......................   Passed    0.01 sec
      Start 10: compile-test
 8/19 Test  #9: ostream-test .....................   Passed    0.00 sec
      Start 11: compile-fp-test
 9/19 Test #11: compile-fp-test ..................   Passed    0.00 sec
      Start 12: printf-test
10/19 Test #10: compile-test .....................   Passed    0.00 sec
      Start 13: ranges-test
11/19 Test #13: ranges-test ......................   Passed    0.00 sec
      Start 14: scan-test
12/19 Test #12: printf-test ......................   Passed    0.01 sec
      Start 15: unicode-test
13/19 Test #15: unicode-test .....................   Passed    0.00 sec
      Start 16: xchar-test
14/19 Test #14: scan-test ........................   Passed    0.00 sec
      Start 17: enforce-checks-test
15/19 Test #17: enforce-checks-test ..............   Passed    0.00 sec
      Start 18: posix-mock-test
16/19 Test #16: xchar-test .......................   Passed    0.00 sec
      Start 19: os-test
17/19 Test #18: posix-mock-test ..................   Passed    0.00 sec
18/19 Test #19: os-test ..........................   Passed    0.01 sec
19/19 Test  #3: chrono-test ......................   Passed    0.06 sec
100% tests passed, 0 tests failed out of 19
Total Test time (real) =   0.06 sec

@phprus phprus marked this pull request as ready for review January 4, 2022 13:03
Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Comment on lines 561 to 577
#ifndef _WIN32
# if defined(__GNUC__) && __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
digits = __builtin_bswap64(digits);
# else
if (is_big_endian()) {
digits = ((digits & 0xff00000000000000ull) >> 56) |
((digits & 0x00ff000000000000ull) >> 40) |
((digits & 0x0000ff0000000000ull) >> 24) |
((digits & 0x000000ff00000000ull) >> 8) |
((digits & 0x00000000ff000000ull) << 8) |
((digits & 0x0000000000ff0000ull) << 24) |
((digits & 0x000000000000ff00ull) << 40) |
((digits & 0x00000000000000ffull) << 56);
}
# endif
#endif
memcpy(buf, &digits, 8);
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using std::reverse_copy instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
And improve is_big_endian() with using of predefined macro. This check is compile time now if it possible (to avoid many is_big_endian() calls on little endian system and on GCC like compilers).

Comment on lines 298 to 305
#ifdef _WIN32
constexpr auto is_big_endian() -> bool { return false; }
#elif defined(__BIG_ENDIAN__)
constexpr auto is_big_endian() -> bool { return true; }
#elif defined(__BYTE_ORDER__) && defined(__ORDER_BIG_ENDIAN__)
constexpr auto is_big_endian() -> bool {
return __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__;
}
#else
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest moving the ifdefs into the body of is_big_endian to avoid repeating its signature. I don't think we care about constexpr here, it will be inlined and optimized away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 563 to 564
memcpy(tmp, &digits, 8);
std::reverse_copy(tmp, tmp + 8, buf);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not std::reverse_copy(digits, digits + 8, buf)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

digits is unsigned long long.
Without memcpy into temp buffer this is UB.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, indeed. Let's at least turn 8 into a constant then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@vitaut vitaut merged commit 214cf13 into fmtlib:master Jan 4, 2022
@vitaut
Copy link
Contributor

vitaut commented Jan 4, 2022

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

Successfully merging this pull request may close these issues.

3 participants