Opened 15 years ago
Closed 15 years ago
#2649 closed defect (fixed)
CPLScanPointer doesn't do its job rightly when compiled in MSYS environment
Reported by: | Ari Jolma | Owned by: | warmerdam |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | ConfigBuild | Version: | unspecified |
Severity: | normal | Keywords: | mingw |
Cc: | Mateusz Łoskot, dron |
Description
CPLScanPointer uses
#if defined(WIN32) && defined(_MSC_VER)
to test whether to give sscanf a hex without the 0x prefix. The requirement is (probably) a feature of sscanf in msvcrt.dll. This does not work for MSYS, which does not define _MSC_VER. Instead MSYS (MinGW gcc?) defines MSVCRT to indicate that it will be using msvcrt.dll (and not msys-1.0.dll).
The question is now how to fix this. In this case the issue is the runtime that is used and not the compiler. If _MSC_VER checks for the compiler, then it shouldn't be used here. See similar discussion here: http://mail.gnome.org/archives/xml/2002-October/msg00002.html Maybe we should use MSVCRT every time the runtime is the issue and somewhere in configuration add
#ifdef _MSC_VER #define __MSVCRT__ #endif
Change History (9)
comment:1 by , 15 years ago
comment:2 by , 15 years ago
I'm not against testing sscanf in runtime.
Meanwhile I've found out that there's an issue with printf in MSYS. While it has long long because it's gcc, it requires %I64d because it uses msvcrt. This causes problems for example in tif_print.c. There's a nice abstraction cpl_port.h, which could be exploited and we wouldn't need those defines in tif_print.c and elsewhere, there's on for example cpl_conv.cpp. (Maybe libtiff in GDAL is downstream and the logics in cpl_port.h should be copied).
BTW, I don't understand why
#if defined(WIN32) && defined(_MSC_VER)
is used. Don't _MSC_VER *always* imply WIN32? Or am I missing something?
comment:3 by , 15 years ago
My suggestions are thus, for cpl_port.h section 64bit support:
#if defined(_MSC_VER) #define VSI_LARGE_API_SUPPORTED typedef __int64 GIntBig; typedef unsigned __int64 GUIntBig; #elif HAVE_LONG_LONG typedef long long GIntBig; typedef unsigned long long GUIntBig; #else typedef long GIntBig; typedef unsigned long GUIntBig; #endif #if defined(__MSVCRT__) #define CPL_FRMT_GB "I64" #elif HAVE_LONG_LONG #define CPL_FRMT_GB "ll" #else #define CPL_FRMT_GB "l" #endif
The sprintf statement in CPLPrintUIntBig:
sprintf( szTemp, "%*"CPL_FRMT_GB"u", nMaxLen, iValue );
comment:4 by , 15 years ago
Cc: | added |
---|
I'm not sure that #if defined(WIN32) && defined(_MSC_VER) is redundant, because of WinCE... I think that for WinCE, WIN32 is not defined, but _MSC_VER is.
When I read, the first few lines of cpl_port.h, my understanding is that :
- WIN32 is defined (by CPL) because of _WIN32 being defined and not _WIN32_WCE
- WIN32CE is defined (by CPL) because of _WIN32_WCE being defined.
So it would mean that WIN32 is not defined for WinCE . However, in cpl_vsil_win32.cpp, the code inside the #if defined(WIN32) could let us think that we could go here even in the WinCE case...
I'm lost. All of this has been introduced in r9210. Mateusz, could you bring up some light on the subject ? To sum up :
- is WIN32 defined in the WinCE case ? Or is it just WIN32CE ?
- Can we expect int64, _atoi64, "%I64d" being defined on WinCE ?
comment:5 by , 15 years ago
One more MSVCRT issue: _MSC_VER at iom_utilities.cpp:55 should be MSVCRT
I'd like to move forward with this issue before the todo list becomes too long and my copy of GDAL for MSYS differs too much from trunk. I'm not sure the latest issue I brought up which added the WinCE complication is needed and we could move on with more clear things like this one.
comment:6 by , 15 years ago
Ari, in r16702 I've commited variations on the above discussions to improve mingw compatibility. Could you test ?
comment:7 by , 15 years ago
Thanks, looks good but I'll test a bit later.
Shouldn't you use "u" instead of "d" in CPLPrintUIntBig? iValue is GUIntBig.
comment:8 by , 15 years ago
Component: | default → ConfigBuild |
---|---|
Keywords: | mingw added |
I have had to alter the iom_utilities.cpp code to not assume that MSVCRT will be defined by MSVC (r16745).
comment:9 by , 15 years ago
Cc: | added |
---|---|
Resolution: | → fixed |
Status: | new → closed |
I'm generally happy with what Even commited in r16702. Only two things remain:
pcidsk dataset code within DEBUG (pcidskdataset.cpp:704 and below) (I cc this to dron, who is the author of pcidskdataset), and
libtiff, which uses in several places %I64d conditioned only with WIN32 and MSC. IMO libtiff code would also become much nicer in all those places if it would/could use GUIntBig, CPL_FRMT_GIB, and CPL_FRMT_GUIB. I'll post a message on the libtiff list.
But otherwise, as a GDAL bug I believe this is fixed.
Ari,
your idea about distinguishing between compiler and runtime library make sense. But for that particular issue about CPLScanPointer, what would you say about :
This is similar in spirit to what is already currently done in CPLPrintPointer. The advantage is that we don't even need to worry about what other strange environment would do in such situations...