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

Hi,

On 2025-07-12 13:42:54 -0400, Tom Lane wrote:
> Dmitry Mityugov <d.mityugov@postgrespro.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.

That shouldn't be a problem - pg_assume() is defined to be an Assert() in
USE_ASSERT_CHECKING builds.


> 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.

I had played with using pg_assume here too, but I couldn't really convince
myself that it's a good idea...


> 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).

WFM.

Greetings,

Andres Freund