Thread: pgsql: Use FLEXIBLE_ARRAY_MEMBER in a bunch more places.
Use FLEXIBLE_ARRAY_MEMBER in a bunch more places. Replace some bogus "x[1]" declarations with "x[FLEXIBLE_ARRAY_MEMBER]". Aside from being more self-documenting, this should help prevent bogus warnings from static code analyzers and perhaps compiler misoptimizations. This patch is just a down payment on eliminating the whole problem, but it gets rid of a lot of easy-to-fix cases. Note that the main problem with doing this is that one must no longer rely on computing sizeof(the containing struct), since the result would be compiler-dependent. Instead use offsetof(struct, lastfield). Autoconf also warns against spelling that offsetof(struct, lastfield[0]). Michael Paquier, review and additional fixes by me. Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/09d8d110a604e52216102e73fb8475b7aa88f1d1 Modified Files -------------- contrib/cube/cubedata.h | 15 +++++++-------- contrib/intarray/_int.h | 4 ++-- contrib/ltree/ltree.h | 14 +++++++------- contrib/pageinspect/rawpage.c | 2 +- contrib/pg_trgm/trgm.h | 2 +- src/backend/catalog/namespace.c | 13 +++++++------ src/backend/commands/prepare.c | 5 ++--- src/backend/executor/functions.c | 6 +++--- src/backend/executor/spi.c | 5 ++--- src/backend/nodes/params.c | 5 ++--- src/backend/postmaster/syslogger.c | 4 ++-- src/backend/tcop/postgres.c | 5 ++--- src/backend/utils/adt/geo_ops.c | 22 +++++++++++----------- src/backend/utils/cache/catcache.c | 2 +- src/bin/pg_dump/dumputils.c | 3 +-- src/bin/pg_dump/dumputils.h | 2 +- src/include/access/gin_private.h | 4 ++-- src/include/access/gist_private.h | 7 ++++--- src/include/access/heapam_xlog.h | 2 +- src/include/access/spgist_private.h | 10 +++++----- src/include/access/xact.h | 8 ++++---- src/include/c.h | 8 ++++---- src/include/catalog/namespace.h | 4 ++-- src/include/commands/dbcommands.h | 15 --------------- src/include/commands/tablespace.h | 2 +- src/include/executor/hashjoin.h | 2 +- src/include/nodes/bitmapset.h | 4 ++-- src/include/nodes/params.h | 2 +- src/include/nodes/tidbitmap.h | 4 ++-- src/include/postgres.h | 8 ++++---- src/include/postmaster/syslogger.h | 2 +- src/include/replication/walsender_private.h | 4 ++-- src/include/storage/bufpage.h | 2 +- src/include/storage/fsm_internals.h | 2 +- src/include/storage/standby.h | 4 ++-- src/include/tsearch/dicts/regis.h | 2 +- src/include/tsearch/dicts/spell.h | 6 +++--- src/include/tsearch/ts_type.h | 6 +++--- src/include/utils/catcache.h | 4 ++-- src/include/utils/datetime.h | 4 ++-- src/include/utils/geo_decls.h | 4 ++-- src/include/utils/jsonb.h | 2 +- src/include/utils/relmapper.h | 2 +- src/include/utils/varbit.h | 3 ++- 44 files changed, 109 insertions(+), 127 deletions(-)
On 2015-02-20 05:12:00 +0000, Tom Lane wrote:
> Use FLEXIBLE_ARRAY_MEMBER in a bunch more places.
>
> Replace some bogus "x[1]" declarations with "x[FLEXIBLE_ARRAY_MEMBER]".
> Aside from being more self-documenting, this should help prevent bogus
> warnings from static code analyzers and perhaps compiler misoptimizations.
>
> This patch is just a down payment on eliminating the whole problem, but
> it gets rid of a lot of easy-to-fix cases.
>
> Note that the main problem with doing this is that one must no longer rely
> on computing sizeof(the containing struct), since the result would be
> compiler-dependent. Instead use offsetof(struct, lastfield). Autoconf
> also warns against spelling that offsetof(struct, lastfield[0]).
>
> Michael Paquier, review and additional fixes by me.
This triggers a large number of warnings with my preexisting clang 3.6 settings...
In file included from /home/andres/src/postgresql/src/backend/postmaster/pgstat.c:40:
/home/andres/src/postgresql/src/include/catalog/pg_proc.h:61:12: warning: 'proargtypes' may not be nested in a struct
dueto flexible array
member [-Wflexible-array-extensions]
oidvector proargtypes; /* parameter types (excludes OUT params) */
^
In file included from /home/andres/src/postgresql/src/backend/executor/execQual.c:40:
In file included from /home/andres/src/postgresql/src/include/access/nbtree.h:21:
/home/andres/src/postgresql/src/include/catalog/pg_index.h:48:13: warning: 'indkey' may not be nested in a struct due
toflexible array
member [-Wflexible-array-extensions]
int2vector indkey; /* column numbers of indexed cols, or 0 */
and many more.
Now, -Wflexible-array-extensions is IIRC not a standard option and I
only got it because I use -Weverything and disable the crazy stuff. So
maybe it's not worth worrying about. I'll just include
-Wno-flexible-array-extensions for now.
But I thought it's worth mentioning that at least one person went
through the trouble of adding a warning about this ;). I can't really
agree in this case because indkey still is the, as far the compiler can
see, last element of a struct, even if there's nesting.
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On 2015-02-20 16:39:14 +0100, Andres Freund wrote:
> SI'll just include -Wno-flexible-array-extensions for now.
Even after that I get:
/home/andres/src/postgresql/src/backend/utils/adt/tsrank.c:201:2: warning: flexible array initialization is a GNU
extension
[-Wgnu-flexible-array-initializer]
{0}
^
/home/andres/src/postgresql/src/include/tsearch/ts_type.h:66:15: note: initialized flexible array member 'pos' is here
WordEntryPos pos[FLEXIBLE_ARRAY_MEMBER];
^
Not sure if somebody that supports FLEXIBLE_ARRAY_MEMBER will not
implement that extension - seems natural to me - but it might be
worthwhile fixing nonetheless.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
> Even after that I get:
> /home/andres/src/postgresql/src/backend/utils/adt/tsrank.c:201:2: warning: flexible array initialization is a GNU
extension
> [-Wgnu-flexible-array-initializer]
> {0}
> ^
> /home/andres/src/postgresql/src/include/tsearch/ts_type.h:66:15: note: initialized flexible array member 'pos' is
here
> WordEntryPos pos[FLEXIBLE_ARRAY_MEMBER];
> ^
I was wondering about that one. We can get rid of it probably. I'm not
really convinced we need the static constant at all ...
regards, tom lane
Andres Freund <andres@2ndquadrant.com> writes:
> This triggers a large number of warnings with my preexisting clang 3.6 settings...
> In file included from /home/andres/src/postgresql/src/backend/postmaster/pgstat.c:40:
> /home/andres/src/postgresql/src/include/catalog/pg_proc.h:61:12: warning: 'proargtypes' may not be nested in a struct
dueto flexible array
That's annoying. I saw that Sun Studio was complaining similarly, but
I figured we could ignore it.
It's weird that compiler writers have such a hard time understanding
what actually counts as a *useful* warning, ie "you've got a flexible
array embedded in the middle of a bigger struct". Instead we get
either nothing (gcc) or pedantry (this).
regards, tom lane
On 2015-02-20 10:49:32 -0500, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > This triggers a large number of warnings with my preexisting clang 3.6 settings... > > > In file included from /home/andres/src/postgresql/src/backend/postmaster/pgstat.c:40: > > /home/andres/src/postgresql/src/include/catalog/pg_proc.h:61:12: warning: 'proargtypes' may not be nested in a structdue to flexible array > > That's annoying. I saw that Sun Studio was complaining similarly, but > I figured we could ignore it. > > It's weird that compiler writers have such a hard time understanding > what actually counts as a *useful* warning, ie "you've got a flexible > array embedded in the middle of a bigger struct". Instead we get > either nothing (gcc) or pedantry (this). It's two different warnings in clang, so it's not that bad. -Wgnu-variable-sized-type-not-at-end vs -Wflexible-array-extensions Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
> Even after that I get:
> /home/andres/src/postgresql/src/backend/utils/adt/tsrank.c:201:2: warning: flexible array initialization is a GNU
extension
> [-Wgnu-flexible-array-initializer]
> {0}
> ^
> /home/andres/src/postgresql/src/include/tsearch/ts_type.h:66:15: note: initialized flexible array member 'pos' is
here
> WordEntryPos pos[FLEXIBLE_ARRAY_MEMBER];
> ^
I cleaned that up, and also committed most of Michael's other changes.
What remains is the patch that flexible-izes HeapTupleHeaderData.t_bits.
I think we should do that, but I'm not happy with the widespread
changes like this:
- MAXALIGN(sizeof(HeapTupleHeaderData));
+ MAXALIGN(offsetof(HeapTupleHeaderData, t_bits));
I think this coding exposes much more knowledge about the innards of
HeapTupleHeaderData than we really want floating around in the places
that currently use sizeof(HeapTupleHeaderData). Most of them are not
that excited about having an exact answer anyway, and to the extent
that it does need to be exact this would not give the same result as
before.
A relevant technique that's been used in a lot of our code is to define
an intermediate macro, along the lines of
#define SizeofHeapTupleHeader offsetof(HeapTupleHeaderData, t_bits)
or maybe it would better be called HeapTupleHeaderFixedSize or
HeapTupleHeaderOverhead. Not sure what reads most nicely.
regards, tom lane
Tom Lane wrote: > A relevant technique that's been used in a lot of our code is to define > an intermediate macro, along the lines of > > #define SizeofHeapTupleHeader offsetof(HeapTupleHeaderData, t_bits) > > or maybe it would better be called HeapTupleHeaderFixedSize or > HeapTupleHeaderOverhead. Not sure what reads most nicely. Maybe the macro could take an argument which is the size of the data part, so that it could be allocated together with the Overhead part; the addition would be done in the macro rather than its caller. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Feb 21, 2015 at 10:16 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Tom Lane wrote: > >> A relevant technique that's been used in a lot of our code is to define >> an intermediate macro, along the lines of >> >> #define SizeofHeapTupleHeader offsetof(HeapTupleHeaderData, t_bits) >> >> or maybe it would better be called HeapTupleHeaderFixedSize or >> HeapTupleHeaderOverhead. Not sure what reads most nicely. > > Maybe the macro could take an argument which is the size of the data > part, so that it could be allocated together with the Overhead part; the > addition would be done in the macro rather than its caller. I think that we would be just fine with SizeofHeapTupleHeader, a notation with a suffix of the type FixedSize or Overhead is not something used in any of the existing #define of src/include using offsetof(). My 2c. -- Michael