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

Detect endianness at configuration with Autoconf AC_C_BIGENDIAN feature #400

Merged
merged 1 commit into from Jun 12, 2014

Conversation

kdeme
Copy link
Contributor

@kdeme kdeme commented Jun 11, 2014

Currently IEEE_8087 (little endian) is defined as default in "jv_dtoa.c". From my understanding, to be able to use jq on big endian architecture you have to manually change this default define.

So to be able to use jq on bigendian architectures without changing code we need to be able to configure endianness. This patch does that.

FYI, compiling for bigendian, without changing the default define would result in numbers not being parsed correctly or even causing the jq application to hang.

@nicowilliams nicowilliams added this to the 1.5 release milestone Jun 11, 2014
@nicowilliams
Copy link
Contributor

I suppose this does no harm, so I will probably pull it anyways, with a comment to the effect that we know this isn't good enough.

@kdeme
Copy link
Contributor Author

kdeme commented Jun 12, 2014

I didn't know about this until now but it seems it is indeed possible for words to be little-endian but then the words of a floating point to be big-endian (and probably vice versa). Apparently this can be so because endianness is not defined in IEEE 754. I don't know much about this so I am not sure if this is something common or rather exotic. But I would guess it is a pretty rare architecture nowadays?

Anyway, the patch supplied does not really alter the behaviour. It will just make the already existing defines configurable. If there are issues with floating point endianness they will be the same with or without this change. So I think it is safe indeed to pull this, unless I am missing something.

I had a very quick look at the code in jv_dtoa.c and I think that if we would want to support this special case we would have to make an extra define for this code:

#ifdef IEEE_8087
#define word0(x) (x)->L[1]
#define word1(x) (x)->L[0]
#else
#define word0(x) (x)->L[0]
#define word1(x) (x)->L[1]
#endif

It looks like this is the macro that swaps around the words of the double. So instead of IEEE_8087 we would need an extra define here. I only had a very quick look and the code is quite a read so I am not sure this would be enough. As you mentioned we would still have to foresee a way in autoconf to define this, as I couldn't find a macro for this either (https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Floating-Point-Portability.html).

@nicowilliams
Copy link
Contributor

I'll merge this now. I'll add some comments about how to do this right later. Ideally it'd be a one-time run-time test, but I wouldn't want to add a pointless branch (yeah, early optimization), mostly because there'd be a lot of places to sprinkle that around. In practice the best fix will be to add a test program for this, invoke it from autoconf, and define the right macro as a result. (Incidentally, the macro names from jv_dtoa.c don't make good interfaces... but that's another story, for another day.)

nicowilliams added a commit that referenced this pull request Jun 12, 2014
Heuristic IEEE754 endianness autoconf detection

Use AC_C_BIGENDIAN, though it's not really the correct approach.  Autoconf ought to have provided a test specifically for IEEE754 endianness, since it can differ from integer/pointer endianness!
@nicowilliams nicowilliams merged commit 15c4a7f into jqlang:master Jun 12, 2014
@kdeme
Copy link
Contributor Author

kdeme commented Jun 13, 2014

Alright, thanks for merging!

And I agree that the macro names are not that good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants