Skip to content

Commit

Permalink
CONFIGURE: Add detection for 64-bitness and define int64 when applicable
Browse files Browse the repository at this point in the history
  • Loading branch information
@sev-
sev- committed Jan 8, 2012
1 parent d66bfb1 commit 0024228
Showing 1 changed file with 51 additions and 0 deletions.
51 changes: 51 additions & 0 deletions configure
View file
Expand Up @@ -1705,6 +1705,35 @@ EOF
echo $datatype
}

#
# Check whether the system is 32-bit
#
pointer_is_32bit() {
cat > tmp_pointer_is_32bit.cpp << EOF
int main() {
void *p;
int v = (int)p;
return 0;
}
EOF
$CXX $CXXFLAGS -c -o $TMPO.o tmp_pointer_is_32bit.cpp 2>/dev/null
status=$?
cc_check_clean tmp_pointer_is_32bit.cpp
return $status
}

echo_n "Checking 64-bitness... "
pointer_is_32bit
if test $? -eq 0; then
type_ptr=int32
echo "no"
add_line_to_config_h "/* #define SCUMM_64BITS */"
else
type_ptr=int64
echo "yes"
add_line_to_config_h "#define SCUMM_64BITS"
fi

#
# Determine data type sizes
#
Expand All @@ -1726,6 +1755,17 @@ TMPR="$?"
echo "$type_4_byte"
test $TMPR -eq 0 || exit 1 # check exit code of subshell

echo_n "Type with 8 bytes... "
type_8_byte=`find_type_with_size 8`
TMPR="$?"
echo "$type_8_byte"
if test $TMPR -eq 0; then
_def_64bit_type_signed="typedef signed $type_8_byte int64;"
_def_64bit_type_unsigned="typedef unsigned $type_8_byte uint64;"
fi
# force cleanup after check for 8 bytes type
cc_check_clean tmp_find_type_with_size.cpp

#
# Check whether memory alignment is required
#
Expand Down Expand Up @@ -3853,6 +3893,17 @@ typedef signed $type_1_byte int8;
typedef signed $type_2_byte int16;
typedef signed $type_4_byte int32;
/* 64-bit stuff */
$_def_64bit_type_signed
#if defined(__APPLE__) && !defined(__ppc__)
#ifndef _UINT64
#define _UINT64

This comment has been minimized.

Sign in to view

Copy link
@fingolfin

fingolfin Jan 9, 2012

Contributor

Why is this mysterious macro being defined on Mac OS X? I can't think of a reason, but I am sure you had a good one, but shouldn't this then be explained in a comment?

This comment has been minimized.

Sign in to view

Copy link
@clone2727

clone2727 via email Jan 9, 2012

Contributor

This comment has been minimized.

Sign in to view

Copy link
@fingolfin

fingolfin Jan 10, 2012

Contributor

Removing this extra code yields build errors in two files:

In file included from /System/Library/Frameworks/Security.framework/Headers/Security.h:25,
from /System/Library/Frameworks/Foundation.framework/Headers/NSURLCredential.h:15,
from /System/Library/Frameworks/Foundation.framework/Headers/Foundation.h:80,
from /System/Library/Frameworks/Cocoa.framework/Headers/Cocoa.h:13,
from backends/platform/sdl/macosx/appmenu_osx.mm:30:
/System/Library/Frameworks/Security.framework/Headers/cssmconfig.h:53: error: conflicting declaration ‘typedef uint64_t uint64’
./config.h:58: error: ‘uint64’ has a previous declaration as ‘typedef long unsigned int uint64’

The other error is identical and in gui/browser_osx.mm. So one header in the Security framework defines uint64 (and uint32, uint16) in an incompatible way: It uses uint64_t, which is "unsigned long long", as opposed to what ScummVM uses ("unsigned long").

But this problem only occurs when one includes OS X specific headers, i.e. only in OS X specific source code. Wouldn't it make more sense to remove this OS X specific hack here, and instead add appropriate workarounds to the affected OS X specific files?

However this is handled, though: The reason for the workaround should be documented in a comment, shouldn't it?

This comment has been minimized.

Sign in to view

Copy link
@fingolfin

fingolfin Jan 10, 2012

Contributor

