Feedback-ID: id4a34324:Fastmail
Date: Wed, 7 Feb 2024 12:31:38 -0800
From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Alexander Korotkov <akorotkov@postgresql.org>,
	pgsql-hackers@lists.postgresql.org
Subject: gcc build warnings at -O3
Message-ID: <20240207203138.sknifhlppdtgtxnk@awork3.anarazel.de>
References: <E1qsBJZ-000l5A-FT@gemulon.postgresql.org>
 <CALj2ACVQoFXxFm2kCmhHcdM7DjA84_bOjoM8HVAKHbE+KrZ1uA@mail.gmail.com>
 <375043.1705028119@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <375043.1705028119@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/20240207203138.sknifhlppdtgtxnk%40awork3.anarazel.de>
Precedence: bulk

Hi,

On 2024-01-11 21:55:19 -0500, Tom Lane wrote:
> Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> > Hi, I'm seeing a compiler warning with CFLAGS -O3 but not with -O2.
> 
> > In file included from dbcommands.c:20:
> > dbcommands.c: In function ‘createdb’:
> > ../../../src/include/postgres.h:104:16: warning: ‘src_hasloginevt’ may
> > be used uninitialized in this function [-Wmaybe-uninitialized]
> 
> Hmm, I also see that at -O3 (not at -O2) when using Fedora 39's
> gcc 13.2.1, but *not* when using RHEL8's gcc 8.5.0.

It's visible here with gcc >= 10. That's enough versions that I think we
should care.  Interestingly enough, it seems to have recently have gotten
fixed in gcc master (14 to be).


> Some of them match up with warnings we're seeing on buildfarm member
> serinus, which I seem to recall that Andres had tracked to a known gcc bug.

Some, but I don't think all.


> In file included from ../../../../src/include/executor/instrument.h:16,
>                  from pgstat_io.c:19:
> pgstat_io.c: In function 'pgstat_count_io_op_time':
> pgstat_io.c:149:60: warning: array subscript 2 is above array bounds of 'instr_time[2][4][8]' [-Warray-bounds=]
>   149 |                 INSTR_TIME_ADD(PendingIOStats.pending_times[io_object][io_context][io_op],
>       |                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~

Huh, I don't see that with any version of gcc I tried.


> In file included from ../../../../src/include/access/htup_details.h:22,
>                  from pl_exec.c:21:
> In function 'assign_simple_var',
>     inlined from 'exec_set_found' at pl_exec.c:8349:2:
> ../../../../src/include/varatt.h:230:36: warning: array subscript 0 is outside array bounds of 'char[0]' [-Warray-bounds=]
>   230 |         (((varattrib_1b_e *) (PTR))->va_tag)
>       |                                    ^
> ../../../../src/include/varatt.h:94:12: note: in definition of macro 'VARTAG_IS_EXPANDED'
>    94 |         (((tag) & ~1) == VARTAG_EXPANDED_RO)
>       |            ^~~
> ../../../../src/include/varatt.h:284:57: note: in expansion of macro 'VARTAG_1B_E'
>   284 | #define VARTAG_EXTERNAL(PTR)                            VARTAG_1B_E(PTR)
>       |                                                         ^~~~~~~~~~~
> ../../../../src/include/varatt.h:301:57: note: in expansion of macro 'VARTAG_EXTERNAL'
>   301 |         (VARATT_IS_EXTERNAL(PTR) && !VARTAG_IS_EXPANDED(VARTAG_EXTERNAL(PTR)))
>       |                                                         ^~~~~~~~~~~~~~~
> pl_exec.c:8537:17: note: in expansion of macro 'VARATT_IS_EXTERNAL_NON_EXPANDED'
>  8537 |                 VARATT_IS_EXTERNAL_NON_EXPANDED(DatumGetPointer(newvalue)))
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In function 'exec_set_found':
> cc1: note: source object is likely at address zero

This I see.  If I hint to the compiler that var->datatype->typlen != 1 when
called from exec_set_found(), the warning vanishes. E.g. with

	if (var->datatype->typlen == -1)
		__builtin_unreachable();

I see one more warning:

[1390/2375 42  58%] Compiling C object src/backend/postgres_lib.a.p/utils_adt_jsonb_util.c.o
../../../../../home/andres/src/postgresql/src/backend/utils/adt/jsonb_util.c: In function 'compareJsonbContainers':
../../../../../home/andres/src/postgresql/src/backend/utils/adt/jsonb_util.c:296:34: warning: 'va.type' may be used uninitialized [-Wmaybe-uninitialized]
  296 |                         res = (va.type > vb.type) ? 1 : -1;
      |                                ~~^~~~~
../../../../../home/andres/src/postgresql/src/backend/utils/adt/jsonb_util.c:204:33: note: 'va' declared here
  204 |                 JsonbValue      va,
      |                                 ^~


I can't really blame the compiler here.  There's a fairly lengthy comment
explaining that va.type/vb.type are set, and it took me a while to understand:

			/*
			 * It's safe to assume that the types differed, and that the va
			 * and vb values passed were set.
			 *
			 * If the two values were of the same container type, then there'd
			 * have been a chance to observe the variation in the number of
			 * elements/pairs (when processing WJB_BEGIN_OBJECT, say). They're
			 * either two heterogeneously-typed containers, or a container and
			 * some scalar type.
			 *
			 * We don't have to consider the WJB_END_ARRAY and WJB_END_OBJECT
			 * cases here, because we would have seen the corresponding
			 * WJB_BEGIN_ARRAY and WJB_BEGIN_OBJECT tokens first, and
			 * concluded that they don't match.
			 */

It's not surprising that the compiler can't understand that you can't get
ra = WJB_DONE, rb = WJB_END_ARRAY or such.

Greetings,

Andres Freund