flexible array members
gcc 4.6 has now arrived as the default compiler on my desktop, and as
previously reported, it throws a bunch of warnings, foiling my life-long
plan of compiling PostgreSQL with -Werror.
So looking more aggressively into fixing some of these, let's look at
this case:
gistutil.c: In function ‘gistMakeUnionKey’:
gistutil.c:263:16: warning: array subscript is above array bounds [-Warray-bounds]
gistutil.c:268:16: warning: array subscript is above array bounds [-Warray-bounds]
gistutil.c:273:16: warning: array subscript is above array bounds [-Warray-bounds]
The code in question is this:
typedef struct
{
int32 n; /* number of elements */
GISTENTRY vector[1]; /* variable-length array */
} GistEntryVector;
Not sure why the new gcc is confused about this when -Warray-bounds has
existed for a while. But thinking a bit further, the "proper" fix for
this would be to use flexible array members like this:
typedef struct
{
int32 n; /* number of elements */
GISTENTRY vector[];
} GistEntryVector;
This is C99, but with some gentle standard autoconf seasoning, it can be
made transparent. See attached patch.
Is this a route we want to go down?
It looks as though other compilers could also benefit from this. clang
throws even more warnings of this kind, and the clang static analyzer
even more.
One thing that is a bit concerning is that throwing more flexible array
members around the code wherever variable-length arrays are used results
in crash and burn. Probably some places are using sizeof or offsetof on
these structures in incompatible ways. So each place would have to be
examined separately.
Attachments:
flexible-array-member.patchtext/x-patch; charset=UTF-8; name=flexible-array-member.patchDownload
diff --git i/configure.in w/configure.in
index ddc4cc9..e873c7b 100644
--- i/configure.in
+++ w/configure.in
@@ -1110,6 +1110,7 @@ AC_C_BIGENDIAN
AC_C_CONST
PGAC_C_INLINE
AC_C_STRINGIZE
+AC_C_FLEXIBLE_ARRAY_MEMBER
PGAC_C_SIGNED
AC_C_VOLATILE
PGAC_C_FUNCNAME_SUPPORT
diff --git i/src/include/access/gist.h w/src/include/access/gist.h
index df9f39c..f3dfcaa 100644
--- i/src/include/access/gist.h
+++ w/src/include/access/gist.h
@@ -144,7 +144,7 @@ typedef struct GISTENTRY
typedef struct
{
int32 n; /* number of elements */
- GISTENTRY vector[1]; /* variable-length array */
+ GISTENTRY vector[FLEXIBLE_ARRAY_MEMBER];
} GistEntryVector;
#define GEVHDRSZ (offsetof(GistEntryVector, vector))
diff --git i/src/include/pg_config.h.in w/src/include/pg_config.h.in
index 5d38f25..19f38cc 100644
--- i/src/include/pg_config.h.in
+++ w/src/include/pg_config.h.in
@@ -61,6 +61,15 @@
(--enable-thread-safety) */
#undef ENABLE_THREAD_SAFETY
+/* Define to nothing if C supports flexible array members, and to 1 if it does
+ not. That way, with a declaration like `struct s { int n; double
+ d[FLEXIBLE_ARRAY_MEMBER]; };', the struct hack can be used with pre-C99
+ compilers. When computing the size of such an object, don't use 'sizeof
+ (struct s)' as it overestimates the size. Use 'offsetof (struct s, d)'
+ instead. Don't use 'offsetof (struct s, d[0])', as this doesn't work with
+ MSVC and with C++ compilers. */
+#undef FLEXIBLE_ARRAY_MEMBER
+
/* float4 values are passed by value if 'true', by reference if 'false' */
#undef FLOAT4PASSBYVAL
Peter Eisentraut <peter_e@gmx.net> writes:
Is this a route we want to go down?
- GISTENTRY vector[1]; /* variable-length array */ + GISTENTRY vector[FLEXIBLE_ARRAY_MEMBER];
Yes, I was thinking about the same trick after noting these warnings on
Fedora 15, although personally I'd name the macro VARIABLE_LENGTH_ARRAY.
One thing that is a bit concerning is that throwing more flexible array
members around the code wherever variable-length arrays are used results
in crash and burn. Probably some places are using sizeof or offsetof on
these structures in incompatible ways. So each place would have to be
examined separately.
Hmm, that's nasty. But from a code-documentation standpoint I think
this is a useful improvement, so it seems worth doing the work to clean
things up.
(I do recall a number of places that assume that sizeof() includes a
single array element ...)
regards, tom lane
On ons, 2011-06-15 at 18:19 -0400, Tom Lane wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
Is this a route we want to go down?
- GISTENTRY vector[1]; /* variable-length
array */
+ GISTENTRY vector[FLEXIBLE_ARRAY_MEMBER];
Yes, I was thinking about the same trick after noting these warnings
on Fedora 15, although personally I'd name the macro
VARIABLE_LENGTH_ARRAY.
This macro is provided by Autoconf and it appears to be using the
standard's terminology.
Actually, the term "variable-length array" appears to refer to another
C99 feature, namely this one:
void
foo(int n)
{
bar int[n];
do_something();
}
On Thu, 16 Jun 2011 22:49:45 +0300, Peter Eisentraut
<peter_e@gmx.net> wrote:
This macro is provided by Autoconf and it appears to be using the
standard's terminology.
commit dbbba5279f66f95805c1e084e6f646d174931e56 refs/heads/master
Author: Peter Eisentraut <peter_e@gmx.net>
Date: Thu Jun 16 22:39:09 2011 +0300
Periodically checking my VS2010 patch I noticed that this one broke
Visual Studio builds.
At least mine and "chough
<http://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=chough&br=HEAD>"
in the build farm - I expect others to follow once they rebuild.
error C2065: 'FLEXIBLE_ARRAY_MEMBER' : undeclared identifier
error C2057: expected constant expression
Regards,
Brar