Opened 8 years ago

Closed 8 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


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: Maybe we should use MSVCRT every time the runtime is the issue and somewhere in configuration add

#ifdef _MSC_VER
#define __MSVCRT__

Change History (9)

comment:1 Changed 8 years ago by Even Rouault


your idea about distinguishing between compiler and runtime library make sense. But for that particular issue about CPLScanPointer, what would you say about :

    if( EQUALN(szTemp,"0x",2) )
        pResult = NULL;
        /* With some runtime libraries, sscanf doesn't like the 0x prefix */
        /* So do a quick check to determine if we must strip the prefix or not */
        sscanf( "0x12345678", "%p", &pResult );
        if (pResult == (void*) 0x12345678)
            sscanf( szTemp, "%p", &pResult );
            sscanf( szTemp+2, "%p", &pResult );

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

comment:2 Changed 8 years ago by Ari Jolma

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 Changed 8 years ago by Ari Jolma

My suggestions are thus, for cpl_port.h section 64bit support:

#if defined(_MSC_VER)

typedef __int64          GIntBig;
typedef unsigned __int64 GUIntBig;


typedef long long        GIntBig;
typedef unsigned long long GUIntBig;


typedef long             GIntBig;
typedef unsigned long    GUIntBig;


#if defined(__MSVCRT__)

#define CPL_FRMT_GB     "I64"


#define CPL_FRMT_GB     "ll"


#define CPL_FRMT_GB     "l"


The sprintf statement in CPLPrintUIntBig:

sprintf( szTemp, "%*"CPL_FRMT_GB"u", nMaxLen, iValue );

comment:4 Changed 8 years ago by Even Rouault

Cc: Mateusz Łoskot 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 Changed 8 years ago by Ari Jolma

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 Changed 8 years ago by Even Rouault

Ari, in r16702 I've commited variations on the above discussions to improve mingw compatibility. Could you test ?

comment:7 Changed 8 years ago by Ari Jolma

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 Changed 8 years ago by warmerdam

Component: defaultConfigBuild
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 Changed 8 years ago by Ari Jolma

Cc: dron added
Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.