ST_AsText, ST_AsEWKT under Windows 64-bit PostGIS 64-bit is all screwy

[robe]

I was willing to dismiss this as something I was doing wrong during compile, when I tried compiling PostGIS 1.5 under my mingw64 and discovered that it did not exhibit the same screwy behavior. So now I'm not sure where the error is.

The PostGIS 1.5 64-bit compiled version doesn't have issue with ST_AsText. Here is an an example:

SELECT ST_AsText(ST_Buffer(ST_GeomFromText('POINT(1 2)',4326),1,'quad_segs=3'));

-- under PostGIS 1.5 64-bit mingw64

POLYGON((2 2,1.86602540378444 1.5,1.5 1.13397459621556,1 1,0.500000000000002 1.13397459621556,0.133974596215563 1.5,0 2,0.133974596215559 2.5,0.499999999999997 2.86602540378444,0.999999999999995 3,1.5 2.86602540378444,1.86602540378444 2.50000000000001,2 2))

-- under PostGIS 2.0.0beta2 64-bit mingw64

POLYGON((2 2,1.86602540378444 1.5,1.5 1.13397459621556,1 1,0.500000000000002 1.13397459621556,0.133974596215563 1.5,0 2, 2.5, , 3,1.5 2.86602540378444,1.86602540378444 2.50000000000001,2 2))

Note the ,s with no numbers.

I think the underlying geometry is fine, because if I do an ST_AsGML (my 32-bit and 64-bit installs of PostGIS 2.0.0 agree)

[robe]

Finally got CUnit working -- as expected the wkt assert fails and then later on Cunit crashes.

  Test: test_wkt_out_collection ...FAILED
    1. cu_out_wkt.c:145  - CU_ASSERT_STRING_EQUAL(cu_wkt("GEOMETRYCOLLECTION(POL
YGON((100 100 2, 100 200 2, 200 200 2, 200 100 2, 100 100 2)),POINT(.5 .5 .5),CI
RCULARSTRING(.8 .8 .8,.8 .8 .8,.8 .8 .8))",WKT_ISO),"GEOMETRYCOLLECTION Z (POLYG
ON Z ((100 100 2,100 200 2,200 200 2,200 100 2,100 100 2)),POINT Z (0.5 0.5 0.5)
,CIRCULARSTRING Z (0.8 0.8 0.8,0.8 0.8 0.8,0.8 0.8 0.8))")

[pramsey]

Something about the stringbuffer code could be handled differently between platforms. That is something the WKT emitter uses that the GML emitter does not. My windows build doesn't show up this problem so I can't be of much help.

[robe]

reran check and Cunit consistently crashes on:

 Test: out_kml_test_geoms ...

I do note that when I was trying ST_AsKML it was outputting blanky.

-- this test --

SELECT ST_AsKML(ST_Buffer(ST_GeomFromText('POINT(1 2)',4326),1,'quad_segs=3'));

-- run on my PostGIS 2.0.0 beta2 mingw64 -- returns nothing

-- run on my PostGIS 1.5.3 mingw64 -- returns:

<Polygon><outerBoundaryIs><LinearRing><coordinates>2,2 1.866025403784439,1.5 1.500000000000001,1.133974596215562 1.000000000000001,1 0.500000000000002,1.13397459621556 0.133974596215563,1.499999999999998 0,1.999999999999997 0.133974596215559,2.499999999999996 0.499999999999997,2.866025403784437 0.999999999999995,3 1.499999999999996,2.866025403784441 1.866025403784436,2.500000000000005 2,2</coordinates></LinearRing></outerBoundaryIs></Polygon>

[robe]

But Paul didn't the 1.5.3 use stringbuffer as well or is this a new thing?

[pramsey]

New in 2.0

[pramsey]

And looks like KML emitting also uses stringbuffer. I'd say you have a culprit, just need to figure the mechanism.

[robe]

I'm guessing whatever is going wrong is happening around here:

http://trac.osgeo.org/postgis/browser/trunk/liblwgeom/lwout_wkt.c#L95

But not really sure how to test that. Any thoughts.

[pramsey]

No, it's going to be deeper than that, inside the stringbuffer code itself. Where and why are going to be obscure, since it works on every other platform (including win32) tickety boo.

[robe]

Okay I made some progress. Issue seems to be with vsnprintf.

changed this line: http://trac.osgeo.org/postgis/browser/trunk/liblwgeom/stringbuffer.c#L211

	/* Propogate any printing errors upwards (check errno for info) */
	//if ( len < 0 ) return len;

To

if (len < 0 ) len = 128;

and then the cunit out tests were passing and kml was no longer crashing and was also passing. So the only cunit failing is the wkt_in for points. I screwed up my postgres some how so my postgres tests aren't running. I think that's a separate issue.

This msdn doc says that vsnprintf is deprecated --  http://msdn.microsoft.com/en-us/library/1kt27hek.aspx

But not sure if that only applies to VC++. I tried replacing with the suggested: vsnprintf_s and that did not exist. My next thought is to upgrade my C++. I'm running 4.4.7 prerelease something or other so perhaps its fixed in later.

[dustymugs]

robe, hopefully this isn't just me making noise but check comment 12 at...

 http://sourceforge.net/projects/mingw-w64/forums/forum/723797/topic/3482477

[pramsey]

Your fix makes no sense, robe... or at least you're saying that swallowing the error instead of returning on it makes things "work". But the error shouldn't be occurring. I wonder if dustymugs noise points closer to the right direction...

[robe]

I'll check dusty's link out. My thinking was that the value for length being returned by vnsprintf is too low. By setting len to higher when it fails, it then falls into the next loop -- your We don't have enough space:

/* We didn't have enough space! Expand the buffer. */
214	        if ( len >= maxlen )

And 128 should be plenty for anybody:)

