Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#3426 closed defect (fixed)

Patch to fix test_wkb_out_point failure on hppa & mips.

Reported by: Bas Couwenberg Owned by: robe
Priority: medium Milestone: PostGIS 2.2.2
Component: postgis Version: 2.2.x
Keywords: Cc:

Description

As reported in Debian Bug #810859:

postgis fails to build because the testcases fail, e.g.: https://buildd.debian.org/status/fetch.php?pkg=postgis&arch=hppa&ver=2.2.0%2Bdfsg-3&stamp=1451249930

specifically this test fails:

Suite: wkb_output
  Test: test_wkb_out_point ...FAILED
    1. cu_out_wkb.c:78  - CU_ASSERT_STRING_EQUAL(s,"00000000017FF80000000000007FF8000000000000")
    2. cu_out_wkb.c:81  - CU_ASSERT_STRING_EQUAL(s,"0020000001000010E67FF80000000000007FF8000000000000")
    3. cu_out_wkb.c:84  - CU_ASSERT_STRING_EQUAL(s,"00800000017FF80000000000007FF80000000000007FF8000000000000")
    4. cu_out_wkb.c:87  - CU_ASSERT_STRING_EQUAL(s,"00400000017FF80000000000007FF80000000000007FF8000000000000")
    5. cu_out_wkb.c:90  - CU_ASSERT_STRING_EQUAL(s,"00C00000017FF80000000000007FF80000000000007FF80000000000007FF8000000000000")

The reason why this fails is, because parisc/hppa and mips have different floating point representations for NAN (not-a-number). The attached patch fixes it for hppa (tested!) and most likely for mips as well (untested). Can you apply this patch for the next upload and/or push it upstream ?

Thanks, Helge

PS: For hppa postgis will probably still fail to build, but that's most likely a problem in postgres not postgis (still working on it).

Attachments (1)

hppa.patch (2.1 KB ) - added by Bas Couwenberg 8 years ago.

Download all attachments as: .zip

Change History (17)

by Bas Couwenberg, 8 years ago

Attachment: hppa.patch added

comment:1 by robe, 8 years ago

Owner: changed from pramsey to robe

comment:2 by robe, 8 years ago

(In [14592]) Patch from Sebastiaan Couwenberg to fix test_wkb_out_point failure on hppa & mips. references #3426

comment:3 by robe, 8 years ago

Resolution: fixed
Status: newclosed

(In [14593]) Patch from Sebastiaan Couwenberg to fix test_wkb_out_point failure on hppa & mips. closes #3426

comment:4 by strk, 8 years ago

Resolution: fixed
Status: closedreopened

I think this ticket should stay here until we find a portable way to represent POINT EMPTY, as it turns out the NaN approach isn't portable (introduced in 2.2.0, see #3181)

comment:5 by gdt, 8 years ago

I think this change should be backed out as not clean enough, and instead the code should start with a real NaN and convert to WKB to test.

I don't think it's reasonable to worry about vax (or other, even less plausible, non-IEEE754 machines).

This change probably also fails to address a number of other big-endian platforms, like sparc64 and BE arm variants.

comment:6 by pramsey, 8 years ago

I've a suggestion on #3181, for consideration.

comment:7 by strk, 8 years ago

Sebastiaan, is the IEEE NaN number (as you can see in the test) a valid [finite] double on mips/hppa ?

comment:8 by Bas Couwenberg, 8 years ago

I've asked Helge, the author of the patch, to join this discussion (I could not add him to the CC). As I don't know if the IEEE NaN number is a valid double on mips/hppa.

comment:9 by strk, 8 years ago

There seem to be a broad range of possible NaN values here, maybe we can find a good one, including the current one may be: https://en.wikipedia.org/wiki/NaN#Encoding

comment:10 by strk, 8 years ago

On my system both the values introduced in this patch return true from isnan():

