Обсуждение: compiler warnings with gcc 4.8 and -Og
forking: <20220302205058.GJ15744@telsasoft.com>: Re: Adding CI to our tree On Wed, Mar 02, 2022 at 02:50:58PM -0600, Justin Pryzby wrote: > BTW (regarding the last patch), I just noticed that -Og optimization can cause > warnings with gcc-4.8.5-39.el7.x86_64. > > be-fsstubs.c: In function 'be_lo_export': > be-fsstubs.c:522:24: warning: 'fd' may be used uninitialized in this function [-Wmaybe-uninitialized] > if (CloseTransientFile(fd) != 0) > ^ > trigger.c: In function 'ExecCallTriggerFunc': > trigger.c:2400:2: warning: 'result' may be used uninitialized in this function [-Wmaybe-uninitialized] > return (HeapTuple) DatumGetPointer(result); > ^ > xml.c: In function 'xml_pstrdup_and_free': > xml.c:1205:2: warning: 'result' may be used uninitialized in this function [-Wmaybe-uninitialized] > return result; Today's "warnings" thread suggests to me that these are worth fixing - it seems reasonable to compile postgres 14 on centos7 (as I sometimes have done), and the patch seems even more reasonable when backpatched to older versions. (Also, I wonder if there's any consideration to backpatch cirrus.yaml, which uses -Og) The buildfarm has old GCC, but they all use -O2, so the warnings are not seen there. The patch below applies and fixes warnings back to v13. In v13, pl_handler.c has another warning, which suggests to backpatch 7292fd8f1. In v12, there's a disparate separate set of warnings which could be dealt with separately. v9.3-v11 have no warnings on c7 with -Og. Thomas mentioned [0] that cfbot's linux (which is using gcc 10) gives other warnings since using -Og, which (in addition to being unpleasant to look at) is hard to accept, seeing as there's a whole separate task just for "CompilerWarnings"... But I don't know what to do about those. [0] https://www.postgresql.org/message-id/CA+hUKGK1cF+TMW1cyoujoDAX5FBdoA59C--1HT7yCQGBbq1ddQ@mail.gmail.com diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 40441fdb4c..bb64de2843 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -2105,7 +2105,7 @@ ExecCallTriggerFunc(TriggerData *trigdata, { LOCAL_FCINFO(fcinfo, 0); PgStat_FunctionCallUsage fcusage; - Datum result; + Datum result = 0; MemoryContext oldContext; /* diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c index 63eaccc80a..3e2c094e1e 100644 --- a/src/backend/libpq/be-fsstubs.c +++ b/src/backend/libpq/be-fsstubs.c @@ -467,7 +467,7 @@ be_lo_export(PG_FUNCTION_ARGS) { Oid lobjId = PG_GETARG_OID(0); text *filename = PG_GETARG_TEXT_PP(1); - int fd; + int fd = -1; int nbytes, tmp; char buf[BUFSIZE]; diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c index f90a9424d4..7ffbae5a09 100644 --- a/src/backend/utils/adt/xml.c +++ b/src/backend/utils/adt/xml.c @@ -1185,7 +1185,7 @@ pg_xmlCharStrndup(const char *str, size_t len) static char * xml_pstrdup_and_free(xmlChar *str) { - char *result; + char *result = NULL; if (str) { @@ -1199,8 +1199,6 @@ xml_pstrdup_and_free(xmlChar *str) } PG_END_TRY(); } - else - result = NULL; return result; }
On Wed, Jun 01, 2022 at 09:42:44PM -0500, Justin Pryzby wrote: > Today's "warnings" thread suggests to me that these are worth fixing - it seems > reasonable to compile postgres 14 on centos7 (as I sometimes have done), and > the patch seems even more reasonable when backpatched to older versions. > (Also, I wonder if there's any consideration to backpatch cirrus.yaml, which > uses -Og) https://en.wikipedia.org/wiki/CentOS#CentOS_releases tells that centos 7 will be supported until the end of 2024, so I would fix that. > The patch below applies and fixes warnings back to v13. I don't mind fixing what you have here, as a first step. All those cases are telling us that the compiler does not see PG_TRY() as something is can rely on to set up each variable. 7292fd8 is complaining about the same point, actually, aka setjmp clobberring the variable, isn't it? So wouldn't it be better to initialize them, as your patch does, but also mark them as volatile? In short, what happens with -Wclobber and a non-optimized compilation? -- Michael
Вложения
Justin Pryzby <pryzby@telsasoft.com> writes:
> forking: <20220302205058.GJ15744@telsasoft.com>: Re: Adding CI to our tree
> On Wed, Mar 02, 2022 at 02:50:58PM -0600, Justin Pryzby wrote:
>> BTW (regarding the last patch), I just noticed that -Og optimization can cause
>> warnings with gcc-4.8.5-39.el7.x86_64.
I'm a little dubious about whether -Og is a case we should pay special
attention to? Our standard optimization setting for gcc is -O2, and
once you go away from that there are any number of weird cases that
may or may not produce warnings. I'm not entirely willing to buy
the proposition that we must suppress warnings on
any-random-gcc-version combined with any-random-options.
regards, tom lane
> On 2 Jun 2022, at 07:09, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I'm a little dubious about whether -Og is a case we should pay special > attention to? Our standard optimization setting for gcc is -O2, and > once you go away from that there are any number of weird cases that > may or may not produce warnings. I think we should pick one level to keep warning free, and stick to that. In light of that, -O2 seems a lot more appealing than -Og. -- Daniel Gustafsson https://vmware.com/
On Thu, 2 Jun 2022, 07:10 Tom Lane, <tgl@sss.pgh.pa.us> wrote: > > Justin Pryzby <pryzby@telsasoft.com> writes: > > forking: <20220302205058.GJ15744@telsasoft.com>: Re: Adding CI to our tree > > On Wed, Mar 02, 2022 at 02:50:58PM -0600, Justin Pryzby wrote: > >> BTW (regarding the last patch), I just noticed that -Og optimization can cause > >> warnings with gcc-4.8.5-39.el7.x86_64. > > I'm a little dubious about whether -Og is a case we should pay special > attention to? Our standard optimization setting for gcc is -O2, and > once you go away from that there are any number of weird cases that > may or may not produce warnings. I'm not entirely willing to buy > the proposition that we must suppress warnings on > any-random-gcc-version combined with any-random-options. The "Developer FAQ" page on the wiki suggests that when you develop with gcc that you use CFLAGS="-ggdb -Og -g3 -fno-omit-frame-pointer" during development, so I'd hardly call -Og "any random option". -Matthias
Matthias van de Meent <boekewurm+postgres@gmail.com> writes:
> On Thu, 2 Jun 2022, 07:10 Tom Lane, <tgl@sss.pgh.pa.us> wrote:
>> I'm a little dubious about whether -Og is a case we should pay special
>> attention to?
> The "Developer FAQ" page on the wiki suggests that when you develop
> with gcc that you use CFLAGS="-ggdb -Og -g3 -fno-omit-frame-pointer"
> during development, so I'd hardly call -Og "any random option".
I have no idea who wrote that FAQ entry, and I'd certainly not
accept it as being project policy. I'd actually say that's an
excellent example of adding some random compiler options.
regards, tom lane
Hi, On 2022-06-02 13:24:28 +0900, Michael Paquier wrote: > On Wed, Jun 01, 2022 at 09:42:44PM -0500, Justin Pryzby wrote: > > Today's "warnings" thread suggests to me that these are worth fixing - it seems > > reasonable to compile postgres 14 on centos7 (as I sometimes have done), and > > the patch seems even more reasonable when backpatched to older versions. > > (Also, I wonder if there's any consideration to backpatch cirrus.yaml, which > > uses -Og) > > https://en.wikipedia.org/wiki/CentOS#CentOS_releases tells that centos > 7 will be supported until the end of 2024, so I would fix that. To me fixing gcc 4.8 warnings feels like a fools errand, unless they're verbose enough to make compilation exceedingly verbose (e.g. warnings in a header). > > The patch below applies and fixes warnings back to v13. > > I don't mind fixing what you have here, as a first step. All those > cases are telling us that the compiler does not see PG_TRY() as > something is can rely on to set up each variable. 7292fd8 is > complaining about the same point, actually, aka setjmp clobberring the > variable, isn't it? So wouldn't it be better to initialize them, as > your patch does, but also mark them as volatile? In short, what > happens with -Wclobber and a non-optimized compilation? FWIW, I found -Wclobber to be so buggy as to be pointless. I don't think it needs be volatile because it'll not be accessed if we error out? At least in the first instances in Justin's patch. Greetings, Andres Freund
Hi, On 2022-06-02 10:33:52 -0400, Tom Lane wrote: > Matthias van de Meent <boekewurm+postgres@gmail.com> writes: > > On Thu, 2 Jun 2022, 07:10 Tom Lane, <tgl@sss.pgh.pa.us> wrote: > >> I'm a little dubious about whether -Og is a case we should pay special > >> attention to? > > > The "Developer FAQ" page on the wiki suggests that when you develop > > with gcc that you use CFLAGS="-ggdb -Og -g3 -fno-omit-frame-pointer" > > during development, so I'd hardly call -Og "any random option". > > I have no idea who wrote that FAQ entry, and I'd certainly not > accept it as being project policy. I don't know either. However: > I'd actually say that's an excellent example of adding some random compiler > options. To me they mostly make sense. -g3 with -ggdb makes gcc emit enough information about macros that the debugger can interpret them. -fno-omit-frame-pointer makes profiling with call graphs much much smaller. I tried to use -Og many times, but in the end mostly gave up, because it still makes debugging harder compared to -O0. Greetings, Andres Freund
Hi, On 2022-06-02 01:09:58 -0400, Tom Lane wrote: > Justin Pryzby <pryzby@telsasoft.com> writes: > > forking: <20220302205058.GJ15744@telsasoft.com>: Re: Adding CI to our tree > > On Wed, Mar 02, 2022 at 02:50:58PM -0600, Justin Pryzby wrote: > >> BTW (regarding the last patch), I just noticed that -Og optimization can cause > >> warnings with gcc-4.8.5-39.el7.x86_64. > > I'm a little dubious about whether -Og is a case we should pay special > attention to? Our standard optimization setting for gcc is -O2, and > once you go away from that there are any number of weird cases that > may or may not produce warnings. I'm not entirely willing to buy > the proposition that we must suppress warnings on > any-random-gcc-version combined with any-random-options. I think it'd be useful to have -Og in a usable state, despite my nearby griping about it. It makes our tests use noticably fewer CPU cycles, and debugging is less annoying than with -O2. It's also faster to compile. However, making that effort for compiler versions for a compiler that went out of support in 2015 doesn't seem useful. It may be useful to pay some attention to not producint too many warnings on LTS distribution compilers when compiling with production oriented flags, but nobody should develop on them. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> I tried to use -Og many times, but in the end mostly gave up, because it still
> makes debugging harder compared to -O0.
Yeah. My own habit is to build with -O2 normally. If I'm trying to
debug some bit of code and find that I can't follow things adequately
in gdb, I recompile just the relevant file(s) with -O0.
regards, tom lane