Обсуждение: headerscheck warnings with late-model gcc

Using gcc 15.1.1 (from Fedora 42) I see these warnings that
didn't appear with older gcc:

$ src/tools/pginclude/headerscheck
In file included from /tmp/headerscheck.xp0AI5/test.c:2:
./src/common/kwlist_d.h:1163:23: warning: no previous declaration for 'ScanKeywords' [-Wmissing-variable-declarations]
 1163 | const ScanKeywordList ScanKeywords = {
      |                       ^~~~~~~~~~~~
In file included from /tmp/headerscheck.xp0AI5/test.c:2:
./src/interfaces/ecpg/test/preproc/strings.h:1:13: warning: no previous declaration for 's1'
[-Wmissing-variable-declarations]
    1 | char       *s1,
      |             ^~
./src/interfaces/ecpg/test/preproc/strings.h:2:21: warning: no previous declaration for 's2'
[-Wmissing-variable-declarations]
    2 |                    *s2,
      |                     ^~
./src/interfaces/ecpg/test/preproc/strings.h:3:21: warning: no previous declaration for 's3'
[-Wmissing-variable-declarations]
    3 |                    *s3,
      |                     ^~
./src/interfaces/ecpg/test/preproc/strings.h:4:21: warning: no previous declaration for 's4'
[-Wmissing-variable-declarations]
    4 |                    *s4,
      |                     ^~
./src/interfaces/ecpg/test/preproc/strings.h:5:21: warning: no previous declaration for 's5'
[-Wmissing-variable-declarations]
    5 |                    *s5,
      |                     ^~
./src/interfaces/ecpg/test/preproc/strings.h:6:21: warning: no previous declaration for 's6'
[-Wmissing-variable-declarations]
    6 |                    *s6,
      |                     ^~
./src/interfaces/ecpg/test/preproc/strings.h:7:21: warning: no previous declaration for 's7'
[-Wmissing-variable-declarations]
    7 |                    *s7,
      |                     ^~
./src/interfaces/ecpg/test/preproc/strings.h:8:21: warning: no previous declaration for 's8'
[-Wmissing-variable-declarations]
    8 |                    *s8;
      |                     ^~

While we could possibly get away with making headerscheck ignore that
ecpg test header, it seems unwise to skip kwlist_d.h.  So I propose
the attached patch, which I've confirmed silences these warnings.

Curiously, no such complaints appear with cpluspluscheck (which is
using g++ 15.1.1).  I don't really understand why not: why would
they have turned on -Wmissing-variable-declarations by default
for C but not C++?  But anyway, since there doesn't seem to be
any C++ compatibility issue here, I think it's sufficient to fix
this in master and not back-patch.

            regards, tom lane

diff --git a/src/interfaces/ecpg/test/expected/preproc-strings.c b/src/interfaces/ecpg/test/expected/preproc-strings.c
index a26817968de..55859b624eb 100644
--- a/src/interfaces/ecpg/test/expected/preproc-strings.c
+++ b/src/interfaces/ecpg/test/expected/preproc-strings.c
@@ -18,6 +18,16 @@
 #line 3 "strings.pgc"
 /* exec sql begin declare section */
 #line 1 "strings.h"
+/* This extern silences headerscheck warnings with some gcc versions */
+
+
+
+
+
+
+
+
+



@@ -29,7 +39,10 @@

 #line 5 "strings.pgc"

-#line 1 "strings.h"
+#line 2 "strings.h"
+ extern char * s1 , * s2 , * s3 , * s4 , * s5 , * s6 , * s7 , * s8 ;
+
+#line 11 "strings.h"
  char * s1 , * s2 , * s3 , * s4 , * s5 , * s6 , * s7 , * s8 ;
 /* exec sql end declare section */
 #line 5 "strings.pgc"
diff --git a/src/interfaces/ecpg/test/preproc/strings.h b/src/interfaces/ecpg/test/preproc/strings.h
index edb5be5339e..71581d6f94a 100644
--- a/src/interfaces/ecpg/test/preproc/strings.h
+++ b/src/interfaces/ecpg/test/preproc/strings.h
@@ -1,3 +1,13 @@
+/* This extern silences headerscheck warnings with some gcc versions */
+extern char *s1,
+           *s2,
+           *s3,
+           *s4,
+           *s5,
+           *s6,
+           *s7,
+           *s8;
+
 char       *s1,
            *s2,
            *s3,
diff --git a/src/tools/gen_keywordlist.pl b/src/tools/gen_keywordlist.pl
index 6ec83ff33f9..8825b4476ac 100644
--- a/src/tools/gen_keywordlist.pl
+++ b/src/tools/gen_keywordlist.pl
@@ -169,7 +169,16 @@ printf $kwdef qq|static %s\n|, $f;

 # Emit the struct that wraps all this lookup info into one variable.

-printf $kwdef "static " if !$extern;
+if ($extern)
+{
+    # This redundant extern declaration is needed to silence headerscheck
+    # warnings with some gcc versions.
+    printf $kwdef "extern const ScanKeywordList %s;\n\n", $varname;
+}
+else
+{
+    printf $kwdef "static ";
+}
 printf $kwdef "const ScanKeywordList %s = {\n", $varname;
 printf $kwdef qq|\t%s_kw_string,\n|, $varname;
 printf $kwdef qq|\t%s_kw_offsets,\n|, $varname;

On 05.08.25 20:09, Tom Lane wrote:
> Curiously, no such complaints appear with cpluspluscheck (which is
> using g++ 15.1.1).  I don't really understand why not: why would
> they have turned on -Wmissing-variable-declarations by default
> for C but not C++?  But anyway, since there doesn't seem to be
> any C++ compatibility issue here, I think it's sufficient to fix
> this in master and not back-patch.

-Wmissing-variable-declarations is added by us as of PG18 (commit 
66188912566).  It's available since gcc 14 and doesn't exist for C++.

Peter Eisentraut <peter@eisentraut.org> writes:
> On 05.08.25 20:09, Tom Lane wrote:
>> Curiously, no such complaints appear with cpluspluscheck (which is
>> using g++ 15.1.1).  I don't really understand why not ...

> -Wmissing-variable-declarations is added by us as of PG18 (commit 
> 66188912566).  It's available since gcc 14 and doesn't exist for C++.

Oh!  Okay, that explains the lack of messages, but it still seems
like an odd omission.

            regards, tom lane

On 06.08.25 21:11, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> On 05.08.25 20:09, Tom Lane wrote:
>>> Curiously, no such complaints appear with cpluspluscheck (which is
>>> using g++ 15.1.1).  I don't really understand why not ...
> 
>> -Wmissing-variable-declarations is added by us as of PG18 (commit
>> 66188912566).  It's available since gcc 14 and doesn't exist for C++.
> 
> Oh!  Okay, that explains the lack of messages, but it still seems
> like an odd omission.

Yeah, this is pretty much my fault for not checking this for the above 
commit.

I've been having a hard time getting headerscheck to work reliably in my 
environment, so I ended up relying on CI, which doesn't have new-enough 
compilers yet (and/or doesn't run it everywhere; the clang on the 
FreeBSD task might have caught it (but we also don't have this 
integrated with meson yet)).

Attached are three patches to fix some unrelated problems with 
headerscheck in my environment.

The fourth one is to fix the ecpg issue; I think we can ignore it under 
the "code fragment" category.

kwlist_d.h doesn't show up in my run, probably because I'm using a 
separate build directory, which headerscheck doesn't handle?  Another 
thing to fix.  But I guess it would also fall under the code fragment 
category?

But the code fragment exception is also faulty, because we plausibly do 
want to check that file for C++ compatibility, just not necessarily as a 
standalone file.  Not sure how to cover all these bases at once.

Peter Eisentraut <peter@eisentraut.org> writes:
> Attached are three patches to fix some unrelated problems with 
> headerscheck in my environment.

0001 seems fine; it's an oversight that I'd not noticed because
ICU_CFLAGS is empty in my usage.

Don't like 0002 as-is.  I'd be okay with skipping that header if
--with-llvm isn't given.  However, we already tell users that
they'd better configure --with-perl and --with-python, so maybe
just add --with-llvm to that list?

0003: +1, I noticed that too yesterday.

0004: I prefer the solution I exhibited yesterday, ie add
externs to those headers.

> kwlist_d.h doesn't show up in my run, probably because I'm using a 
> separate build directory, which headerscheck doesn't handle?  Another 
> thing to fix.

Yeah, as it stands headerscheck is really only meant for in-tree
builds.  We will definitely have to do something about that to make
it handle built headers in meson builds, and I guess it'd be nice if
VPATH works that way as well.  One idea could be to redefine it as
searching the installation include-file tree instead of the source
tree, so that the build process washes out of the matter.

            regards, tom lane

Hi,

On 2025-08-07 10:58:56 -0400, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
> > kwlist_d.h doesn't show up in my run, probably because I'm using a
> > separate build directory, which headerscheck doesn't handle?  Another
> > thing to fix.
>
> Yeah, as it stands headerscheck is really only meant for in-tree
> builds.  We will definitely have to do something about that to make
> it handle built headers in meson builds, and I guess it'd be nice if
> VPATH works that way as well.  One idea could be to redefine it as
> searching the installation include-file tree instead of the source
> tree, so that the build process washes out of the matter.

One thing I dislike rather intensely about cpluspluscheck/headerscheck is that
they're abominally slow and thus can't just be executed as part of normal
compile-test-edit cycles. That's obviously due to a) not being incremental b)
not being parallel.

The reason I bring that up here is that the fix for that would be to move the
iteration over all headers from headerscheck to the build system, which then
could perform the check only if headers changed and in parallel. That'd
obviously also address VPATH / meson builds.

The challenge with that is the buildsystem needs to be aware of the list of
headers. However, if we had that awareness, we'd gain two additional benefits:

1) The compile_commands.json that meson generates would provide the
   information necessary to compile headers, which would allow editors to do
   auto-completion etc for header files.