By the way, there are 64bit compilers where "long long" is 64bit and long is only 32bit (and of course there are other compilers which don't have any "long long" type. Maybe ScummVM should thus try to use "long long", too? Or perhaps its time to start using stdint.h and the types it provides by default, only falling back to self defined types if this is missing? (Now that even MSVC includes stdint.h ...) This should minimize conflicts with system headers...

This comment has been minimized.

Sign in to view

Copy link
@lordhoto

lordhoto Jan 10, 2012

Contributor

Which version of MSVC does contain stdint.h? If we would use stdint.h, we would definitly need to provide a fallback for MSVC8/9/10, which we still support and some developers still use.

For configure systems this might be a bit easier, i.e. try to detect if stdint.h is available and usable and if not just use our old type detection.

PS: Win64 actually should be a platform where only long long is 64bit.

This comment has been minimized.

Sign in to view

Copy link
@fingolfin

fingolfin Jan 10, 2012

Contributor

MSVC 2010 includes stdint.h

This comment has been minimized.

Sign in to view

Copy link
@lordhoto

lordhoto Jan 10, 2012

Contributor

Yeah fuzzie told me so on IRC already, still we need it for MSVC 8/9.

This comment has been minimized.

Sign in to view

Copy link
@bluegr

bluegr Jan 10, 2012

Member

There's this which can be used for MSVC:
http://msinttypes.googlecode.com/svn/trunk/stdint.h

it looks as if it'll work in all of the MSVC versions... but the copyright notice on top looks a bit too much for a header file

Alternatively, we could only include this header file for the appropriate MSVC versions (i.e. if _MSC_VER >= 1000) and keep the defines for older versions?

This comment has been minimized.

Sign in to view

Copy link
@bluegr

bluegr Jan 10, 2012

Member

Sorry, by "this" I mean the stdint.h file included with MSVC10, not the linked stdint.h

This comment has been minimized.

Sign in to view

Copy link
@criezy

criezy Jan 18, 2012

Member

I have an issue here: with this _UINT64 define I do get error messages
/System/Library/Frameworks/Security.framework/Headers/cssmtype.h:39: error: ‘uint64’ does not name a type
/System/Library/Frameworks/Security.framework/Headers/cssmtype.h:43: error: ‘CSSM_LONG_HANDLE’ does not name a type
/System/Library/Frameworks/Security.framework/Headers/cssmtype.h:163: error: ‘uint64’ does not name a type
/System/Library/Frameworks/Security.framework/Headers/cssmtype.h:164: error: ‘CSSM_PRIVILEGE’ does not name a type
/System/Library/Frameworks/Security.framework/Headers/cssmtype.h:1708: error: ‘CSSM_LONG_HANDLE’ does not name a type

and I have a lot of other similar messages in several other system files.
If I remove this define everything works fine.

I suspect this is linked to the fact that configure does not find a 64 bits type on my system and therefore uint64 is not defined in config.h:
Type with 8 bytes... couldn't find data type with 8 bytes

Maybe a check that $_def_64bit_type_unsigned is not empty should be added before defining _UINT64?
But maybe the issue is actually that the detection of a 8 bytes data type fails (I checked that long long is 64 bits but configure apparently doesn't detect that).

This is on 32 bits MacOS X 10.6

This comment has been minimized.

Sign in to view

Copy link
@criezy

criezy Jan 18, 2012

Member

A bit more info:
The code used in find_type_with_size() works OK but the issue is that configure compiles its test programs with -pedantic with gives the following error when compiling find_test_with_size.cpp with long long:
error: ISO C++ does not support ‘long long’

This comment has been minimized.

Sign in to view

Copy link
@lordhoto

lordhoto Jan 18, 2012

Contributor

If your compiler does not support long long that should be no problem really. I suspect the problem you encounter is that we seem to define _UINT64, even when we are not on a 64bit OS X and thus even when we do not need to define any uint64 type on our own.

A quick woraround would be to only define _UINT64 when we actually define a 64bit unsigned integer type. I think adding that for now might make sense.

Personally I think it's really time to consider to use stdint.h. As I got Eugene, we really only need that uint64 type for pointer casts in the Pluto code. So if we would use stdint.h and use (u)intptr_h for that it might be an better idea. I will look into what is needed to make a safe transition this evening. We still have the problem that (u)intptr_t is optional, but I think for all our target systems they should be available.
$_def_64bit_type_unsigned
#endif
#else
$_def_64bit_type_unsigned
#endif
#endif /* CONFIG_H */
EOF

Expand Down

1 comment on commit 0024228

@fingolfin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, indention in this patch is different from the rest of configure: It uses 8 spaces per level, instead of 1 tab (of width 4)

Please sign in to comment.