Bug 158646 - scorched3d not 64bit clean
scorched3d not 64bit clean
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: scorched3d (Show other bugs)
rawhide
x86_64 Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 161694 FE5Target
  Show dependency treegraph
 
Reported: 2005-05-24 11:27 EDT by Jeremy Katz
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-02-11 16:54:19 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Preliminary x86_64 fixes (12.26 KB, patch)
2005-05-24 15:07 EDT, Toshio Kuratomi
no flags Details | Diff
my patch, for reference (12.21 KB, patch)
2005-05-25 16:14 EDT, Jeremy Katz
no flags Details | Diff

  None (edit)
Description Jeremy Katz 2005-05-24 11:27:00 EDT
Fails to rebuild on x86_64 due to int/pointer mismatches

http://extras64.linux.duke.edu/failed/development/scorched3d/37.2-3/x86_64/scorched3d-37.2-3.failure.log
Comment 1 Toshio Kuratomi 2005-05-24 15:07:18 EDT
Created attachment 114792 [details]
Preliminary x86_64 fixes

Here's a preliminary patch.  It gets scorched3d to build and run as far as the
menu and preferences.  I don't know about getting further -- I don't have a
working 3d card right now and I'm getting errors about GLX that I assume mean I
need that.  (If not, I'll open another ticket or research upstream.)

I think there are some problems with this patch but I need someone more
knowledgable about 64bit issues to confirm.  When we have code like:

int foo(void *ptr) {
 return (int)ptr;
}

I think it should be converted to:
long foo(void *ptr) {
 return (long)ptr;
}

This is what's been done in this patch.  I think it also has to address calling
code:

void main() {
 int temp;
 temp = foo();
}

and change to:

void main() {
 long temp;

 temp = foo();
}

Which hasn't been done yet.

Also, should pointers all be unsigned long?  Or does long suffice?
Comment 2 Jeremy Katz 2005-05-25 13:02:09 EDT
Yes, pointers are long on 64bit arches.

A different approach to take might be to do like glib does and define a macro
like the following (and this is the anaconda version)
/* 64 bit platforms, definitions courtesy of glib */
#if defined (__x86_64__) || defined(__ia64__) || defined(__alpha__) ||
defined(__powerpc64__) || defined(__sparc64__) || defined(__s390x__)
#define POINTER_TO_INT(p)  ((int) (long) (p))
#define INT_TO_POINTER(i)  ((void *) (long) (i))
#else
#define POINTER_TO_INT(p)  ((int) (p))
#define INT_TO_POINTER(i)  ((void *) (i))
#endif

Then you just use that instead of the casting and it handles things properly
without having to change any of the return types (similar macros can be done for
the unsigned versions as well)
Comment 3 Jeremy Katz 2005-05-25 16:07:22 EDT
Hmmm, even with these changes it still doesn't really work beyond the settings
dialog.

For now, I'm thinking an excludearch might make sense.  The code looks like it's
littered with int/long assumptions that you really can't make :(
Comment 4 Jeremy Katz 2005-05-25 16:14:00 EDT
Created attachment 114850 [details]
my patch, for reference
Comment 5 Panu Matilainen 2005-05-31 05:38:43 EDT
Hmm.. Scorced CVS doesn't seem to have anything to help the situation either,
best to make it excludearch for now as I don't expect to have this sorted out by
FC4 release by any means.

Feel free to commit that excudearch change, I still don't have CVS access :-/
Comment 6 John Ellson 2005-06-12 11:36:11 EDT
Re comment #2.  Why not just "#include <stdint.h>" and use "intptr_t" ?
Comment 7 Jeremy Katz 2005-06-12 12:00:42 EDT
Because a number of upstream projects aren't ready to commit to C99.  That may
be (slowly) changing as I think the new Debian stable should have a new enough
compiler, but it has traditionally been a problem as I've pushed 64bit fixes
upstream.  To the point that I stopped trying to go the c99 route
Comment 8 Ville Skyttä 2005-10-11 15:23:17 EDT
I've updated the devel branch to 39.1 in CVS but not tagged yet, could someone 
check if it still needs patching for ExcludeArch before I request builds of 
it?  Feel free to commit whatever is needed. 
  
(You'll need a new openal-devel from devel CVS to build 39.1; it's been built  
but not yet published.)  
Comment 9 Hans de Goede 2006-02-05 03:54:05 EST
I've taken ownership of scorched3d and will look into this (as time permits).
Comment 10 Wart 2006-02-07 23:52:02 EST
I just managed to build this on FC-4 x86_64 using the devel sources.  I only had
to change BR: wxGTK-devel to wxGTK2-devel to get it to build on FC4.

I was able to start it up and begin a game without any problems.
Comment 11 Hans de Goede 2006-02-08 16:26:16 EST
Hi Wart,

Thanks for looking, I know it compiles and runs kinda sorta in local play mode,
but internet play is (was?) severly hoosed.

It has taken me a lot of fiddling and looking at the code to get this fixed, but
I'm about to commit a new 64bit patch to fe-cvs devel tree which should also fix
internet play, both hosting a server and as a client. If you want I can give you
the gory details, I need to type them out for upstream anyways.
Comment 12 Hans de Goede 2006-02-10 08:27:42 EST
This is fixed in FE-CVS devel tree now, I'll psuh an update when I also have
fixed the security issue reported in bug 161694.

As promised here is an explanation of the fix as send upstream:

The problem is NetServer.cpp and the destinationID's used elsewhere, destination
ID's are unsigned int and thus are 32 bits even on a 64 bit arch. Since these
also get exchanged over the network and I did't want to break (network)
compatibility with the existing release I've choosen to keep the destinationID's
unsigned int, which also minimizes changes outside of NetServer.cpp . The
problem is that NetServer.cpp generates this ideas by casting TCPsocket to
unsigned int, as TCPsocket is a ptr to a struct TCPsocket_, this means that
NetServer is putting ptr's into ints this won't work on 64 bits since ints are
only 32 bits and addresses are 64 bit, thus this will loose a part of the address.

NetServer uses NetServerRead todo most of the work and when NetServer gets
called it uses a std::map<TCPsocket, NetServerRead *> to find the NetServerRead
to use. I've changed this to an std::map<unsigned int, NetServerRead *>. And I
generate unique ideas in addClient. Thus removing the ugly and on 64 bit defect
storing of TCPsocket addresses in unsigned int. For the rare case where the
TCPsocket (getIpAddress) is actually needed by NetServer I've added a
getSocket() to NetServerRead, so NetServer can get to the socket asociated with
the destinationID.

Since NetServerRead and NetServerXXXProtocol need access to both the socket and
the destinationId and those are no longer the same I've extended NetServerRead
to get passed both the socket and the destID on create and I've extended the
relevant NetServerXXXProtocol methods to have both a socket and a destID as
parameters.

Besides that there are a few places where s3d stores integers in pointers, but
except for needing a cast to compile on 64 bit this is no problem.
Comment 13 Hans de Goede 2006-02-11 16:54:19 EST
scorched3d-39.1-2 has been built for devel / FC5 which fixes this.

Note You need to log in before you can comment on or make changes to this bug.