OpenTTD

Tasklist

FS#4623 - Assumptions that sizeof(void *) is 4 bytes breaking 64 bit built (mingw64)

Attached to Project: OpenTTD
Opened by J G Rennison (JGR) - Sunday, 22 May 2011, 07:57 GMT
Last edited by Jose Soler (Terkhen) - Wednesday, 25 May 2011, 16:51 GMT
Type Bug
Category Core
Status Closed
Assigned To No-one
Operating System All
Severity Low
Priority Normal
Reported Version trunk
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

At the top of trunk/src/3rdparty/squirrel/include/squirrel.h, SQInteger, SQUnsignedInteger and SQHash are incorrectly typedefd to be (unsigned) long instead of (unsigned) long long for non MSVC compilers. This will cause an error when the code tries to store a pointer in said integer type.

#ifdef _SQ64
#ifdef _MSC_VER
typedef __int64 SQInteger;
typedef unsigned __int64 SQUnsignedInteger;
typedef unsigned __int64 SQHash; /*should be the same size of a pointer*/
#else
typedef long SQInteger;
typedef unsigned long SQUnsignedInteger;
typedef unsigned long SQHash; /*should be the same size of a pointer*/
#endif
typedef int SQInt32;
#else
typedef int SQInteger;
typedef int SQInt32; /*must be 32 bits(also on 64bits processors)*/
typedef unsigned int SQUnsignedInteger;
typedef unsigned int SQHash; /*should be the same size of a pointer*/
#endif

In trunk/src/settings.cpp:IniSaveSettings, "unsigned long" should probably be "size_t", as otherwise mingw64 halts with a precision loss error.

case SDT_MANYOFMANY:
switch (GetVarMemType(sld->conv)) {
case SLE_VAR_BL:
if (*(bool*)ptr == (p != NULL)) continue;
break;
case SLE_VAR_I8:
case SLE_VAR_U8:
if (*(byte*)ptr == (byte)(unsigned long)p) continue;
break;
case SLE_VAR_I16:
case SLE_VAR_U16:
if (*(uint16*)ptr == (uint16)(unsigned long)p) continue;
break;
case SLE_VAR_I32:
case SLE_VAR_U32:
if (*(uint32*)ptr == (uint32)(unsigned long)p) continue;
break;
default: NOT_REACHED();
}

Compilation using mingw64 gcc 20110516 pre-release 4.5.4
This task depends upon