2) We'd be able to install headers with meson using an explicit list, rather
   than the install_subdir(), which has issues with editor files as mentioned
   in src/include/meson.build


It's possible to do this by globing for files at configure time, but that
wouldn't detect adding new headers (which would need to trigger a
re-configure). Whether that's an issue worth caring about I'm a bit on the
fence about.

Greetings,

Andres Freund

On 07.08.25 16:58, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> Attached are three patches to fix some unrelated problems with
>> headerscheck in my environment.
> 
> 0001 seems fine; it's an oversight that I'd not noticed because
> ICU_CFLAGS is empty in my usage.

committed

> Don't like 0002 as-is.  I'd be okay with skipping that header if
> --with-llvm isn't given.  However, we already tell users that
> they'd better configure --with-perl and --with-python, so maybe
> just add --with-llvm to that list?

Yes, just added documentation.

> 0003: +1, I noticed that too yesterday.

committed

> 0004: I prefer the solution I exhibited yesterday, ie add
> externs to those headers.

That solution seems fine, too.

This comment should be clarified:

+    # This redundant extern declaration is needed to silence headerscheck
+    # warnings with some gcc versions.

You could just write

# silence -Wmissing-variable-declarations

which also appears elsewhere in the code.

Peter Eisentraut <peter@eisentraut.org> writes:
> On 07.08.25 16:58, Tom Lane wrote:
>> 0004: I prefer the solution I exhibited yesterday, ie add
>> externs to those headers.

> That solution seems fine, too.

Great.

> This comment should be clarified:
> +    # This redundant extern declaration is needed to silence headerscheck
> +    # warnings with some gcc versions.
> You could just write
> # silence -Wmissing-variable-declarations
> which also appears elsewhere in the code.

I compromised on writing "redundant declaration to silence
-Wmissing-variable-declarations", and pushed it.

            regards, tom lane