Problem with shapefile indexes and 64bits long integers.

[brage@…]

After upgrading to FreeBSD/AMD64, polygons started to randomly appear and
disappear on maps created by mapscript (or mapserver itself) from shapefiles. 

The problem seems to be with searchDiskTreeNode in maptree.c.:

  long offset;
  [..]
  res = fread( &offset, 4, 1, disktree->fp );

A long is 8 bytes on FreeBSD/AMD64 (and probably many other 64bits) plaforms. 
Initializing offset to 0 solved the problem here. 

readTreeNode is probably also affected.

[brage@…]

Patch to change maptree offset type from long to int32

[brage@…]

The current maptree-code assumes both long and int to be 32 bits wide. The
attached patch should make matree indexes work on most (all supported?) 64-bit
platforms.

The code should probably also be fixed to allow integer sizes <> 32 bits. 

[sdlime]

Adding Frank to the CC list since he's probably best positioned to comment...

Steve

[fwarmerdam]

Clearly we need to use a 32bit integer to read stuff from disk into.  
The approach seems fine, and matches what was done in mapshape.c.

I would mildly discourage use of the name int32 as that is predefined on
some platforms, but apparently it hasn't caused problem in mapshape.c,
so what the heck. 

Will you apply the patch Steve?  Perhaps into 4.6.x as well for the 4.6.1 
release?

[brage@…]

I did prepare a longer patch which attempts to change the relevant ints to
int32_t too (against head). 

Will check it and test it some more, and submit it later.

In this patch I add  AC_CHECK_HEADERS(inttypes.h stdint.h) to configure.in and
replaces the int32 definitions with this in map.h:

#if HAVE_INTTYPES_H
#include <inttypes.h>
#elif HAVE_STDINT_H
#include <stdint.h>
#elif UINT_MAX == 65535
typedef long int32_t;
#else
typedef int  int32_t;
#endif

and changes type to int32_t. Would that be an acceptable approach? 

[sdlime]

I'll apply a new patch once the type name gets sorted out. Why not stick with a 
name like ms_int32 or something like that?

Steve

[brage@…]

Since int32_t is defined in both Posix (1997?) and C99 it seemed reasonable to
use the standard names, and only define it if the standard includes where not
available. 

But ms_int32 should be fine if you want to rename it.

[fwarmerdam]

I personally advise against using the standard name.  In my experience we
will run into problems properly detecting whether it is predefined in some
cases and have lots of misery.  I would concur with Steve's suggestion to 
call it ms_int32. 

This based on a decade and a half of misery because libtiff and libjpeg folks
chose to use standard type names (like boolean, int32, etc) without the ability
to properly detect how or if they are predefined. 

If we can avoid configure magic, I think we are far ahead. 

[brage@…]

ms_int32-patch

[brage@…]

This patch 
 - defines ms_int32 in map.h
 - renames the int32-types in mapshape
 - tries to make all integers and longs which now are assumed to be 32bits
   ms_int32

I suggest applying this patch for 4.7, and the previous patch for 4.6.1.

This patch does not use any autoconf magic. Should we use autoconf to detect if
int32_t is available, and typedef ms_int32 to this is available?

[sdlime]

Are we going to run into any build problems with that 1st patch? I'm kinda 
thinking we should get everything right in 4.7 and perhaps back port to 4.6.2 
(if at all). This is not an issue for the vast majority of users. I will apply 
the second patch to 4.7... Will leave the bug open until the configuration 
issues are sorted out.

Could a simple #ifdef be used to define ms_int32 as int32_t if available? Or 
better yet why even worry about it and just use the primative types...

Steve

[sdlime]

Patch has been applied and committed to 4.7...

Steve

[fwarmerdam]

My opinion is that there is no value in trying to use the int32_t type if
defined. 

I am also a bit nervous about backporting this number of changes to 4.6 
though 64bit systems are becoming quite common so I think applying the simpliest
possible form of the patch in 4.6 would make sense. 

[sdlime]

I think a 2.5-year test period is sufficient. Closing...

Steve