[robe]

Actually I forget where I read it but I read that Linux returns the amount that would have been written could it write the value and windows 64 returns -1 if it can't write all of it. Which is what my thinking was to set it to something that can hold a double if it returns with can't write.

[robe]

Wouldn't it be better practice to check if a buffer can store a value before you write to the buffer? This code seems pretty lazy and hard to follow in my opinion becasue you have to know that vasprintf is expected to return the length even if it can't write the value.

Anyrate with this change (which I guess I should be using sizeof or something similar instead), topology passes with flying colors, raster (the rt_pixelvalue error has gone) away so I'm left with the same 3 failures as I have in 32-bit. One gnawing issue is after the cunit tests of postgis, it crashes trying to start the postgres tests. Another is raster testapi is giving funny messages about l not valuid, but then comes back with "tests all successful" so not sure what to make of that.

[pramsey]

Can you provide the exact patch you have made? The one you outline in your first comment makes no sense, the one in your second I can kind fo see, particularly if the behaviour of vsnprintf is variable (though we don't see this on win32!!!!).

Regarding the practice we don't know how much space a string will take until vsnprintf is called, so we need to depend on the return value of the function. I think rather than over-riding the return, if the msoft behaviour is in fact to return a negaitve errcode on an over-long write, we just need to test for "either more space than we have or a negative error code" on the first call to the function and everything should work fine. If there's a legitimate error, it'll recur inside that block and we cna return on it. If the error is due to a lack of space, we'll have expanded the buffer and everything will work the second time.

[pramsey]

I found a reference to msft vsnprintf that indicates that it returns negative on failure to fit a write within the limit. Perhaps the mingw32 links to a special libc so we don't call the msft version, while mingw64 links to the system libc. I'm patching and testing now.

[pramsey]

If I'm grokking your problem right, then r9435 will have fixed it.

[robe]

Nope -- with your solution, the wkt out tests works but the KML one still crashes.

I think the issue is that you are not allocating space when len < 0

So this line you have here

stringbuffer_makeroom(s, len + 1);

becomes

stringbuffer_makeroom(s, 0);

Which happens to work for the wkt out case because you make_room function overshoots adding an extra 2 or so above length

[strk]

Why don't we use lw_vasprintf ?

[pramsey]

Because it doesn't do what we need. This, on the other hand, is perfect with a capital P.

 http://www.jhweiss.de/software/snprintf.html

Even includes an autoconf macro so we can delegate to system implementations where they are compliant. Wowzers.

[robe]

So i guess this may affect solaris too huh. How much effor involved to make your suggested change. If a lot and windows 64 is the only one affected.

Here is another solution I found, not sure if this function is accessible under mingw64

 http://stackoverflow.com/questions/8488671/unix-to-windows-alternative-to-vsnprintf-to-determine-length

which points to this function:  http://msdn.microsoft.com/en-us/library/w05tbk72%28VS.71%29.aspx

[pramsey]

I implemented the portable snprintf over the weekend, but have been saddened to find that it doesn't quite behave as advertised on 1e300, which is an example in the KML cunit tests. So I'm going to poke it a few more times and see if there's a way around, and then try your workaround. Is there a #define that the mingw64 compiler sets? _WIN64 seems overly broad (though I could use that one).

[robe]

hmm not sure. Actually I think this issue may be patched in a later mingw64, so I'm setting up a newer one(I was kind of irritated about running 4.46 anyway instead of 4.5 something or other). I followed Bborie's thread to a patch they put in but the patch is later than my 20110516 build so I'm trying the 20111211 something or other sezero build.  http://sourceforge.net/projects/mingw-w64/files/Toolchains%20targetting%20Win64/Personal%20Builds/ (4.5 11/01/2011 one)

In theory Paul -- you should be able to build on Mac. They have a Darwin precompiled build. i assume Darwin works on Mac?  http://sourceforge.net/projects/mingw-w64/files/Toolchains%20targetting%20Win64/Automated%20Builds/

anyrate I'll try your other patch in the wrong ticket.

I've also got a ming32 one I'm setting up using the mingw64 chain but still in the middle of building the dependencies (have gdal and proj built ont aht so far.)

[pramsey]

Gulp, here's hoping the general solution is well-received. r9477

[robe]

nope -- doesn't compile. Sadly unless I'm doing something wrong, replacing my mingw with a newer version didn't help either. Perhaps I should just try to cut in that patch I found (if only I had bookmarked it).

[robe]

here is the list of errors if it helps any:

make[1]: Entering directory `/c/ming64/projects/postgis/postgis-2.0.0beta3SVN/li
blwgeom'
/bin/sh ..//libtool --mode=compile x86_64-w64-mingw32-gcc -g -O2  -DDLL_EXPORT -
DPIC  -I/c/ming64/projects/geos/rel-3.3.3w64/include -I/c/ming64/projects/proj/r
el-4.7.0w64/include -c -o snprintf.lo snprintf.c
 x86_64-w64-mingw32-gcc -g -O2 -DDLL_EXPORT -DPIC -I/c/ming64/projects/geos/rel-
3.3.3w64/include -I/c/ming64/projects/proj/rel-4.7.0w64/include -c snprintf.c  -
DDLL_EXPORT -DPIC -o .libs/snprintf.o
In file included from snprintf.c:293:0:
c:\ming64\mingw64\bin\../lib/gcc/x86_64-w64-mingw32/4.5.4/include/varargs.h:4:2:
 error: #error "GCC no longer implements <varargs.h>."
c:\ming64\mingw64\bin\../lib/gcc/x86_64-w64-mingw32/4.5.4/include/varargs.h:5:2:
 error: #error "Revise your code to use <stdarg.h>."
snprintf.c:527:12: warning: '_errno' redeclared without dllimport attribute: pre
vious dllimport ignored
snprintf.c: In function 'rpl_vsnprintf':
snprintf.c:860:10: warning: cast from pointer to integer of different size
snprintf.c: In function 'rpl_snprintf':
snprintf.c:1509:24: error: expected declaration specifiers before 'va_dcl'
snprintf.c:1520:21: error: macro "va_start" requires 2 arguments, but only 1 giv
en
snprintf.c:1527:1: error: expected '{' at end of input
make[1]: *** [snprintf.lo] Error 1
make[1]: Leaving directory `/c/ming64/projects/postgis/postgis-2.0.0beta3SVN/lib
lwgeom'
make: *** [all] Error 1

[robe]

BTW Hudson is not happy either, but in a different way -- log from Hudson. Perhaps we should just put things back the way they were for now.

Updating http://svn.osgeo.org/postgis/trunk
D         postgis/compat.h
G         raster/rt_pg/rt_pg.c
U         configure.ac
U         libpgcommon/lwgeom_pg.c
U         liblwgeom/Makefile.in
G         liblwgeom/stringbuffer.c
G         liblwgeom/cunit/cu_out_kml.c
U         liblwgeom/vsprintf.c
AU        liblwgeom/snprintf.c
U         liblwgeom/lwgeom_geos.c
G         liblwgeom/liblwgeom.h.in
U         liblwgeom/liblwgeom_internal.h
AU        macros/snprintf.m4
U         postgis_config.h.in
At revision 9477
[workspace] $ /bin/sh -xe /tmp/hudson3794278016352853331.sh
++ svn info src
++ grep 'Last Changed Rev:'
++ cut -f4 '-d '
+ SVN_REVISION=9477
+ rm -rf build
+ rm -rf rel
+ rm -rf postgis-2.0.0beta3SVN postgis-2.0.0beta3SVN.tar.gz
+ svn export src rel
Export complete.
+ cd ./rel
+ ./autogen.sh
* Running /usr/bin/libtoolize (1.5.22)
   OPTIONS = --force --copy
You should add the contents of `/usr/share/aclocal/libtool.m4' to `aclocal.m4'.
* Running /usr/bin/aclocal (1.9.6)
configure.ac:43: error: Autoconf version 2.60 or higher is required
macros/snprintf.m4:19: HW_HEADER_STDARG_H is expanded from...
configure.ac:43: HW_HEADER_STDARG_H is required by...
macros/snprintf.m4:57: HW_FUNC_VA_COPY is expanded from...
configure.ac:43: the top level
autom4te: /usr/bin/m4 failed with exit status: 63
aclocal: autom4te failed with exit status: 63

  Something went wrong, giving up!

Archiving artifacts
Sending e-mails to: chodgson@refractions.net pramsey@cleverelephant.ca
Notifying upstream projects of job completion
Finished: FAILURE

[chodgson]

Paul asked me to update the autoconf on hudson to 2.6, it's currently 2.59 which is from 2003... 2.6 is from 2006. I'm not against the update, but it will require a little work and I'm super busy ATM.

[robe]

Yah I'm thinking this is too big of a change in a beta cycle anyway. I'd rather just live with things the way they were and if I have to hack to get things to work in mingw64, that's fine for the first go round I think. Maybe in 2.0.1 we can revisit this.

[robe]

here is the patch I found which I'll try to apply to mine and see if it does the trick:

 http://www.mail-archive.com/mingw-w64-public@lists.sourceforge.net/msg03633.html

[mcayland]

Hang on guys, wait a second. MingW already comes with a set of wrappers around the MSVCRT routines that aren't 100% compliant which can get substituted at compile time to gain the correct behaviour if we need it. Hence we shouldn't need to be adding our own versions of functions like this to the repository until the point where people want to build this stuff on other compilers, e.g. MSVC.

The patch referenced by Regina above suggests that we can enable the MingW wrappers for broken MSVCRT functions by simply adding "-DUSE_MINGW_ANSI_STDIO" to both the compile/link command line - see some of the notes at  http://sourceforge.net/project/shownotes.php?release_id=24832 for further information.

[strk]

Beside this big changes, what strikes me is that the code in stringbuffer_avprintf is just _hoping_ to have enough space for writing the output and if not, hoping for one-more-bit to make a difference, which is kind of luck-based.

I keep thinking that for 2.0 having the new string allocated would save us some headaches. Then auditing for callers to see how they could do to get better performance.

[strk]

Adding an abort() inside the if ( len >= maxlen ) block shows that unit testing is _never_ triggering that case. Worth forcing it into doing.

[pramsey]

No, it's not hoping. You'll see it adds enough to fit maxlen into the buffer. This code is behind all our AsText? emitting and KML emitting, it's not lightly used or tested. If for some unforseen reason it's just plain impossible to add enough buffer or in fact there is a print error it errors back to the caller, which seems perfectly reasonable.

Anyways, I will roll back the big patch shortly and Regina can try and force MinGW to behave sanely.

[robe]

Mark,

I tried doing this thinking the same thing, but evidentally I'm not doing it right since it made no difference: ignore my PROJECTS, PG_VER variables

CPPFLAGS="-I${PROJECTS}/pgx64/pg${PG_VER}/include -D__USE_MINGW_ANSI_STDIO=1" LDFLAGS="-L${PROJECTS}/pgx64/pg${PG_VER}/lib -L${PROJECTS}/gdal/rel-${GDAL_VER}/lib" ./configure ..other stuff

BTW I think that is why the loader is broken, but the raster2pgsql one is broken because of the gettext issue (so trying to build that one from scratch). If I don't specify a schema the raster2pgsql one works fine. On closer inspection looks like shp2pgsql is also using string_buffer.

[pramsey]

rolled back to original at r9485

[mcayland]

I see you've added it to CPPFLAGS but I think you may also need to add it to LDFLAGS too? AFAICT the headers will be affected by the CPPFLAGS, however the linker also needs to told to pull in the modified MingW library routines too.

[strk]

Replying to pramsey:

No, it's not hoping. You'll see it adds enough to fit maxlen into the buffer.

Ah, I finally read it right, the return is the number of characters that _would_have_been_written_iff_ there was enough space. Cool. All fine then. I'll still add unit testing, just in case.

Sorry for the overparanoia

[strk]

With r9487 stringbuffer has a small test on its own.

cd liblwgeom/cunit && ./cu_tester stringbuffer 

[pramsey]

Why not just add #define _GNU_SOURCE to the very top of stringbuffer.c and see how it goes? I've noticed we don't have any terrific place to put these kinds of defines globally, unfortunately. postgis_config.h comes closest, but even it doesn't always get included ahead of system includes in all places.

[robe]

okay will give these a try. For time being I'll push this out of our 2.0.0 milestone phase.

[robe]

okay sadly none of these worked, however, I was able to find a good workaround.

Taking tips from this:  http://stackoverflow.com/questions/8488671/unix-to-windows-alternative-to-vsnprintf-to-determine-length

I used this function:  http://msdn.microsoft.com/en-us/library/w05tbk72%28VS.71%29.aspx

To determine the correct size to allocate. Just don't know how to make this a conditional define.

so my workaround looks like this:

if ( len < 0 ) 
	len = _vscprintf(fmt, ap2);/**Assume windows and compute space **/

That fixed my issue of being able to load large polygons with shp2pgsql.

[strk]

robe could you also see if adding a

#define _ISOC99_SOURCE 1

line makes a difference there ?

[robe]

nope. I tried that adding to top of string_buffer.c.

[robe]

I'll just keep on patching my environment until we come to a consensus on this.

[robe]

Paul and Mark -- if all you got is a 32-bit, I have the same issue building PostGIS under the mingw64-w32 chain for PostgreSQL 9.2. So I am using the same hacks as I did for mingw64-w64. I can package up my ming32 folder if you are interested (with all the prec-compiled proj,geos,etc libraries so you can extract and get to work :)

[woodbri]

robe,

I found this:

To sum up: 1. 32-bit compiler from mingw.org project - defines MINGW32 2. 32-bit compiler from mingw-w64.sf.net - defines MINGW32 3. 64-bit compiler from mingw-w64.sf.net - defines both MINGW32 and MINGW64

at  http://www.nntp.perl.org/group/perl.perl5.porters/2009/10/msg152622.html

this might be helpful to add in a conditional compile based on platform.

You can combine with the above WIN32 to determine if you are on a Windows platform because in theory can't you use mingw to compile for other systems?

[woodbri]

This might provide some more light on conditional compiles with mingw64:  http://sourceforge.net/projects/mingw-w64/forums/forum/723798/topic/3554864

and this is also a cool page that has lots of useful information about various compilers, and includes a section on MinGW and MinGW-w64  http://sourceforge.net/p/predef/wiki/Compilers/

[robe]

Steve,

Thanks for these links. I'm going to try the one you mentioned and hopefully get rid of that stringbuffer replace I've got to do each time or and THANKS A BUNCH for jumping in the mingw64 fire :)

