Обсуждение: gcc 13 warnings
Hi
see
[504/2287] Compiling C object src/backend/postgres_lib.a.p/access_transam_xlogrecovery.c.o
In function ‘recoveryStopsAfter’,
inlined from ‘PerformWalRecovery’ at ../src/backend/access/transam/xlogrecovery.c:1749:8:
../src/backend/access/transam/xlogrecovery.c:2737:42: warning: ‘recordXtime’ may be used uninitialized [-Wmaybe-uninitialized]
2737 | recoveryStopTime = recordXtime;
| ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
../src/backend/access/transam/xlogrecovery.c: In function ‘PerformWalRecovery’:
../src/backend/access/transam/xlogrecovery.c:2628:21: note: ‘recordXtime’ was declared here
2628 | TimestampTz recordXtime;
| ^~~~~~~~~~~
[1642/2287] Compiling C object src/bin/pgbench/pgbench.p/pgbench.c.o
In function ‘coerceToInt’,
inlined from ‘evalStandardFunc’ at ../src/bin/pgbench/pgbench.c:2617:11:
../src/bin/pgbench/pgbench.c:2042:17: warning: ‘vargs[0].type’ may be used uninitialized [-Wmaybe-uninitialized]
2042 | if (pval->type == PGBT_INT)
| ~~~~^~~~~~
../src/bin/pgbench/pgbench.c: In function ‘evalStandardFunc’:
../src/bin/pgbench/pgbench.c:2250:22: note: ‘vargs’ declared here
2250 | PgBenchValue vargs[MAX_FARGS];
| ^~~~~
In function ‘coerceToInt’,
inlined from ‘evalStandardFunc’ at ../src/bin/pgbench/pgbench.c:2617:11:
../src/bin/pgbench/pgbench.c:2044:32: warning: ‘vargs[0].u.ival’ may be used uninitialized [-Wmaybe-uninitialized]
2044 | *ival = pval->u.ival;
| ~~~~~~~^~~~~
../src/bin/pgbench/pgbench.c: In function ‘evalStandardFunc’:
../src/bin/pgbench/pgbench.c:2250:22: note: ‘vargs’ declared here
2250 | PgBenchValue vargs[MAX_FARGS];
| ^~~~~
In function ‘coerceToInt’,
inlined from ‘evalStandardFunc’ at ../src/bin/pgbench/pgbench.c:2617:11:
../src/bin/pgbench/pgbench.c:2049:40: warning: ‘vargs[0].u.dval’ may be used uninitialized [-Wmaybe-uninitialized]
2049 | double dval = rint(pval->u.dval);
| ^~~~~~~~~~~~~~~~~~
../src/bin/pgbench/pgbench.c: In function ‘evalStandardFunc’:
../src/bin/pgbench/pgbench.c:2250:22: note: ‘vargs’ declared here
2250 | PgBenchValue vargs[MAX_FARGS];
| ^~~~~
[1700/2287] 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:8307: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:8495:17: note: in expansion of macro ‘VARATT_IS_EXTERNAL_NON_EXPANDED’
8495 | VARATT_IS_EXTERNAL_NON_EXPANDED(DatumGetPointer(newvalue)))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function ‘exec_set_found’:
cc1: note: source object is likely at address zero
In function ‘recoveryStopsAfter’,
inlined from ‘PerformWalRecovery’ at ../src/backend/access/transam/xlogrecovery.c:1749:8:
../src/backend/access/transam/xlogrecovery.c:2737:42: warning: ‘recordXtime’ may be used uninitialized [-Wmaybe-uninitialized]
2737 | recoveryStopTime = recordXtime;
| ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
../src/backend/access/transam/xlogrecovery.c: In function ‘PerformWalRecovery’:
../src/backend/access/transam/xlogrecovery.c:2628:21: note: ‘recordXtime’ was declared here
2628 | TimestampTz recordXtime;
| ^~~~~~~~~~~
[1642/2287] Compiling C object src/bin/pgbench/pgbench.p/pgbench.c.o
In function ‘coerceToInt’,
inlined from ‘evalStandardFunc’ at ../src/bin/pgbench/pgbench.c:2617:11:
../src/bin/pgbench/pgbench.c:2042:17: warning: ‘vargs[0].type’ may be used uninitialized [-Wmaybe-uninitialized]
2042 | if (pval->type == PGBT_INT)
| ~~~~^~~~~~
../src/bin/pgbench/pgbench.c: In function ‘evalStandardFunc’:
../src/bin/pgbench/pgbench.c:2250:22: note: ‘vargs’ declared here
2250 | PgBenchValue vargs[MAX_FARGS];
| ^~~~~
In function ‘coerceToInt’,
inlined from ‘evalStandardFunc’ at ../src/bin/pgbench/pgbench.c:2617:11:
../src/bin/pgbench/pgbench.c:2044:32: warning: ‘vargs[0].u.ival’ may be used uninitialized [-Wmaybe-uninitialized]
2044 | *ival = pval->u.ival;
| ~~~~~~~^~~~~
../src/bin/pgbench/pgbench.c: In function ‘evalStandardFunc’:
../src/bin/pgbench/pgbench.c:2250:22: note: ‘vargs’ declared here
2250 | PgBenchValue vargs[MAX_FARGS];
| ^~~~~
In function ‘coerceToInt’,
inlined from ‘evalStandardFunc’ at ../src/bin/pgbench/pgbench.c:2617:11:
../src/bin/pgbench/pgbench.c:2049:40: warning: ‘vargs[0].u.dval’ may be used uninitialized [-Wmaybe-uninitialized]
2049 | double dval = rint(pval->u.dval);
| ^~~~~~~~~~~~~~~~~~
../src/bin/pgbench/pgbench.c: In function ‘evalStandardFunc’:
../src/bin/pgbench/pgbench.c:2250:22: note: ‘vargs’ declared here
2250 | PgBenchValue vargs[MAX_FARGS];
| ^~~~~
[1700/2287] 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:8307: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:8495:17: note: in expansion of macro ‘VARATT_IS_EXTERNAL_NON_EXPANDED’
8495 | VARATT_IS_EXTERNAL_NON_EXPANDED(DatumGetPointer(newvalue)))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function ‘exec_set_found’:
cc1: note: source object is likely at address zero
Regards
Pavel
On Thu, Mar 16, 2023 at 9:40 AM Pavel Stehule <pavel.stehule@gmail.com> wrote: > [1700/2287] 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:8307: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:8495:17: note: in expansion of macro ‘VARATT_IS_EXTERNAL_NON_EXPANDED’ > 8495 | VARATT_IS_EXTERNAL_NON_EXPANDED(DatumGetPointer(newvalue))) > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > In function ‘exec_set_found’: > cc1: note: source object is likely at address zero I see these with gcc 12.2.0 also.
Melanie Plageman <melanieplageman@gmail.com> writes: > On Thu, Mar 16, 2023 at 9:40 AM Pavel Stehule <pavel.stehule@gmail.com> wrote: >> ../src/include/varatt.h:230:36: warning: array subscript 0 is outside array bounds of ‘char[0]’ [-Warray-bounds=] > I see these with gcc 12.2.0 also. Hmm, I do not see any warnings on HEAD with Fedora 37's gcc 12.2.1. What non-default configure switches, CFLAGS, etc are you using? regards, tom lane
čt 16. 3. 2023 v 16:43 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
Melanie Plageman <melanieplageman@gmail.com> writes:
> On Thu, Mar 16, 2023 at 9:40 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> ../src/include/varatt.h:230:36: warning: array subscript 0 is outside array bounds of ‘char[0]’ [-Warray-bounds=]
> I see these with gcc 12.2.0 also.
Hmm, I do not see any warnings on HEAD with Fedora 37's gcc 12.2.1.
What non-default configure switches, CFLAGS, etc are you using?
meson build without any settings
I think so it is related to meson build, I didn't see these warnings with autoconf
regards
Pavel
regards, tom lane
Pavel Stehule <pavel.stehule@gmail.com> writes: > čt 16. 3. 2023 v 16:43 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal: >> Hmm, I do not see any warnings on HEAD with Fedora 37's gcc 12.2.1. >> What non-default configure switches, CFLAGS, etc are you using? > meson build without any settings > I think so it is related to meson build, I didn't see these warnings with > autoconf It wouldn't be entirely surprising if meson is selecting some -W switches that the configure script doesn't ... but I don't know where to check or change that. If that is the case, do we want to beat meson over the head till it stops doing that, or try to silence the warnings? The ones you show here don't look terribly helpful ... regards, tom lane
Hi, On 2023-03-16 12:10:27 -0400, Tom Lane wrote: > Pavel Stehule <pavel.stehule@gmail.com> writes: > > čt 16. 3. 2023 v 16:43 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal: > >> Hmm, I do not see any warnings on HEAD with Fedora 37's gcc 12.2.1. > >> What non-default configure switches, CFLAGS, etc are you using? > > > meson build without any settings > > I think so it is related to meson build, I didn't see these warnings with > > autoconf > > It wouldn't be entirely surprising if meson is selecting some -W > switches that the configure script doesn't ... but I don't know > where to check or change that. I think it's just that meson defaults to -O3 (fwiw, I see substantial gains of that over -O2). I see such warnings with autoconf as well if I make it use -O3. I think some of these are stemming from https://postgr.es/m/20230204130708.pta7pjc4dvu225ey%40alap3.anarazel.de Greetings, Andres Freund
Hi, On 2023-03-16 10:05:06 -0700, Andres Freund wrote: > I think it's just that meson defaults to -O3 (fwiw, I see substantial gains of > that over -O2). I see such warnings with autoconf as well if I make it use > -O3. WRT: In file included from /home/andres/src/postgresql/src/include/access/htup_details.h:22, from /home/andres/src/postgresql/src/pl/plpgsql/src/pl_exec.c:21: In function ‘assign_simple_var’, inlined from ‘exec_set_found’ at /home/andres/src/postgresql/src/pl/plpgsql/src/pl_exec.c:8307:2: /home/andres/src/postgresql/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) | ^ /home/andres/src/postgresql/src/include/varatt.h:94:12: note: in definition of macro ‘VARTAG_IS_EXPANDED’ 94 | (((tag) & ~1) == VARTAG_EXPANDED_RO) | ^~~ /home/andres/src/postgresql/src/include/varatt.h:284:57: note: in expansion of macro ‘VARTAG_1B_E’ 284 | #define VARTAG_EXTERNAL(PTR) VARTAG_1B_E(PTR) | ^~~~~~~~~~~ /home/andres/src/postgresql/src/include/varatt.h:301:57: note: in expansion of macro ‘VARTAG_EXTERNAL’ 301 | (VARATT_IS_EXTERNAL(PTR) && !VARTAG_IS_EXPANDED(VARTAG_EXTERNAL(PTR))) | ^~~~~~~~~~~~~~~ /home/andres/src/postgresql/src/pl/plpgsql/src/pl_exec.c:8495:17: note: in expansion of macro ‘VARATT_IS_EXTERNAL_NON_EXPANDED’ 8495 | VARATT_IS_EXTERNAL_NON_EXPANDED(DatumGetPointer(newvalue))) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ I think that's basically because gcc does realize that the datum is just an 8 byte by-value datum: assign_simple_var(estate, var, BoolGetDatum(state), false, false); but doesn't (and probably can't, with the available information) grok that that means we don't even get to the VARATT_IS_EXTERNAL_NON_EXPANDED() in assign_simple_var(). If I add if (var->datatype->typlen == -1) pg_unreachable(); to exec_set_found(), the warning indeed goes away. I've wondered before if we should make at least some Asserts() into something like the above (if we have something better backing it than abort()), so the compiler can understand unreachable code paths even when building without cassert. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2023-03-16 12:10:27 -0400, Tom Lane wrote: >> It wouldn't be entirely surprising if meson is selecting some -W >> switches that the configure script doesn't ... but I don't know >> where to check or change that. > I think it's just that meson defaults to -O3 (fwiw, I see substantial gains of > that over -O2). I see such warnings with autoconf as well if I make it use > -O3. Oh, interesting. Should we try to standardize the two build systems on the same -O level, and if so which one? To my mind, you should ideally get the identical built bits out of either system, so defaulting to a different -O level seems bad. I'm not sure if we're prepared to go to -O3 by default though, especially for some of the older buildfarm critters where that might be buggy. (I'd imagine you take a hit in gdb-ability too.) regards, tom lane
Hi, On 2023-03-16 13:54:29 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2023-03-16 12:10:27 -0400, Tom Lane wrote: > >> It wouldn't be entirely surprising if meson is selecting some -W > >> switches that the configure script doesn't ... but I don't know > >> where to check or change that. > > > I think it's just that meson defaults to -O3 (fwiw, I see substantial gains of > > that over -O2). I see such warnings with autoconf as well if I make it use > > -O3. > > Oh, interesting. Should we try to standardize the two build systems > on the same -O level, and if so which one? I'm on the fence on this one (and posed it as a question before). O3 does result in higher performance for me, but it also does take longer to build, and increases the numbers of warnings. So I just elected to leave it at the default for meson. > To my mind, you should ideally get the identical built bits out of > either system, so defaulting to a different -O level seems bad. I doubt that is attainable, unfortunately. My experience is that even trivial changes can lead to substantial changes in output. Even just being in a different directory (the root build directory for meson vs the subdirectory in make builds) apparently sometimes leads to different compiler output. > I'm not sure if we're prepared to go to -O3 by default though, > especially for some of the older buildfarm critters where that > might be buggy. (I'd imagine you take a hit in gdb-ability too.) My experience is that debuggability is already bad enough at O2 that the difference to O3 is pretty marginal. But it certainly depends a bit on the compiler version and what level of debug information one enables. Greetings, Andres Freund
On Fri, Mar 17, 2023 at 1:11 AM Andres Freund <andres@anarazel.de> wrote:
> On 2023-03-16 13:54:29 -0400, Tom Lane wrote:
> So I just elected to leave it at the default for meson.
In my build scripts I've been setting it to -O2, because that seemed the obvious thing to do, and assumed some later commit would get rid of the need to do it manually. (if it was discussed before, I missed that)
> > I'm not sure if we're prepared to go to -O3 by default though,
> > especially for some of the older buildfarm critters where that
> > might be buggy. (I'd imagine you take a hit in gdb-ability too.)
Newer platforms could be buggy enough. A while back, IIUC gcc moved an optimization pass from O3 to O2, which resulted in obviously bad code generation, which I know because of a bug report filed by one Andres Freund:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101481
...which was never properly addressed as far as I know.
I'm a bit surprised we would even consider changing optimization level based on a build tool default.
--
John Naylor
EDB: http://www.enterprisedb.com
> On 2023-03-16 13:54:29 -0400, Tom Lane wrote:
> So I just elected to leave it at the default for meson.
In my build scripts I've been setting it to -O2, because that seemed the obvious thing to do, and assumed some later commit would get rid of the need to do it manually. (if it was discussed before, I missed that)
> > I'm not sure if we're prepared to go to -O3 by default though,
> > especially for some of the older buildfarm critters where that
> > might be buggy. (I'd imagine you take a hit in gdb-ability too.)
Newer platforms could be buggy enough. A while back, IIUC gcc moved an optimization pass from O3 to O2, which resulted in obviously bad code generation, which I know because of a bug report filed by one Andres Freund:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101481
...which was never properly addressed as far as I know.
I'm a bit surprised we would even consider changing optimization level based on a build tool default.
--
John Naylor
EDB: http://www.enterprisedb.com
On 16.03.23 19:11, Andres Freund wrote: > On 2023-03-16 13:54:29 -0400, Tom Lane wrote: >> Andres Freund <andres@anarazel.de> writes: >>> On 2023-03-16 12:10:27 -0400, Tom Lane wrote: >>>> It wouldn't be entirely surprising if meson is selecting some -W >>>> switches that the configure script doesn't ... but I don't know >>>> where to check or change that. >> >>> I think it's just that meson defaults to -O3 (fwiw, I see substantial gains of >>> that over -O2). I see such warnings with autoconf as well if I make it use >>> -O3. >> >> Oh, interesting. Should we try to standardize the two build systems >> on the same -O level, and if so which one? > > I'm on the fence on this one (and posed it as a question before). O3 does > result in higher performance for me, but it also does take longer to build, > and increases the numbers of warnings. > > So I just elected to leave it at the default for meson. AFAICT, the default for meson is buildtype=debug, which is -O0. The -O3 comes from meson.build setting buildtype=release. I think a good compromise would be buildtype=debugoptimized, which is -O2 with debug symbols, which also sort of matches the default in the autoconf world. (https://mesonbuild.com/Builtin-options.html#details-for-buildtype) At least during the transition phase I would prefer having the same default optimization level in both build systems, mainly because of how this affects warnings.
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes: > On 16.03.23 19:11, Andres Freund wrote: >> So I just elected to leave it at the default for meson. > AFAICT, the default for meson is buildtype=debug, which is -O0. The -O3 > comes from meson.build setting buildtype=release. > I think a good compromise would be buildtype=debugoptimized, which is > -O2 with debug symbols, which also sort of matches the default in the > autoconf world. That sounds promising. > At least during the transition phase I would prefer having the same > default optimization level in both build systems, mainly because of how > this affects warnings. I'd prefer sticking to -O2 mainly because of the risk of new bugs. The meson conversion is a big enough job without adding "harden Postgres against -O3" to the list of tasks that must be accomplished. We can take that on in due time, but let's keep it separate. regards, tom lane
Hi, On 2023-03-17 09:06:05 +0100, Peter Eisentraut wrote: > AFAICT, the default for meson is buildtype=debug, which is -O0. The -O3 > comes from meson.build setting buildtype=release. Right - my point about -O3 was just that buildtype=release defaults to it. > I think a good compromise would be buildtype=debugoptimized, which is -O2 > with debug symbols, which also sort of matches the default in the autoconf > world. Looks like that'd result in a slightly worse build with msvc, as afaict we wouldn't end up with /OPT:REF doesn't get specified, which automatically gets disabled if /DEBUG is specified. I guess we can live with that. Greetings, Andres Freund
On 18.03.23 00:54, Andres Freund wrote: >> I think a good compromise would be buildtype=debugoptimized, which is -O2 >> with debug symbols, which also sort of matches the default in the autoconf >> world. > > Looks like that'd result in a slightly worse build with msvc, as afaict we > wouldn't end up with /OPT:REF doesn't get specified, which automatically gets > disabled if /DEBUG is specified. I guess we can live with that. I looked up what /OPT:REF does (https://learn.microsoft.com/en-us/cpp/build/reference/opt-optimizations?view=msvc-170), and it seems pretty obscure to me, at least for development builds.
On 22.03.23 10:45, Peter Eisentraut wrote: > On 18.03.23 00:54, Andres Freund wrote: >>> I think a good compromise would be buildtype=debugoptimized, which is >>> -O2 >>> with debug symbols, which also sort of matches the default in the >>> autoconf >>> world. >> >> Looks like that'd result in a slightly worse build with msvc, as >> afaict we >> wouldn't end up with /OPT:REF doesn't get specified, which >> automatically gets >> disabled if /DEBUG is specified. I guess we can live with that. > > I looked up what /OPT:REF does > (https://learn.microsoft.com/en-us/cpp/build/reference/opt-optimizations?view=msvc-170), and it seems pretty obscure tome, at least for development builds. I have committed the change of buildtype to debugoptimized.
Hi, > I have committed the change of buildtype to debugoptimized. 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. `` Displayed only for the release builds, e.g.: ``` git clean -dfx meson setup --buildtype release -DPG_TEST_EXTRA="kerberos ldap ssl" -Dldap=disabled -Dssl=openssl -Dcassert=true -Dtap_tests=enabled -Dprefix=/home/eax/pginstall build ninja -C build ``` Compiler version is: ``` gcc (Debian 12.2.0-14) 12.2.0 ``` The overall environment is Raspberry Pi 5 with pretty much default configuration - Raspbian etc. How to fix it? Absolutely no idea :) -- Best regards, Aleksander Alekseev
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.
Hi, > /* > * 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]). IMO we shouldn't allow warnings to appear in release builds, even harmless ones. Otherwise we start ignoring them and will skip something important one day. So I think we should do this. -- Best regards, Aleksander Alekseev