Re: gcc 13 warnings

Hi,

On 2024-07-05 14:19:12 +0300, Aleksander Alekseev wrote:
> There is still a warning previously reported by Melanie:
>
> ```
> [1391/1944] Compiling C object src/pl/plpgsql/src/plpgsql.so.p/pl_exec.c.o
> In file included from ../src/include/access/htup_details.h:22,
> from ../src/pl/plpgsql/src/pl_exec.c:21:
> In function ‘assign_simple_var’,
> inlined from ‘exec_set_found’ at ../src/pl/plpgsql/src/pl_exec.c:8382: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)))
> | ^~~~~~~~~~~~~~~
> ../src/pl/plpgsql/src/pl_exec.c:8570:17: note: in expansion of macro
> ‘VARATT_IS_EXTERNAL_NON_EXPANDED’
> 8570 |
> VARATT_IS_EXTERNAL_NON_EXPANDED(DatumGetPointer(newvalue)))
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> [1687/1944] Compiling C object
> src/test/modules/test_dsa/test_dsa.so.p/test_dsa.c.o^C
> ninja: build stopped: interrupted by user.
> ``

> The overall environment is Raspberry Pi 5 with pretty much default
> configuration - Raspbian etc.
>
> How to fix it? Absolutely no idea :)

I think it's actually a somewhat reasonable warning - the compiler can't know
that in exec_set_found() we'll always deal with typlen == 1 and thus can't
ever reach the inside of the branch it warns about.

Once the compiler knows about that "restriction", the warning vanishes. Try
adding the following to exec_set_found():

/*
* Prevent spurious warning due to compiler not realizing
* VARATT_IS_EXTERNAL_NON_EXPANDED() branch in assign_simple_var() isn't
* reachable due to "found" being byvalue.
*/
if (var->datatype->typlen != 1)
pg_unreachable();

I'm somewhat inclined to think it'd be worth adding something along those
lines to avoid this warning ([1]).

Greetings,

Andres Freund

[1]

In general we're actually hiding a lot of useful information from the compiler
in release builds, due to asserts not being enabled. I've been wondering about
a version of Assert() that isn't completely removed in release builds but
instead transform into the pg_unreachable() form for compilers with an
"efficient" pg_unreachable() (i.e. not using abort()).

We can't just do that for all asserts though, it only makes sense for ones
that are "trivial" in some form (i.e. the compiler can realize it doesn't
have side effects and doesn't need be generated).

That's about generating more optimized code though.