geography_btree.c: array subscript is above array bounds [-Warray-bounds]

[strk]

With gcc 4.6.3:

geography_btree.c: In function ‘geography_lt’:
geography_btree.c:38:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:38:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c: In function ‘geography_le’:
geography_btree.c:38:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:38:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c: In function ‘geography_gt’:
geography_btree.c:38:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:38:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c: In function ‘geography_ge’:
geography_btree.c:38:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:38:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c: In function ‘geography_eq’:
geography_btree.c:38:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:38:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c: In function ‘geography_cmp’:
geography_btree.c:38:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:38:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:39:33: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:9: warning: array subscript is above array bounds [-Warray-bounds]
geography_btree.c:40:33: warning: array subscript is above array bounds [-Warray-bounds]

Worth checking

[strk]

Ok the issue here is GIDX->c being defined as a 1-element array, while we know it's of variable size instead.

One way to fix this would be changing its definition from:

 typedef struct
 {
        int32 varsize;
        float c[1];
 } GIDX;

To:

 typedef struct
 {
        int32 varsize;
        float c[];
 } GIDX;

But doing so would change sizeof(GIDX) to be one float less than possibly expected by client code. This may actually simplify some code, but doesn't look like we have much testing for that (nothing in the testsuite fails with the above changes).

Another way could be doing the cast from within the GIDX_* macros which are using subscripts. Probably safer.

[pramsey]

Yes, could just convert the macros to use pointer arithmetic instead of subscripts.

[robe]

sounds too dangerous to fix. Push back if you feel otherwise.

[pramsey]

The code itself is perfectly legal and good and well tested, it's just a compiler warning. Changing the code is dangerous, since it works fine. Using some other C construct to express the intent might quiet the warning on the newer gcc compilers but break our compilation on other ones. Definitely a 2.1 thing.

[robe]

How about we make this a PostGIS Future thing :)

[mcayland]

Okay - pointer/cast workaround committed to trunk. Feel free to close out in a couple of weeks assuming we've had no complaints.

[robe]

committed at r11508

this really should have waited till we branched and trunk became 2.2 (oh well).

[strk]

robe: I think we should ship a 2.1.0 that builds with no compiler warnings, really

[robe]

my qualm is not about that as we already called beta and we had 7 months to do this before beta was called. This looks to me like something that may fix compiler warnings and cause other issues so really should have undergone more testing before it was shoved in at the last minute.

[mcayland]

I appreciate that we probably should have done this earlier during the cycle, but then again the excessive warnings have unmasked at least one potential crash bug that we should fix.

In terms of risk, I'd say it's reasonably low: it's two self-contained macros that are used in several critical places during the index code (and it's really just changing the arithmetic to use direct pointer arithmetic with no logic change) and so if I made a mistake it would likely crash fairly quickly. I also introduced deliberate errors to both the pre- and post- patch version and confirmed that the regression tests failed in exactly the same way by diffing the files involved so it's already had a pretty good work out.

Apologies if you feel slightly put out by this, but I genuinely feel that the benefits will far outweigh the drawbacks in this particular case.

[robe]

okay this is done right. I think s.