#include <stdio.h>                                                              
#include <stdint.h>                                                             
#include <math.h>                                                               
                                                                                
int main()                                                                      
{                                                                               
  union {                                                                       
    int64_t i;                                                                  
    double d;                                                                   
  } val;                                                                        
                                                                                
  val.i = 0x7FF8000000000000;                                                   
  printf("%g is nan ? %d\n", val.d, isnan(val.d));                              
                                                                                
  val.i = 0x7FF7FFFFFFFFFFFF;                                                   
  printf("%g is nan ? %d\n", val.d, isnan(val.d));                              
}        

Could you run the same on your machine ? Chances are both are NaNs there too, in which case we're good to hard-code one version (could be quiet vs. signaling)

comment:11 by pramsey, 8 years ago

Actually, reading up on NaN some more, the problem is actually with the tests. The WKB we are generating *should* in fact be portable between the systems. The basic problem is that IEE NaN specifies the first portion of the number (it's actually an identical pattern to Inf) and then says "for the rest of the payload, do something non-zero" (to distinguish it from Inf, which specifies a zeroed out payload). And look at our two NaN patterns in the test are:

cu_wkb("POINT EMPTY");
CU_ASSERT_STRING_EQUAL(s, nan_val(
    "00000000017FF80000000000007FF8000000000000",
    "00000000017FF7FFFFFFFFFFFF7FF7FFFFFFFFFFFF"));

Strip out the first 10 chars (which are endian and type) and we get NaN of:

    7FF800000000000 /* intel */
    7FF7FFFFFFFFFFFF /* mips */

They both have the *same* NaN pattern (7FF), and they both have a non-zero payload. So if you ask either system isnan() on those numbers, you'll get back true. So they are portable nan values. What they aren't is identical at a string level. So our problem is with the tests, not with the code underneath.

comment:12 by strk, 8 years ago

I confirm both are valid POINT EMPTY representations, on my x86:

select ST_AsText(decode('00000000017FF80000000000007FF8000000000000', 'hex')::geometry);
POINT EMPTY
select ST_AsText(decode('00000000017FF7FFFFFFFFFFFF7FF7FFFFFFFFFFFF', 'hex')::geometry);
POINT EMPTY

Still I liked the idea of writing our EMPTY NaN in a postgis-specific way, if it's not too much work.

comment:13 by strk, 8 years ago

Example hard-coded values (FOSS4G FOSS4G):

-- XDR:
select ST_AsText(decode('0101000000F05546F05546f87fF05546F05546f87f', 'hex')::geometry);
-- NDR:
select ST_AsText(decode('00000000017FF7F05546F055467FF7F05546F05546', 'hex')::geometry);

in reply to:  10 comment:14 by Bas Couwenberg, 8 years ago

Replying to strk:

Could you run the same on your machine ?

Helge reported these results for parisc/hppa:

root@phantom:~# ./testnan
nan is nan ? 1
nan is nan ? 1

Helge also confirmed the incompleteness of the his patch for other BE platforms, as mentioned by gdt in comment:5.

Regarding the question "is the IEEE NaN number (as you can see in the test) a valid [finite] double on mips/hppa?" he wrote:

I don't think so. When you look at the debian build results on the various architectures, it seems that only hppa and mips seem to be affected by this NaN problem (sparc64 is OK if you click on "old" in sparc64 line): https://buildd.debian.org/status/package.php?p=postgis&suite=sid

He also offered access to a parisc/hppa machine:

If someone needs/want a ssh-login on a parisc machine, just let me know.

comment:15 by pramsey, 8 years ago

Resolution: fixed
Status: reopenedclosed

(In [14787]) fix failing tests on POINT EMPTY, closes #3426

comment:16 by pramsey, 8 years ago

Rather than try and play w/ NaN or predict every arch that will give us funny WKB, let's just test that we can round-trip an EMPTY through the format. r14787 in 2.2, trunk at r14788

Note: See TracTickets for help on using tickets.