Thread: New compiler warnings in buildfarm

Sometime in the last month or so, flaviventris's bleeding-edge
version of gcc has started whining[1] about truncation of a
string literal's implicit trailing '\0' in contexts like this:

../pgsql/src/backend/commands/copyto.c:106:41: warning: initializer-string for array of 'char' is too long
[-Wunterminated-string-initialization]
  106 | static const char BinarySignature[11] = "PGCOPY\n\377\r\n\0";
      |                                         ^~~~~~~~~~~~~~~~~~~~

../pgsql/src/backend/utils/adt/numutils.c:29:1: warning: initializer-string for array of 'char' is too long
[-Wunterminated-string-initialization]
   29 | "00" "01" "02" "03" "04" "05" "06" "07" "08" "09"
      | ^~~~

Presumably this'll appear in less-bleeding-edge releases in a
few months' time.

In the BinarySignature case, we could silence it in at least two ways.
We could remove the explicit trailing \0 and rely on the implicit one:

-static const char BinarySignature[11] = "PGCOPY\n\377\r\n\0";
+static const char BinarySignature[11] = "PGCOPY\n\377\r\n";

Or just drop the unnecessary array length specification:

-static const char BinarySignature[11] = "PGCOPY\n\377\r\n\0";
+static const char BinarySignature[] = "PGCOPY\n\377\r\n\0";

Or we could do both things, but that feels perhaps too magic.

In the numutils.c case, I think that dropping the array length
spec is the only reasonable fix, since the last desired character
isn't a \0:

-static const char DIGIT_TABLE[200] =
+static const char DIGIT_TABLE[] =
 "00" "01" "02" "03" "04" "05" "06" "07" "08" "09"
 ...
 "90" "91" "92" "93" "94" "95" "96" "97" "98" "99";

There is one more similar complaint:

../pgsql/contrib/fuzzystrmatch/daitch_mokotoff.c:92:20: warning: initializer-string for array of 'char' is too long
[-Wunterminated-string-initialization]
   92 |         .soundex = "000000",            /* Six digits */
      |                    ^~~~~~~~

Here, the struct field is declared

    char        soundex[DM_CODE_DIGITS];    /* Soundex code */

and we probably don't want to mess with that.  However, elsewhere in
the same struct initialization I see

    char        prev_code_digits[2];
    char        next_code_digits[2];

...

    .prev_code_digits = {'\0', '\0'},
    .next_code_digits = {'\0', '\0'},

and that's *not* drawing a warning.  So the most plausible fix
seems to be

-         .soundex = "000000",            /* Six digits */
+         .soundex = {'0', '0', '0', '0', '0', '0'},  /* Six digits */

(In principle, we could fix the COPY and numutils cases the same
way, but I don't care for the readability loss that'd entail.)

Preferences, other suggestions?

            regards, tom lane

[1] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=flaviventris&dt=2024-07-30%2012%3A29%3A41&stg=build

On 30.07.24 18:19, Tom Lane wrote:
> Sometime in the last month or so, flaviventris's bleeding-edge
> version of gcc has started whining[1] about truncation of a
> string literal's implicit trailing '\0' in contexts like this:
> 
> ../pgsql/src/backend/commands/copyto.c:106:41: warning:
> initializer-string for array of 'char' is too long
> [-Wunterminated-string-initialization]
>    106 | static const char BinarySignature[11] = "PGCOPY\n\377\r\n\0";
>        |                                         ^~~~~~~~~~~~~~~~~~~~
> 
> ../pgsql/src/backend/utils/adt/numutils.c:29:1: warning:
> initializer-string for array of 'char' is too long
> [-Wunterminated-string-initialization]
>     29 | "00" "01" "02" "03" "04" "05" "06" "07" "08" "09"
>        | ^~~~
> 
> Presumably this'll appear in less-bleeding-edge releases in a
> few months' time.

According to the gcc documentation, this warning is part of -Wextra. 
And indeed flaviventris runs with -Wextra:

'CFLAGS' => '-O1 -ggdb -g3 -fno-omit-frame-pointer -Wall -Wextra 
-Wno-unused-parameter -Wno-sign-compare -Wno-missing-field-initializers 
-O0',

So I think the appropriate fix here for now is to add 
-Wno-unterminated-string-initialization to this buildfarm configuration.

