Обсуждение: consider -Wmissing-variable-declarations

In [0] I had noticed that we have no automated verification that global 
variables are declared in header files.  (For global functions, we have 
this through -Wmissing-prototypes.)  As I mentioned there, I discovered 
the Clang compiler option -Wmissing-variable-declarations, which does 
exactly that.  Clang has supported this for quite some time, and GCC 14, 
which was released a few days ago, now also supports it.  I went and 
installed this option into the standard build flags and cleaned up the 
warnings it found, which revealed a number of interesting things.

I think these checks are important.  We have been trying to mark global 
variables as PGDLLIMPORT consistently, but that only catches variables 
declared in header files.  Also, a threading project would surely 
benefit from global variables (thread-local variables?) having 
consistent declarations.

Attached are patches organized by sub-topic.  The most dubious stuff is 
in patches 0006 and 0007.  A bunch of GUC-related variables are not in 
header files but are pulled in via ad-hoc extern declarations.  I can't 
recognize an intentional scheme there, probably just done for 
convenience or copied from previous practice.  These should be organized 
into appropriate header files.


[0]: 
https://www.postgresql.org/message-id/c4ac402f-9d7b-45fa-bbc1-4a5bf0a9f206@eisentraut.org

On 09/05/2024 12:23, Peter Eisentraut wrote:
> In [0] I had noticed that we have no automated verification that global
> variables are declared in header files.  (For global functions, we have
> this through -Wmissing-prototypes.)  As I mentioned there, I discovered
> the Clang compiler option -Wmissing-variable-declarations, which does
> exactly that.  Clang has supported this for quite some time, and GCC 14,
> which was released a few days ago, now also supports it.  I went and
> installed this option into the standard build flags and cleaned up the
> warnings it found, which revealed a number of interesting things.

Nice! More checks like this is good in general.

> Attached are patches organized by sub-topic.  The most dubious stuff is
> in patches 0006 and 0007.  A bunch of GUC-related variables are not in
> header files but are pulled in via ad-hoc extern declarations.  I can't
> recognize an intentional scheme there, probably just done for
> convenience or copied from previous practice.  These should be organized
> into appropriate header files.

+1 for moving all these to header files. Also all the "other stuff" in 
patch 0007.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

On 10.05.24 11:53, Heikki Linnakangas wrote:
> On 09/05/2024 12:23, Peter Eisentraut wrote:
>> In [0] I had noticed that we have no automated verification that global
>> variables are declared in header files.  (For global functions, we have
>> this through -Wmissing-prototypes.)  As I mentioned there, I discovered
>> the Clang compiler option -Wmissing-variable-declarations, which does
>> exactly that.  Clang has supported this for quite some time, and GCC 14,
>> which was released a few days ago, now also supports it.  I went and
>> installed this option into the standard build flags and cleaned up the
>> warnings it found, which revealed a number of interesting things.
> 
> Nice! More checks like this is good in general.
> 
>> Attached are patches organized by sub-topic.  The most dubious stuff is
>> in patches 0006 and 0007.  A bunch of GUC-related variables are not in
>> header files but are pulled in via ad-hoc extern declarations.  I can't
>> recognize an intentional scheme there, probably just done for
>> convenience or copied from previous practice.  These should be organized
>> into appropriate header files.
> 
> +1 for moving all these to header files. Also all the "other stuff" in 
> patch 0007.

I have found a partial explanation for the "other stuff".  We have in 
launch_backend.c:

/*
  * The following need to be available to the save/restore_backend_variables
  * functions.  They are marked NON_EXEC_STATIC in their home modules.
  */
extern slock_t *ShmemLock;
extern slock_t *ProcStructLock;
extern PGPROC *AuxiliaryProcs;
extern PMSignalData *PMSignalState;
extern pg_time_t first_syslogger_file_time;
extern struct bkend *ShmemBackendArray;
extern bool redirection_done;

So these are notionally static variables that had to be sneakily 
exported for the purposes of EXEC_BACKEND.

