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 builds on x32 platform. #1152

Closed
wants to merge 1 commit into from
Closed

Fix builds on x32 platform. #1152

wants to merge 1 commit into from

Conversation

jmallach
Copy link
Contributor

@jmallach jmallach commented Jul 29, 2016

From the Debian wiki: https://wiki.debian.org/X32Port

X32 is an ABI for amd64/x86_64 CPUs using 32-bit integers, longs
and pointers. The idea is to combine the smaller memory and cache
footprint from 32-bit data types with the larger register set of
x86_64. The 64-bit registers can make computation more efficient,
and with 8 additional registers available, there is less pressure
compared to i386/i686.

rapidjson makes an incorrect assumption in a check for 64 bit platforms,
and uses LP64 exclusively. This fix adds an additional check for
x86_64 && ILP32 defines, as a very conservative fix. However, the
usage of LP64 would be a problem for other "mixed" applications like
ARM ILP32, so a better detection scheme might be needed in the future.

From the Debian wiki: https://wiki.debian.org/X32Port

X32 is an ABI for amd64/x86_64 CPUs using 32-bit integers, longs
and pointers. The idea is to combine the smaller memory and cache
footprint from 32-bit data types with the larger register set of
x86_64. The 64-bit registers can make computation more efficient,
and with 8 additional registers available, there is less pressure
compared to i386/i686.

rapidjson makes an incorrect assumption in a check for 64 bit
platforms, and uses __LP64__ exclusively. This fix adds an
additional check for __x86_64__ && __ILP32__ defines, as a very
conservative fix. However, the usage of __LP64__ would be a problem
for other "mixed" applications like ARM ILP32, so a better detection
scheme might be needed in the future.
@jmallach
Copy link
Contributor Author

jmallach commented Aug 1, 2016

After discussing with @cuavas, I've duplicated this pull request with rapidjson upstream:

Tencent/rapidjson#703

@jmallach
Copy link
Contributor Author

jmallach commented Aug 2, 2016

This fix has been applied upstream. Tencent/rapidjson@323a0dc

@rb6502
Copy link
Contributor

rb6502 commented Aug 7, 2016

We've now synced upstream rapidjson, is this still necessary?

@jmallach
Copy link
Contributor Author

jmallach commented Aug 7, 2016

This has been merged, yes. a8ebc11#diff-ad70ca65ccd212456aaca0efb96cd072L253

@jmallach jmallach closed this Aug 7, 2016
@rb6502
Copy link
Contributor

rb6502 commented Aug 8, 2016

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

Successfully merging this pull request may close these issues.

2 participants