Maybe we find this warning useful, in which case we should add 
-Wunterminated-string-initialization to the standard set of warning 
options before undertaking code changes.  But gcc-15 is still about a 
year away from being released, so it seems too early for that.

Peter Eisentraut <peter@eisentraut.org> writes:
> On 30.07.24 18:19, Tom Lane wrote:
>> Sometime in the last month or so, flaviventris's bleeding-edge
>> version of gcc has started whining[1] about truncation of a
>> string literal's implicit trailing '\0' in contexts like this:
>> ../pgsql/src/backend/commands/copyto.c:106:41: warning:
>> initializer-string for array of 'char' is too long
>> [-Wunterminated-string-initialization]
>> 106 | static const char BinarySignature[11] = "PGCOPY\n\377\r\n\0";
>> |                                         ^~~~~~~~~~~~~~~~~~~~

> According to the gcc documentation, this warning is part of -Wextra. 
> And indeed flaviventris runs with -Wextra:

> 'CFLAGS' => '-O1 -ggdb -g3 -fno-omit-frame-pointer -Wall -Wextra 
> -Wno-unused-parameter -Wno-sign-compare -Wno-missing-field-initializers 
> -O0',

Ah --- and it was not doing that a month ago.  So maybe the compiler
has had this warning for longer.  I don't see it with gcc 13.3 though,
which is the newest I have handy.

> So I think the appropriate fix here for now is to add 
> -Wno-unterminated-string-initialization to this buildfarm configuration.

Agreed; our policy so far has been to treat -Wextra warnings with
suspicion, and there is not anything really wrong with these bits
of code.

It looks like serinus needs this fix too.

            regards, tom lane

Hi,

On 2024-07-31 10:11:07 -0400, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
> > On 30.07.24 18:19, Tom Lane wrote:
> >> Sometime in the last month or so, flaviventris's bleeding-edge
> >> version of gcc has started whining[1] about truncation of a
> >> string literal's implicit trailing '\0' in contexts like this:
> >> ../pgsql/src/backend/commands/copyto.c:106:41: warning:
> >> initializer-string for array of 'char' is too long
> >> [-Wunterminated-string-initialization]
> >> 106 | static const char BinarySignature[11] = "PGCOPY\n\377\r\n\0";
> >> |                                         ^~~~~~~~~~~~~~~~~~~~
>
> > According to the gcc documentation, this warning is part of -Wextra.
> > And indeed flaviventris runs with -Wextra:
>
> > 'CFLAGS' => '-O1 -ggdb -g3 -fno-omit-frame-pointer -Wall -Wextra
> > -Wno-unused-parameter -Wno-sign-compare -Wno-missing-field-initializers
> > -O0',
>
> Ah --- and it was not doing that a month ago.

Hm? I've not touched flaviventris config since at least the 26th of March. And
a buildfarm run from before then also shows -Wextra:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=flaviventris&dt=2024-03-17%2011%3A17%3A01


> So maybe the compiler has had this warning for longer.

It's very new:

commit 44c9403ed1833ae71a59e84f9e37af3182be0df5
Author:     Alejandro Colomar <alx@kernel.org>
AuthorDate: 2024-06-29 15:10:43 +0200
Commit:     Martin Uecker <uecker@gcc.gnu.org>
CommitDate: 2024-07-14 11:41:00 +0200

    c, objc: Add -Wunterminated-string-initialization


It might be worth piping up in the gcc bugtracker and suggesting that the
warning isn't issued when there's an explicit \0?


> > So I think the appropriate fix here for now is to add
> > -Wno-unterminated-string-initialization to this buildfarm configuration.
>
> Agreed; our policy so far has been to treat -Wextra warnings with
> suspicion, and there is not anything really wrong with these bits
> of code.
>
> It looks like serinus needs this fix too.

Added to both.  I've forced runs for both animals, so the bf should show
results of that soon.

Greetings,

Andres Freund

Hi,

On 2024-07-31 11:32:56 -0700, Andres Freund wrote:
> On 2024-07-31 10:11:07 -0400, Tom Lane wrote:
> > It looks like serinus needs this fix too.
>
> Added to both.  I've forced runs for both animals, so the bf should show
> results of that soon.

