215688 - Improper configure test for "64-bit OS"

Status: RESOLVED INVALID

(Firefox Build System :: General, defect)

Description

[phil]

User-Agent:       Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.5a) Gecko/20030809 Mozilla Firebird/0.6.1
Build Identifier: Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.5a) Gecko/20030809 Mozilla Firebird/0.6.1

configure has a clause, "checking for 64-bit OS"
This seems to be very inappropriately named. All it does that I can see, is
check if sizeof(long) == 8;

If it is really checking for a "64-bit OS", it should check to see if it
supports 64-bit data with system calls (LP64 type stuff)

If on the other hand, its true purpose is no more than just to check the size of
'long', then it should SAY, "checking for size of long"



Reproducible: Always

Steps to Reproduce:
1. 
2.
3.



Expected Results:  
"checking for 8-bit long" , maybe

Comment 1

[cls]

The point of the test was to check for a 64-bit OS.  sizeof(long)==8 was the
simplest test to determine that.  Mainly out of curiousity, what's the point of
further checking for various LP64 functions across multiple platforms when the
simple test will do?  

Comment 2

[phil]

"The point of the test was to check for a 64-bit OS"

That comment is imprecise. What functionality are you looking for, in a "64bit OS" ?

You should check for that *specific* functionality.

For example, solaris has lots of 64bit file access APIs, even when the kernel is
running in 32-bit mode. So the "simple test" of checking for sizeof(long)==8
does NOT "do", in that case.

Comment 3

[Roland Mainz]

Philip:
What about renaming "checking for 64-bit OS" to "checking for 64-bit compiler" ?

Comment 4

[phil]

I'm not sure that's really what is warranted. Someone needs to go through, see
what flags the configure detect finds, and then rename it appropriately.

But "64 bit compiler" still isnt correct. in theory, someone may have wanted to
check "is the cpu a 64bit cpu". But these days, you shouldnt ever DO that
anyway. you should be using explicit int length types (uint32_t, uint64_t), and
let the compiler figure out the best way to handle things.

Comment 5

[cls]

> You should check for that *specific* functionality.
> 
> For example, solaris has lots of 64bit file access APIs, even when the kernel is
> running in 32-bit mode. So the "simple test" of checking for sizeof(long)==8
> does NOT "do", in that case.

Except that it obviously does "do" because afaik, Mozilla doesn't use any of
that extra 64bit related functionality.  For the specific examples of what the
64bit checks are used for, check lxr for HAVE_64BIT_OS & USE_64.  At a glance,
it seems the most the core Mozilla code cares about is alignment & vtable
issues.  NSPR & NSS may have other issues. 

Is there a real bug here (like that you can't build because solaris was
improperly determined to be 64bit or not) or is this just another
your-definition-doesn't-fit-mine-so-change-it issue?  One obviously is a higher
priority than the other.

Comment 6

[phil]

The issue here is that I dont know what features I am missing out on needlessly,
because the check isnt clear what it does.

Having an inproperly labeled capability IS A BUG. Because in a code base as huge
as mozilla, people just look at the label, and they will make reasonable
assumptions based on the name. 

It is perfectly reasonable for a developer in one of the many submodules of
mozilla, to assume, "If HAVE_64BIT_OS is defined, I can use largefile access,
and if it is not set, then I cannot".  Apart from the fact that the #define
doesnt actually MEAN "64bit OS", it just means "64bit cpu".

There are a bunch of separate things related to cpu size, that should be tested
SEPARATELY:

size of (void*)

size of (int)

size of (long long)

minimum alignment size


As I already commented, you shouldt care about size of int or size of long long. 
The one that *really* counts, is the size of a pointer, aka sizeof(void*)

So any way you look at it, the current test is wrong.
At minimum, it should be changed to test sizeof(void*)==8, to fully test for "a
64 bit OS" the way it seems to be meant.
At best, it should be renamed to 64BIT_PTRSIZE

Hmm. funny thing. looks like there are parts of the codebase that WANT a check
for the LP functions, but there is no autoconf setting for it, so they cant use
it yet.


xpcom/io/nsLocalFileUnix.cpp
nsLocalFile::GetFileSize(PRInt64 *aFileSize)
/* XXX autoconf for and use stat64 if available */

Comment 7

[cls]

*** Bug 293801 has been marked as a duplicate of this bug. ***

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

>It is perfectly reasonable for a developer in one of the many submodules of
>mozilla, to assume, "If HAVE_64BIT_OS is defined, I can use largefile access,
>and if it is not set, then I cannot". 

I disagree. Is windows on x86 a 64-bit OS? Is linux on the same CPU? I don't
think they are, yet they do want to (and can) use largefile functions.

Comment 9

[(not currently active) Ted Mielczarek]

Don't really care about this. This test suits our needs. In general, if we wanted to support 64-bit system calls, we'd have a specific configure test for that.