Opened 9 years ago

Closed 8 years ago

#979 closed defect (fixed)

chicken.h wrongly assumes that __x86_64__ implies 64 bits

Reported by: Ivan Raikov Owned by:
Priority: major Milestone: 4.9.0
Component: build system Version: 4.8.x
Keywords: Cc:
Estimated difficulty:

Description

This bug was filed by a Debian maintainer against the Chicken 4.8.0 source:

In chicken.h, the following line:

# if defined (__alpha__) || defined(__ia64__) || defined(__x86_64__) || defined(__LP64__) || defined(__powerpc64__)

contains both a zoology of systems, and the actual feature it is looking
for.  The zoology is wrong: if the CPU is capable of 64 bit code, this does
not mean that the architecture you're building for has 64 bit pointers.
Because of compatibility with plenty of code that assumes sizeof(long) ==
sizeof(void*), x32 has longs of only 32 bits.  What you want to check here,
is whether longs are 64 bit.

A fix:
change that line to:

# ifdef __LP64__

This lets chicken build on both amd64 and x32.


Change History (9)

comment:1 Changed 9 years ago by felix winkelmann

Hm. This fix looks like its compiler-dependent: is LP64 defined everywhere on a 64-bit machine? And what platforms are meant with this well-meant suggestion? (I assume MING64 isn't which is the only one I'm aware of because only stupid OSs that are paranoid and pathologic about backwards-compatibility take that half-assed, dangerous and error-prone measure to maintain the illusion that you can simply recompile your 32-bit code on such a system and expect things to work).

In other words, what should we do?

comment:2 in reply to:  1 Changed 9 years ago by Ivan Raikov

Since this was filed by a Debian developer, it may very well be that only Linux platforms are meant by this suggestion. And actually, I am able to compile Chicken 4.8.x on both 64-bit and 32-bit systems in Debian testing, so I need to ask for clarification on the specific platform and compiler version that is causing compilation errors.

Replying to felix:

Hm. This fix looks like its compiler-dependent: is LP64 defined everywhere on a 64-bit machine? And what platforms are meant with this well-meant suggestion? (I assume MING64 isn't which is the only one I'm aware of because only stupid OSs that are paranoid and pathologic about backwards-compatibility take that half-assed, dangerous and error-prone measure to maintain the illusion that you can simply recompile your 32-bit code on such a system and expect things to work).

In other words, what should we do?

comment:3 Changed 8 years ago by sjamaan

Ping Ivan. Any news on this?

comment:4 Changed 8 years ago by felix winkelmann

Resolution: wontfix
Status: newclosed

comment:5 Changed 8 years ago by Ivan Raikov

Resolution: wontfix
Status: closedreopened

OK, we finally have an answer from the Debian developers:

Hi!

Somehow, I did not get the BTS mail, and looking in my mail server's logs, I
see no matches. Hrm.

Thank you for the suggestion, but for which platforms is this intended?

At least x32 with gcc and/or clang.

Is LP64 always defined, or is it specific to compiler and platform?

Googling around, I see certain old proprietary compilers use _LP64 instead,
if you care about them.

Windows doesn't have the LP64 define, but that's right as win64 doesn't
use the LP64 model but LLP64: sizeof(long) == 4, while sizeof(void*) == 8.
The current check in chicken is inconsistent: on mingw64 before this patch
chicken.h will define SIXTY_FOUR but on MSVC it will not. I'm not sure
whether you care about sizeof(long) or sizeof(void*) here, but it's obvious
you want one or the other; this patch did not intend to make a change here
but accidentally it makes mingw64 follow MSVC.

I think this covers all modern platforms then.

comment:6 Changed 8 years ago by sjamaan

What, concretely, needs to happen next?

comment:7 in reply to:  6 Changed 8 years ago by Ivan Raikov

Well, the Debian developer is suggesting to change this line in chicken.h:

# if defined (alpha)
defined(ia64) defined(x86_64) defined(LP64) defined(powerpc64)

to

# ifdef LP64

If we agree that this is sensible, then we should make the change and test it on x32 with gcc, clang and the windows stuff.

Replying to sjamaan:

What, concretely, needs to happen next?

comment:8 Changed 8 years ago by sjamaan

OK, thanks for clarifying. I've sent a patch to chicken-hackers, so hopefully we can close this soon.

comment:9 Changed 8 years ago by sjamaan

Resolution: fixed
Status: reopenedclosed

This went in as 37cf50fe7f4dd2335fa330ab9538d245f1f58a06

Note: See TracTickets for help on using tickets.