Opened 12 years ago

Closed 11 years ago

#1984 closed defect (fixed)

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

Reported by: strk Owned by: pramsey
Priority: medium Milestone: PostGIS 2.1.0
Component: postgis Version: master
Keywords: history Cc:

Description

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

Change History (11)

comment:1 by strk, 12 years ago

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.

comment:2 by pramsey, 12 years ago

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

comment:3 by robe, 12 years ago

Milestone: PostGIS 2.0.2PostGIS 2.1.0

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

comment:4 by pramsey, 12 years ago

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.

comment:5 by robe, 12 years ago

Milestone: PostGIS 2.1.0PostGIS Future

How about we make this a PostGIS Future thing :)

comment:6 by mcayland, 11 years ago

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

comment:7 by robe, 11 years ago

Keywords: history added
Milestone: PostGIS FuturePostGIS 2.1.0

committed at r11508

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

comment:8 by strk, 11 years ago

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

comment:9 by robe, 11 years ago

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.

comment:10 by mcayland, 11 years ago

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.

comment:11 by robe, 11 years ago

Resolution: fixed
Status: newclosed

okay this is done right. I think s.

Note: See TracTickets for help on using tickets.