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

Don't define __*_ENDIAN__ macro on Unix. #495

Merged
merged 1 commit into from
Jan 19, 2022
Merged

Don't define __*_ENDIAN__ macro on Unix. #495

merged 1 commit into from
Jan 19, 2022

Conversation

methane
Copy link
Member

@methane methane commented Jan 19, 2022

@methane methane merged commit 89ea577 into main Jan 19, 2022
@methane methane deleted the endian branch January 19, 2022 05:42
@kulikjak
Copy link
Contributor

Hi, this change broke msgpack (half the test suite) on big endian systems (in our case Solaris SPARC). Now, __LITTLE_ENDIAN is always chosen as default. Is that expected?

I managed to fix it by passing -D__BIG_ENDIAN__=1 to /usr/bin/python3 ./setup.py build but it's not exactly elegant :(.

@methane
Copy link
Member Author

methane commented Jul 28, 2022

Please suggest an elegant way to detect endian.

@kulikjak
Copy link
Contributor

Well, the previous solution of defining it in setup.py based on sys.byteorder seemed pretty nice before the borg issue.

If I understand the original borg issue, was the problem in the cross-compilation (which is why both LITTLE and BIG endians were defined at the same time)? If so, I can see why this is an issue (because msgpack doesn't detect cross compilation).

Maybe setup.py can check whether __BIG_ENDIAN__ or __LITTLE_ENDIAN__ is defined and only set it itself if neither of them is?

@methane
Copy link
Member Author

methane commented Jul 28, 2022

It's not elegant.
C compile time solution is preferred.

@kulikjak
Copy link
Contributor

That's true, not very elegant.

Well, I think __BYTE_ORDER__, which should be defined by most compilers, can be used for compile time detection. I am not sure that it's available everywhere, but I just checked clang 11.0, gcc 7, 10 and 11, and Solaris Studio 12.4, and all of those define it.

The sysdep.h header currently uses #if __BYTE_ORDER == __LITTLE_ENDIAN from arpa/inet.h but those are not standard and not defined on Solaris (or FreeBSD I tested as well), but it can be changed to #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__/#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__, and let compiler to make it right (rather than header file with non-standard defines).

Or, to make it even more compatible, __BYTE_ORDER, __LITTLE_ENDIAN and __BIG_ENDIAN can be defined if they don't exist to __BYTE_ORDER__, __ORDER_LITTLE_ENDIAN__ and __ORDER_BIG_ENDIAN__ respectively.

@kulikjak
Copy link
Contributor

kulikjak commented Aug 1, 2022

I created #513 to address this.

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