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

DorpsGek · 2011-05-22T09:57:03Z

JGR opened the ticket and wrote:

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

Reported version: trunk
Operating system: All

This issue was imported from FlySpray: https://bugs.openttd.org/task/4623

The text was updated successfully, but these errors were encountered:

DorpsGek · 2011-05-22T10:42:47Z

Rubidium wrote:

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.

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9943

DorpsGek · 2011-05-22T17:59:20Z

JGR wrote:

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

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9944

DorpsGek · 2011-05-22T21:56:45Z

Terkhen wrote:

Can you please upload the required changes as a diff/patch file? It is quite complicated to review that code.

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9946

DorpsGek · 2011-05-22T23:25:43Z

JGR wrote:

Patch attached.
Not necessarily the best way to do it, but should illustrate the issue.

Attachments

mingw64_fixes.patch (2.37 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9950

DorpsGek · 2011-05-23T22:06:06Z

Terkhen wrote:

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?

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9951

DorpsGek · 2011-05-23T23:38:47Z

JGR wrote:

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).

Attachments

mingw64_fixes_3.patch (2.53 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9952

DorpsGek · 2011-05-24T15:58:53Z

Terkhen wrote:

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]

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9953

DorpsGek · 2011-05-24T18:25:18Z

JGR wrote:

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...

Attachments

mingw64_crashlog3.patch (1.47 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9954

DorpsGek · 2011-05-24T18:49:15Z

Terkhen wrote:

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.

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9955

DorpsGek · 2011-05-24T19:44:37Z

JGR wrote:

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...

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9956

DorpsGek · 2011-05-24T21:12:34Z

Terkhen wrote:

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.

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9957

DorpsGek · 2011-05-24T21:29:30Z

JGR wrote:

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.

Attachments

mingw64_fixes_4.patch (2.49 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9958

DorpsGek · 2011-05-24T22:31:59Z

Terkhen wrote:

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.

Attachments

crash.log (8.02 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9959

DorpsGek · 2011-05-25T18:51:49Z

Terkhen closed the ticket.

Reason for closing: Fixed

In r22489, r22490 and r22491.

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623

DorpsGek closed this as completed May 25, 2011

DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) labels Apr 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

DorpsGek commented May 22, 2011

DorpsGek commented May 22, 2011

DorpsGek commented May 22, 2011

DorpsGek commented May 22, 2011

DorpsGek commented May 22, 2011

DorpsGek commented May 23, 2011

DorpsGek commented May 23, 2011

DorpsGek commented May 24, 2011

DorpsGek commented May 24, 2011

DorpsGek commented May 24, 2011

DorpsGek commented May 24, 2011

DorpsGek commented May 24, 2011

DorpsGek commented May 24, 2011

DorpsGek commented May 24, 2011

DorpsGek commented May 25, 2011

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

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

Comments

DorpsGek commented May 22, 2011

This issue was imported from FlySpray: https://bugs.openttd.org/task/4623

DorpsGek commented May 22, 2011

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9943

DorpsGek commented May 22, 2011

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9944

DorpsGek commented May 22, 2011

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9946

DorpsGek commented May 22, 2011

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9950

DorpsGek commented May 23, 2011

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9951

DorpsGek commented May 23, 2011

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9952

DorpsGek commented May 24, 2011

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9953

DorpsGek commented May 24, 2011

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9954

DorpsGek commented May 24, 2011

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9955

DorpsGek commented May 24, 2011

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9956

DorpsGek commented May 24, 2011

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9957

DorpsGek commented May 24, 2011

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9958

DorpsGek commented May 24, 2011

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623#comment9959

DorpsGek commented May 25, 2011

This comment was imported from FlySpray: https://bugs.openttd.org/task/4623