Re: patch: Use pg_assume in jsonb_util.c to fix GCC 15 warnings

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Dmitry Mityugov <d(dot)mityugov(at)postgrespro(dot)ru>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: patch: Use pg_assume in jsonb_util.c to fix GCC 15 warnings
Date: 2025-07-12 17:42:54
Message-ID: 82868.1752342174@sss.pgh.pa.us
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Dmitry Mityugov <d(dot)mityugov(at)postgrespro(dot)ru> writes:
> When compiled with Assert() macro disabled, GCC 15 produces warnings
> about possibly uninitialized variables in
> src/backend/utils/adt/jsonb_util.c module. This problem was discussed in
> detail in this thread, in April 2025:
> https://www.postgresql.org/message-id/988bf1bc-3f1f-99f3-bf98-222f1cd9dc5e@xs4all.nl
> .

> Recently introduced pg_assume() macro let fix such problems easily. The
> attached patch fixes them in jsonb_util.c module. I verified that
> PostgreSQL compiles clearly with this patch and GCC 15.1.1 on an x86
> 64-bit machine (with and without --enable-cassert), and with GCC 14.2.1
> on a 64-bit ARM machine. `make check` also passes.

I don't care for this patch: replacing an Assert with pg_assume just
seems like a very bad idea. If the assertion condition ever failed,
which doesn't seem all that impossible in logic as complicated as
this, then instead of an identifiable assertion trap we'd get
unspecified and impossible-to-debug behavior.

We could conceivably do

+#ifdef USE_ASSERT_CHECKING
Assert(ra != WJB_END_ARRAY && ra != WJB_END_OBJECT);
Assert(rb != WJB_END_ARRAY && rb != WJB_END_OBJECT);
+#else
+ pg_assume(ra != WJB_END_ARRAY && ra != WJB_END_OBJECT);
+ pg_assume(rb != WJB_END_ARRAY && rb != WJB_END_OBJECT);
+#endif

but that seems ugly. In any case, it's just doubling down on the
assumption that a compiler capable of detecting the "may be used
uninitialized" issue will conclude that rejecting WJB_END_ARRAY
and WJB_END_OBJECT (but not WJB_DONE) eliminates the possibility of
va.type/vb.type not being set.

What I think we should do about this is what I mentioned in the
other thread: adjust the code to guarantee that va.type/vb.type
are defined in all cases. There are a couple of ways we could
do that, but after reflection I think the best way is to modify
JsonbIteratorNext to make that guarantee. I've checked that
the attached silences the warning on gcc 15.1.1 (current
Fedora 42).

regards, tom lane

Attachment Content-Type Size
v2-silence-uninitialized-warnings.patch text/x-diff 2.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2025-07-12 17:55:32 Re: patch: Use pg_assume in jsonb_util.c to fix GCC 15 warnings
Previous Message Greg Sabino Mullane 2025-07-12 15:29:43 Re: PATCH: warn about, and deprecate, clear text passwords