(This probably also means my current patch set won't work cleanly on 
EXEC_BACKEND builds.  I'll need to check that further.)

However, it turns out that that comment is not completely true. 
ShmemLock, ShmemBackendArray, and redirection_done are not in fact 
NON_EXEC_STATIC.  I think they probably once were, but then they were 
needed elsewhere and people thought, if launch_backend.c (formerly 
postmaster.c) gets at them via its own extern declaration, then I will 
do that too.

ShmemLock has been like that for a longer time, but ShmemBackendArray 
and redirection_done are new like that in PG17, probably from all the 
postmaster.c refactoring.

ShmemLock and redirection_done have now escaped for wider use and should 
be in header files, as my patches are proposing.

ShmemBackendArray only exists if EXEC_BACKEND, so it's fine, but the 
comment is slightly misleading.  Maybe sticking a NON_EXEC_STATIC onto 
ShmemBackendArray, even though it's useless, would make this more 
consistent.

Here is an updated patch set.  I have implemented proper solutions for 
the various hacks in the previous patch set.  So this patch set should 
now be ready for proper consideration.

The way I have organized it here is that patches 0002 through 0008 
should be improvements in their own right.

The remaining two patches 0009 and 0010 are workarounds that are just 
necessary to satisfy -Wmissing-variable-declarations.  I haven't made up 
my mind if I'd want to take the bison patch 0010 like this and undo it 
later if we move to pure parsers everywhere, or instead wait for the 
pure parsers to arrive before we install -Wmissing-variable-declarations 
for everyone.

Obviously, people might also have opinions on some details of where 
exactly to put the declarations etc.  I have tried to follow existing 
patterns as much as possible, but those are also not necessarily great 
in all cases.

Hi,

+many for doing this in principle


> -const char *EAN13_range[][2] = {
> +static const char *EAN13_range[][2] = {
>      {"000", "019"},                /* GS1 US */
>      {"020", "029"},                /* Restricted distribution (MO defined) */
>      {"030", "039"},                /* GS1 US */

> -const char *ISBN_range[][2] = {
> +static const char *ISBN_range[][2] = {
>      {"0-00", "0-19"},
>      {"0-200", "0-699"},
>      {"0-7000", "0-8499"},
> @@ -967,7 +967,7 @@ const char *ISBN_range[][2] = {
>   */

I think these actually ought be "static const char *const" - right now the
table is mutable, unless this day ends in *day and I've confused myself with C
syntax again.




>  /* Hook to check passwords in CreateRole() and AlterRole() */
>  check_password_hook_type check_password_hook = NULL;
> diff --git a/src/backend/postmaster/launch_backend.c b/src/backend/postmaster/launch_backend.c
> index bdfa238e4fe..bb1b0ac2b9c 100644
> --- a/src/backend/postmaster/launch_backend.c
> +++ b/src/backend/postmaster/launch_backend.c
> @@ -176,7 +176,7 @@ typedef struct
>      bool        shmem_attach;
>  } child_process_kind;
>  
> -child_process_kind child_process_kinds[] = {
> +static child_process_kind child_process_kinds[] = {
>      [B_INVALID] = {"invalid", NULL, false},
>  
>      [B_BACKEND] = {"backend", BackendMain, true},

This really ought to be const as well and is new.  Unless somebody protests
I'm going to make it so soon.


Structs like these, containing pointers, make for nice helpers in
exploitation. We shouldn't make it easier by unnecessarily making them
mutable.


> diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
> index 07bf356b70c..5a124385b7c 100644
> --- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
> +++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
> @@ -19,17 +19,18 @@
>  #include "common/logging.h"
>  #include "getopt_long.h"
>  
> -const char *progname;
> +static const char *progname;

Hm, this one I'm not so sure about. The backend version is explicitly globally
visible, and I don't see why we shouldn't do the same for other binaries.



> From d89312042eb76c879d699380a5e2ed0bc7956605 Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter@eisentraut.org>
> Date: Sun, 16 Jun 2024 23:52:06 +0200
> Subject: [PATCH v2 05/10] Fix warnings from -Wmissing-variable-declarations
>  under EXEC_BACKEND
> 
> The NON_EXEC_STATIC variables need a suitable declaration in a header
> file under EXEC_BACKEND.
> 
> Also fix the inconsistent application of the volatile qualifier for
> PMSignalState, which was revealed by this change.

I'm very very unenthused about adding volatile to more places. It's rarely
correct and often slow. But I guess this doesn't really make it any worse.


> +#ifdef TRACE_SYNCSCAN
> +#include "access/syncscan.h"
> +#endif

I'd just include it unconditionally.




> From f99c8712ff3dc2156c3e437cfa14f1f1a7f09079 Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter@eisentraut.org>
> Date: Wed, 8 May 2024 13:49:37 +0200
> Subject: [PATCH v2 09/10] Fix -Wmissing-variable-declarations warnings for
>  float.c special case
> 
> Discussion: https://www.postgresql.org/message-id/flat/e0a62134-83da-4ba4-8cdb-ceb0111c95ce@eisentraut.org
> ---
>  src/backend/utils/adt/float.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
> index cbbb8aecafc..bf047ee1b4c 100644
> --- a/src/backend/utils/adt/float.c
> +++ b/src/backend/utils/adt/float.c
> @@ -56,6 +56,11 @@ static float8 cot_45 = 0;
>   * compiler to know that, else it might try to precompute expressions
>   * involving them.  See comments for init_degree_constants().
>   */
> +extern float8 degree_c_thirty;
> +extern float8 degree_c_forty_five;
> +extern float8 degree_c_sixty;
> +extern float8 degree_c_one_half;
> +extern float8 degree_c_one;
>  float8        degree_c_thirty = 30.0;
>  float8        degree_c_forty_five = 45.0;
>  float8        degree_c_sixty = 60.0;

Yikes, this is bad code. Relying on extern to have effects like this will just
break with lto. But not the responsibility of this patch.


> From 649e8086df1f175e843b26cad41a698c8c074c09 Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter@eisentraut.org>
> Date: Wed, 8 May 2024 13:49:37 +0200
> Subject: [PATCH v2 10/10] Fix -Wmissing-variable-declarations warnings in
>  bison code
> 
> Add extern declarations for global variables produced by bison.

:(

Greetings,

Andres Freund

I have committed the first few of these.  (The compiler warning flag 
itself is not activated yet.)  This should allow you to proceed with 
your patches that add various const qualifiers.  I'll come back to the 
rest later.


On 18.06.24 17:02, Andres Freund wrote:
>> diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
>> index 07bf356b70c..5a124385b7c 100644
>> --- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
>> +++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
>> @@ -19,17 +19,18 @@
>>   #include "common/logging.h"
>>   #include "getopt_long.h"
>>   
>> -const char *progname;
>> +static const char *progname;
> 
> Hm, this one I'm not so sure about. The backend version is explicitly globally
> visible, and I don't see why we shouldn't do the same for other binaries.

We have in various programs a mix of progname with static linkage and 
with external linkage.  AFAICT, this is merely determined by whether 
there are multiple source files that need it, not by some higher-level 
scheme.

>>  From d89312042eb76c879d699380a5e2ed0bc7956605 Mon Sep 17 00:00:00 2001
>> From: Peter Eisentraut <peter@eisentraut.org>
>> Date: Sun, 16 Jun 2024 23:52:06 +0200
>> Subject: [PATCH v2 05/10] Fix warnings from -Wmissing-variable-declarations
>>   under EXEC_BACKEND
>>
>> The NON_EXEC_STATIC variables need a suitable declaration in a header
>> file under EXEC_BACKEND.
>>
>> Also fix the inconsistent application of the volatile qualifier for
>> PMSignalState, which was revealed by this change.
> 
> I'm very very unenthused about adding volatile to more places. It's rarely
> correct and often slow. But I guess this doesn't really make it any worse.

Yeah, it's not always clear with volatile, but in this one case it's 
probably better to keep it consistent rather than having to cast it away 
or something.

>> +#ifdef TRACE_SYNCSCAN
>> +#include "access/syncscan.h"
>> +#endif
> 
> I'd just include it unconditionally.

My thinking here was that if we apply an include file cleaner (like 
iwyu) sometime, it would flag this include as unused.  This way it's 
clearer what it's for.

I have committed all of the fixes that I had previously posted, but 
before actually activating the warning option, I found another small 
hiccup with the Bison files.

Before Bison 3.4, the generated parser implementation files run afoul of 
-Wmissing-variable-declarations (in spite of commit ab61c40bfa2) because 
declarations for yylval and possibly yylloc are missing.  The generated 
header files contain an extern declaration, but the implementation files 
don't include the header files.  Since Bison 3.4, the generated 
implementation files automatically include the generated header files, 
so then it works.

To make this work with older Bison versions as well, I made a patch to 
include the generated header file from the .y file.

(With older Bison versions, the generated implementation file contains 
effectively a copy of the header file pasted in, so including the header 
file is redundant.  But we know this works anyway because the core 
grammar uses this arrangement already.)

On 26.07.24 11:07, Peter Eisentraut wrote:
> I have committed all of the fixes that I had previously posted, but 
> before actually activating the warning option, I found another small 
> hiccup with the Bison files.

This has all been committed now.

Peter Eisentraut <peter@eisentraut.org> writes:
> This has all been committed now.

Various buildfarm animals are complaining about

g++ -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla
-Werror=unguarded-availability-new-Wendif-labels -Wmissing-format-attribute -Wcast-function-type -Wformat-security
-Wmissing-variable-declarations-fno-strict-aliasing -fwrapv -fexcess-precision=standard
-Wno-unused-command-line-argument-Wno-compound-token-split-by-macro -Wno-cast-function-type-strict -g -O2
-Wno-deprecated-declarations-fPIC -fvisibility=hidden -shared -o llvmjit.so  llvmjit.o llvmjit_error.o llvmjit_inline.o
llvmjit_wrap.ollvmjit_deform.o llvmjit_expr.o -L../../../../src/port -L../../../../src/common   -L/usr/lib64
-Wl,--as-needed-Wl,-rpath,'/home/centos/17-lancehead/buildroot/HEAD/inst/lib',--enable-new-dtags  -fvisibility=hidden
-lLLVM-17 
g++: error: unrecognized command line option \342\200\230-Wmissing-variable-declarations\342\200\231; did you mean
\342\200\230-Wmissing-declarations\342\200\231?
make[2]: *** [../../../../src/Makefile.shlib:261: llvmjit.so] Error 1

It looks like we are passing CFLAGS not CXXFLAGS to this particular
g++ invocation.

            regards, tom lane

On 03.08.24 22:46, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> This has all been committed now.
> 
> Various buildfarm animals are complaining about
> 
> g++ -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla
-Werror=unguarded-availability-new-Wendif-labels -Wmissing-format-attribute -Wcast-function-type -Wformat-security
-Wmissing-variable-declarations-fno-strict-aliasing -fwrapv -fexcess-precision=standard
-Wno-unused-command-line-argument-Wno-compound-token-split-by-macro -Wno-cast-function-type-strict -g -O2
-Wno-deprecated-declarations-fPIC -fvisibility=hidden -shared -o llvmjit.so  llvmjit.o llvmjit_error.o llvmjit_inline.o
llvmjit_wrap.ollvmjit_deform.o llvmjit_expr.o -L../../../../src/port -L../../../../src/common   -L/usr/lib64
-Wl,--as-needed-Wl,-rpath,'/home/centos/17-lancehead/buildroot/HEAD/inst/lib',--enable-new-dtags  -fvisibility=hidden
-lLLVM-17
> g++: error: unrecognized command line option \342\200\230-Wmissing-variable-declarations\342\200\231; did you mean
\342\200\230-Wmissing-declarations\342\200\231?
> make[2]: *** [../../../../src/Makefile.shlib:261: llvmjit.so] Error 1
> 
> It looks like we are passing CFLAGS not CXXFLAGS to this particular
> g++ invocation.

Changing this seems to have done the trick.

On Wed, Jun 19, 2024 at 3:02 AM Andres Freund <andres@anarazel.de> wrote:
> > -const char *EAN13_range[][2] = {
> > +static const char *EAN13_range[][2] = {
> >       {"000", "019"},                         /* GS1 US */
> >       {"020", "029"},                         /* Restricted distribution (MO defined) */
> >       {"030", "039"},                         /* GS1 US */
>
> > -const char *ISBN_range[][2] = {
> > +static const char *ISBN_range[][2] = {
> >       {"0-00", "0-19"},
> >       {"0-200", "0-699"},
> >       {"0-7000", "0-8499"},
> > @@ -967,7 +967,7 @@ const char *ISBN_range[][2] = {
> >   */

FYI these ones generate -Wunused-variable warnings from headerscheck
on CI, though it doesn't fail the task.  Hmm, these aren't really
headers, are they?

On 28.08.24 05:31, Thomas Munro wrote:
> On Wed, Jun 19, 2024 at 3:02 AM Andres Freund <andres@anarazel.de> wrote:
>>> -const char *EAN13_range[][2] = {
>>> +static const char *EAN13_range[][2] = {
>>>        {"000", "019"},                         /* GS1 US */
>>>        {"020", "029"},                         /* Restricted distribution (MO defined) */
>>>        {"030", "039"},                         /* GS1 US */
>>
>>> -const char *ISBN_range[][2] = {
>>> +static const char *ISBN_range[][2] = {
>>>        {"0-00", "0-19"},
>>>        {"0-200", "0-699"},
>>>        {"0-7000", "0-8499"},
>>> @@ -967,7 +967,7 @@ const char *ISBN_range[][2] = {
>>>    */
> 
> FYI these ones generate -Wunused-variable warnings from headerscheck
> on CI, though it doesn't fail the task.  Hmm, these aren't really
> headers, are they?

Yes, it looks like these ought to be excluded from checking:

diff --git a/src/tools/pginclude/headerscheck 
b/src/tools/pginclude/headerscheck
index 436e2b92a33..3fc737d2cc1 100755
--- a/src/tools/pginclude/headerscheck
+++ b/src/tools/pginclude/headerscheck
@@ -138,6 +138,12 @@ do
     test "$f" = src/pl/tcl/pltclerrcodes.h && continue

     # Also not meant to be included standalone.
+   test "$f" = contrib/isn/EAN13.h && continue
+   test "$f" = contrib/isn/ISBN.h && continue
+   test "$f" = contrib/isn/ISMN.h && continue
+   test "$f" = contrib/isn/ISSN.h && continue
+   test "$f" = contrib/isn/UPC.h && continue
+
     test "$f" = src/include/common/unicode_nonspacing_table.h && continue
     test "$f" = src/include/common/unicode_east_asian_fw_table.h && 
continue