Thread: gcc 13 warnings

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

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