Re: consider -Wmissing-variable-declarations - Mailing list pgsql-hackers

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