I Wonder if I should also should add -Wno-clobbered to serinus' config. Afaict
-Wclobbered is pretty useless once optimizations are used. I've long added
that to my local dev environment flags because it's so noisy (which is too
bad, in theory a good warning for this would be quite helpful).

Or whether we should just do that on a project level?

Greetings,

Andres Freund

On 31.07.24 20:39, Andres Freund wrote:
> On 2024-07-31 11:32:56 -0700, Andres Freund wrote:
>> On 2024-07-31 10:11:07 -0400, Tom Lane wrote:
>>> It looks like serinus needs this fix too.
>>
>> Added to both.  I've forced runs for both animals, so the bf should show
>> results of that soon.
> 
> I Wonder if I should also should add -Wno-clobbered to serinus' config. Afaict
> -Wclobbered is pretty useless once optimizations are used. I've long added
> that to my local dev environment flags because it's so noisy (which is too
> bad, in theory a good warning for this would be quite helpful).

It's unclear to me what to make of this.  We have in the past fixed a 
number of these, IIRC, and clearly in theory the risk that the warning 
points out does exist.  But these warnings appear erratic and 
inconsistent.  I'm using the same compiler versions but I don't see any 
of these warnings.  So I don't understand exactly what triggers these.

Peter Eisentraut <peter@eisentraut.org> writes:
> On 31.07.24 20:39, Andres Freund wrote:
>> I Wonder if I should also should add -Wno-clobbered to serinus' config. Afaict
>> -Wclobbered is pretty useless once optimizations are used. I've long added
>> that to my local dev environment flags because it's so noisy (which is too
>> bad, in theory a good warning for this would be quite helpful).

> It's unclear to me what to make of this.  We have in the past fixed a
> number of these, IIRC, and clearly in theory the risk that the warning
> points out does exist.  But these warnings appear erratic and
> inconsistent.  I'm using the same compiler versions but I don't see any
> of these warnings.  So I don't understand exactly what triggers these.

Yeah, -Wclobbered's results seem to vary quite a lot across different
compiler versions and perhaps different compiler options.  I'd be more
excited about trying to silence it if there were some consistency to
the reports, but there's not that much; plus, we've never seen any
evidence that the reports from the noisier compilers correspond to
real bugs.

Just for context, here's a quick count of -Wclobbered warnings in
the buildfarm:

     71 calliphoridae
     66 canebrake
     71 culicidae
     67 grassquit
     65 serinus
     89 skink
     66 taipan
     68 tamandua

The other hundred-plus animals report zero such warnings.

I also tried slicing the data by the variable being complained of:

$ grep 'Wclobbered' currentwarnings | sed -e 's/.*: argument //' -e 's/.*: variable //' | awk '{print $1}' | sort |
uniq-c 

    118 '_do_rethrow'
     24 '_do_rethrow2'
      8 'arrayname'
      6 'bump_level'
      1 'cell__state'
      7 'commandCollected'
      8 'commands'
      3 'cstr'
      6 'cur_datname'
      6 'cur_nspname'
      6 'cur_relname'
      7 'data'
      2 'dboid'
      8 'dbstrategy'
      8 'elevel'
     14 'error'
      1 'fd'
      8 'found_concurrent_worker'
      4 'ft_htab'
      8 'has_pending_wal'
      8 'ib'
      8 'import_collate'
      8 'import_default'
      8 'import_generated'
      8 'import_not_null'
      1 'is_program'
      1 'iter'
      8 'loop_body'
      8 'method'
      6 'named_arg_strings'
      7 'nulls'
      5 'objname'
      1 'options'
      7 'params'
      8 'primary'
      8 'processed'
      8 'rel'
      8 'relations'
      8 'relids_logged'
      8 'reltuples'
     44 'result'
      8 'retry'
     17 'retval'
     16 'rv'
      6 'seq_relids'
      8 'sqlstate'
      8 'stats'
      3 'success'
      8 'switch_lsn'
      8 'switch_to_superuser'
      8 'sync_slotname'
      5 'tab'
      7 'table_oids'
      8 'tb'
      6 'update_failover'
      2 'update_tuple'
      8 'update_two_phase'
      8 'vob'

That shows that the apparent similarity of the total number of reports
per animal is illusory: there are some that all eight animals agree
on, but a lot where they don't.

            regards, tom lane