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

Support Big Endian Linux platform (z/Linux) #136

Closed
wants to merge 1 commit into from

Conversation

MookThompson
Copy link
Contributor

The fixes are broadly in 3 areas:
o use of memcpy to copy between char* and integer* types. In my PR, I created a 'tvintmemcpy', which #defines to the normal memcpy on LE platforms (so no runtime changes), but does a crude byte-reversed memcpy on BE ones. I then went through and changed any memcpy calls where one param is an integer pointer and the other is a char* to use this new tvintmemcpy. This issue affects TermColor::operator= and TermColor::operator uint32_t, CpTranslator::toPackedUtf8, utf32To8, TCellChar::moveInt, and the struct colorconv_r::colorconv_r(TermColor aColor...) constructor. string_as_int has a similar issue, but I've had to address it slightly differently. I've noticed that platform/far2l.cpp seems to have 12 memcpy calls which look like they would need to also be replaced by tvintmemcpy, but I'm unable to test this so haven't made these changes.

o the optimisation of fast_btoa, as the comment says, is only suitable for LE platforms as the btoa_lut_elem_t is a compound struct (3 chars and a uint8). In my PR, I've provided a non-optimised version under the BE #define, which also (probably unnecessarily) null-terminates the destination buffer.

o The struct KeyDownEvent relies on a union of ushort and CharScanType types, but the order of the elements of the CharScanType are reversed on BE platforms. To minimise the diff, I've just reversed the order of the charCode and scanCode members of the struct CharScanType under the TV_BIG_ENDIAN define.

The fixes are broadly in 3 areas:
  o  use of memcpy to copy between char* and integer* types. In my PR, I created a 'tvintmemcpy', which #defines to the normal memcpy on LE platforms (so no runtime changes), but does a crude byte-reversed memcpy on BE ones. I then went through and changed any memcpy calls where one param is an integer pointer and the other is a char* to use this new tvintmemcpy. This issue affects TermColor::operator= and TermColor::operator uint32_t, CpTranslator::toPackedUtf8, utf32To8, TCellChar::moveInt, and the struct colorconv_r::colorconv_r(TermColor aColor...) constructor. string_as_int has a similar issue, but I've had to address it slightly differently. I've noticed that platform/far2l.cpp seems to have 12 memcpy calls which look like they would need to also be replaced by tvintmemcpy, but I'm unable to test this so haven't made these changes.

  o  the optimisation of fast_btoa, as the comment says, is only suitable for LE platforms as the btoa_lut_elem_t is a compound struct (3 chars and a uint8). In my PR, I've provided a non-optimised version under the BE #define, which also (probably unnecessarily) null-terminates the destination buffer.

  o  The struct KeyDownEvent relies on a union of ushort and CharScanType types, but the order of the elements of the CharScanType are reversed on BE platforms. To minimise the diff, I've just reversed the order of the charCode and scanCode members of the struct CharScanType under the TV_BIG_ENDIAN define.
@magiblot magiblot closed this in f933af5 Jan 21, 2024
@magiblot
Copy link
Owner

Hi Mark!

Thank you very much for submitting this PR and excuse me for the late response.

I grabbed the following changes from it:

  • The macro name (TV_BIG_ENDIAN).
  • The CMake endianess auto-detection (since I didn't know the TestBigEndian script existed, I was going to use check_cxx_source_compiles, but I guess it's better to use the CMake-provided method).

As for the rest, it turns out it is not always necessary to reverse the bytes when copying. For example, TCellChar::moveInt is only used for copying an integer which had been previously bit-casted from a byte array, such as the result of CpTranslator::toPackedUtf8. In this case the comment was not clarifying enough.

In fast_btoa I found a way to keep the optimization without using bit-casting, so I used that instead.

If you are interested, commit f933af5 contains all the changes.

@MookThompson
Copy link
Contributor Author

Hi magiblot, thanks for the quick response and the work on this. I'll get the latest version and give it a test, but I'm sure it'll be fine - your changes and tests look very comprehensive.

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