Обсуждение: 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