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

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