[pramsey]

Robe will add conditional compilation around the windows code.

[robe]

for 2.1.0 r11217 . Seems to work fine on my mingw64 w64. Waiting to see if Debbie complains and works okay on my mingw64-w32 and old msys chain before commit to 2.0 branch. So if this works, no more hacking required for mingw64

[robe]

okay that worked great for my mingw64-w64, but not for mingw64-w32. I should have read the fine print. Switched to MINGW64_VERSION_MAJOR as documented in  http://sourceforge.net/p/predef/wiki/Compilers/

[robe]

fixed in 2.1.0 for both mingw64-w32 and mingw64-w64 at r11218 I'll close out once backported to 2.0 branch

[robe]

backported to 2.0 at r11229. seems to work fine on my old msys install (I use for 8.4-9.1 32-bit builds).

[jesseeichar]

Just as an additional quick test. I think it is related. Could you try the following out on you system:

select ST_AsText(ST_GeomFromText('POLYGON ((8.176 47.222, 8.174 47.227, 8.185 47.235, 8.183 47.24, 8.179 47.241, 8.173 47.236, 8.172 47.228, 8.161 47.232, 8.156 47.239, 8.18 47.246, 8.207 47.25, 8.208 47.247, 8.202 47.226, 8.176 47.222))'))

On a 64 bit windows 8 install with