Closed by  Jose Soler (Terkhen)
Wednesday, 25 May 2011, 16:51 GMT
Reason for closing:  Fixed
Additional comments about closing:  In r22489, r22490 and r22491.
Comment by Remko Bijker (Rubidium) - Sunday, 22 May 2011, 08:42 GMT
Well... more the assumption that longs are as big as pointers on all GCC platforms as it works perfectly fine on my AMD64 (and compiles fine on all of Debian's 64 bits platforms). In any case, the first thing is more a "library" issue, as squirrel is developed externally and somewhat modified by us. The question is what to use as "long long" is a C++-0x feature and as such might not be supported by the somewhat older but still supported compilers.

The latter shouldn't be much of a problem at all; they are a byte, uint16 or uint32 in the p variable (okay pointer). Even then, if mingw64 uses the same pointer/long sizes as MSVC64, then why doesn't MSVC complain as it complains quite often when stuff works fine with GCC.
Comment by J G Rennison (JGR) - Sunday, 22 May 2011, 15:59 GMT
Having looked into it it seems that for 64 bit targets, on standard Unixy platforms longs are 64 bits and on standard Windows they're 32 bit, which is inconvenient.
Mingw64 also defines an __int64, however the typedef is in a header file, which at least is included at the top of every std header file from what I can see.
Plonking an #include <limits.h> or whatever and changing the ifdefs from _MSC_VER to that or mingw would work, though somewhat clumsy.

As for the latter, there may well be a compiler switch to turn the error off that I've not found yet, however it seems reasonable to object to trying to store a 64 bit pointer in a 32 bit integer, even if you then typecast that to a smaller integer.


There's also an issue I've just noticed with trunk/src/os/windows/crashlog_win.cpp:CrashLog::InitialiseCrashLog() implicitly assuming that all non-MSVC compilers are 32-bit (asm referring to esp), but I'll have a go at fixing that.
Edit:
This can be sorted by simply re-arranging the order of ifdefs slightly from:
#if defined(_MSC_VER)
#ifdef _M_AMD64
CONTEXT ctx;
RtlCaptureContext(&ctx);

/* The stack pointer for AMD64 must always be 16-byte aligned inside a
* function. As we are simulating a function call with the safe ESP value,
* we need to subtract 8 for the imaginary return address otherwise stack
* alignment would be wrong in the called function. */
_safe_esp = (void *)(ctx.Rsp - 8);
#else
_asm {
mov _safe_esp, esp
}
#endif
#else
asm("movl %esp, __safe_esp");
#endif

to:

#ifdef _M_AMD64

CONTEXT ctx;
RtlCaptureContext(&ctx);

/* The stack pointer for AMD64 must always be 16-byte aligned inside a
* function. As we are simulating a function call with the safe ESP value,
* we need to subtract 8 for the imaginary return address otherwise stack
* alignment would be wrong in the called function. */
_safe_esp = (void *)(ctx.Rsp - 8);
#else
#if defined(_MSC_VER)
_asm {
mov _safe_esp, esp
}
#else
asm("movl %esp, __safe_esp");
#endif
#endif
Comment by Jose Soler (Terkhen) - Sunday, 22 May 2011, 19:56 GMT
Can you please upload the required changes as a diff/patch file? It is quite complicated to review that code.
Comment by J G Rennison (JGR) - Sunday, 22 May 2011, 21:25 GMT
Patch attached.
Not necessarily the best way to do it, but should illustrate the issue.
Comment by Jose Soler (Terkhen) - Monday, 23 May 2011, 20:06 GMT
The patch file seems to be broken.

patching file src/3rdparty/squirrel/include/squirrel.h
patching file src/os/windows/crashlog_win.cpp
patch: **** malformed patch at line 38: @@ -559,13 +559,14 @@

Did you test if crash logs are generated correctly with your diff?
Comment by J G Rennison (JGR) - Monday, 23 May 2011, 21:38 GMT
Whitespace issue it seems. This one should be fine.

As for testing the crash log, now that you've mentioned it, I've so far not been able to make it produce any kind of crash log at all, which is potentially another issue, though crashing it by a number of methods has not been difficult. Will look into that in detail when I have more time (later).
Comment by Jose Soler (Terkhen) - Tuesday, 24 May 2011, 13:58 GMT
I finally managed to set up MinGW64 properly. I get lots of warnings during OpenTTD compilation coming from zlib and lzo. The warnings seem to hint that these libraries do not support MinGW64.

Regarding your patch: I get a lot of warnings at os/windows/crashlog_win.cpp, specifically at the part of the code that logs the registers. Also, the modifications of settings.cpp are no longer required (see r22489).

[SRC] Compiling os/windows/crashlog_win.cpp
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp: In member function 'virtual char* CrashLogWindows::LogRegisters(char*, const char*) const':
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: unknown conversion type character 'l' in format [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: unknown conversion type character 'l' in format [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: unknown conversion type character 'l' in format [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: unknown conversion type character 'l' in format [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: unknown conversion type character 'l' in format [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: unknown conversion type character 'l' in format [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: unknown conversion type character 'l' in format [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: unknown conversion type character 'l' in format [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: unknown conversion type character 'l' in format [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: unknown conversion type character 'l' in format [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: unknown conversion type character 'l' in format [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: unknown conversion type character 'l' in format [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: unknown conversion type character 'l' in format [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: unknown conversion type character 'l' in format [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: unknown conversion type character 'l' in format [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: unknown conversion type character 'l' in format [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: unknown conversion type character 'l' in format [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: format '%X' expects argument of type 'unsigned int', but argument 4 has type 'DWORD64' [-Wformat]
C:/mingw64/msys/home/terkhen/fs/src/os/windows/crashlog_win.cpp:269:2: warning: too many arguments for format [-Wformat-extra-args]
Comment by J G Rennison (JGR) - Tuesday, 24 May 2011, 16:25 GMT
The PRIX64 macro in inttypes.h defines the correct format type specifier for __int64, which is I64X.
Inconveniently that header file is not defined in MSVC though.
However, according to the various MSVC docs, the correct format is also I64X, not llX. Which implies that MSVC should also frown upon the existing code?
Will try to get my visual studio installation working again to test this...
Comment by Jose Soler (Terkhen) - Tuesday, 24 May 2011, 16:49 GMT
With the current trunk code, MSVC has no warnings when compiling 64 bits builds. See: http://binaries.openttd.org/binaries/nightlies/trunk/r22489/logs/windows-win64-compile.log

My guess is that Mingw64 does it differently than MSVC.
Comment by J G Rennison (JGR) - Tuesday, 24 May 2011, 17:44 GMT
Both mingw and MSVC use the MSVC runtime.
I don't get any errors/warnings when compiling using MSVC with the above patch applied and it produces a correctly formatted crashlog.
Interestingly on crashlog_win.cpp:116, %.16IX, a third formatting code, is used for a 64 bit int, and that doesn't raise any warnings.
Edit: see stdafx.h:277

They all seem to work fine, so I would guess that mingw64 is just being pedantic with the warnings and/or the warning scanner supports less format codes than the runtime.


Edit: removed stuff about crashlog generation which was wrong...
Comment by Jose Soler (Terkhen) - Tuesday, 24 May 2011, 19:12 GMT
mingw64_fixes_3.patch and mingw64_crashlog3.patch conflict with each other, so I can't test with Mingw64.

With mingw64_crashlog3.patch there are no warnings in MSVC with either 32 or 64 builds, but it makes me wonder which format is right and which one is wrong. I have learned to not trust mingw on implementing the MSVC runtime 100% correctly; some stuff is missing, and other things are made in gcc style instead.
Comment by J G Rennison (JGR) - Tuesday, 24 May 2011, 19:29 GMT
I've merged the two patches together, and removed the stuff for settings.cpp

As for the crash log not being generated, it seems that this is an issue with the mingw64 CRT/exception support, not an issue in the OTTD source. http://sourceforge.net/tracker/?func=detail&aid=2958949&group_id=202880&atid=983355
It would be cumbersome (though quite possible) to work round that, but frankly it's not really worth doing.

The MSVC documentation says to use I64, and I would suggest that that (or just I) would be the best way to do it, as every other part of the code already uses it, and the gcc-way, ll, causes warnings on mingw.
Comment by Jose Soler (Terkhen) - Tuesday, 24 May 2011, 20:31 GMT
That bug report is quite outdated. There are already some MinGW64 builds using gcc 4.6, and they should have SEH support. I'm using this one:

http://sourceforge.net/projects/mingw-w64/files/Toolchains%20targetting%20Win64/Personal%20Builds/rubenvb/

With that build of MinGW64 and the mingw64_fixes_4.patch, crash.log seems to be generated correctly.

Loading...