"PostgreSQL 9.2.3, compiled by Visual C++ build 1600, 32-bit" "POSTGIS="2.0.3 r11132" GEOS="3.3.8-CAPI-1.7.8" PROJ="Rel. 4.8.0, 6 March 2012" GDAL="GDAL 1.9.2, released 2012/10/08" LIBXML="2.7.8" LIBJSON="UNKNOWN" TOPOLOGY RASTER"

I get the obviously broken result:

"POLYGON((8.176 47.222,8.174 47.227,8.185 47.235,8.183 47.24,8.179 47.241,8.173 47.236,8.172 47.228,8.161 47.232,8.156 47.239, , 47.25,8.208 47.247,8.202 47.226,8.176 47.222))"

(Notice the 8.156 47.239, , 47.25)

Thanks,

Jesse

[robe]

Jesse,

I get this

on my windows 7 9.2 32-bit -

PostgreSQL 9.2.1, compiled by Visual C++ build 1600, 32-bit POSTGIS="2.0.3 r11132" GEOS="3.4.0dev-CAPI-1.8.0 r0" PROJ="Rel. 4.8.0, 6 March 2012" GDAL="GDAL 1.9.2, released 2012/10/08" LIBXML="2.7.8" LIBJSON="UNKNOWN" RASTER

POLYGON((8.176 47.222,8.174 47.227,8.185 47.235,8.183 47.24,8.179 47.241,8.173 47.236,8.172 47.228,8.161 47.232,8.156 47.239,8.18 47.246,8.207 47.25,8.208 47.247,8.202 47.226,8.176 47.222))

So looks fine. But this is running on my 2.1.0 build.

It sounds like you got my broken build. If you installed with zip files that didn't have a -2 or an installer that didn't have a -2, then you got a bad build. I was hoping I swapped out before too many people were affected.

Reinstall using one of these and you should be fine:

 http://download.osgeo.org/postgis/windows/pg92/

[robe]

I meant to say I tested on both my 32-bit 2.0.3 build and my 64-bit 2.1.0 SVN build and both looked fine.

[jesseeichar]

I grabbed a new build and installed and as you said. it works fine now. Thanks, Jesse