pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedFix last unitialized memory warning
17+ messages / 6 participants
[nested] [flat]
Commits from this thread:
2023-08-10 5e0c761d0a Fix erroneous -Werror=missing-braces on old GCC
2023-08-09 4a8fef0d73 Fix last remaining uninitialized memory warnings
* Fix last unitialized memory warning
@ 2023-06-07 14:31 Tristan Partin <tristan@neon.tech>
0 siblings, 2 replies; 17+ messages in thread
From: Tristan Partin @ 2023-06-07 14:31 UTC (permalink / raw)
To: pgsql-hackers
This patch is really not necessary from a functional point of view. It
is only necessary if we want to silence a compiler warning.
Tested on `gcc (GCC) 13.1.1 20230511 (Red Hat 13.1.1-2)`.
After silencing this warning, all I am left with (given my build
configuration) is:
[1667/2280] 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:8349: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:8537:17: note: in expansion of macro ‘VARATT_IS_EXTERNAL_NON_EXPANDED’
8537 | VARATT_IS_EXTERNAL_NON_EXPANDED(DatumGetPointer(newvalue)))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
From my perspective, this warning definitely seems like a false
positive, but I don't know the code well-enough to say that for certain.
--
Tristan Partin
Neon (https://neon.tech)
Attachments:
v1-0001-Fix-last-remaining-uninitialized-memory-warnings.patch (text/x-patch, 882B) download
From 0daab1d0231668da4ac701bd240fc1da5fbcd5bb Mon Sep 17 00:00:00 2001
From: Tristan Partin <tristan@neon.tech>
Date: Wed, 7 Jun 2023 09:21:59 -0500
Subject: [PATCH v1] Fix last remaining uninitialized memory warnings
gcc fails to properly analyze the code due to the loop stop condition
including `l != NULL`. Let's just help it out.
---
src/bin/pgbench/pgbench.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 1d1670d4c2..536f1721ff 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -2247,7 +2247,7 @@ evalStandardFunc(CState *st,
{
/* evaluate all function arguments */
int nargs = 0;
- PgBenchValue vargs[MAX_FARGS];
+ PgBenchValue vargs[MAX_FARGS] = { 0 };
PgBenchExprLink *l = args;
bool has_null = false;
--
--
Tristan Partin
Neon (https://neon.tech)
* Re: Fix last unitialized memory warning
@ 2023-06-09 03:16 Gurjeet Singh <gurjeet@singh.im>
parent: Tristan Partin <tristan@neon.tech>
1 sibling, 0 replies; 17+ messages in thread
From: Gurjeet Singh @ 2023-06-09 03:16 UTC (permalink / raw)
To: Tristan Partin <tristan@neon.tech>; +Cc: pgsql-hackers
On Wed, Jun 7, 2023 at 7:31 AM Tristan Partin <tristan@neon.tech> wrote:
>
> This patch is really not necessary from a functional point of view. It
> is only necessary if we want to silence a compiler warning.
>
> Tested on `gcc (GCC) 13.1.1 20230511 (Red Hat 13.1.1-2)`.
...
> From my perspective, this warning definitely seems like a false
> positive, but I don't know the code well-enough to say that for certain.
It was a bit confusing to see a patch for src/bin/pgbench/pgbench.c,
but that file not mentioned in the warning messages you quoted. I take
it that your patch silences a warning in pgbench.c; it would've been
nice to see the actual warning.
- PgBenchValue vargs[MAX_FARGS];
+ PgBenchValue vargs[MAX_FARGS] = { 0 };
If I'm right about what kind of warning this might've caused (use of
possibly uninitialized variable), you're correct that it is benign.
The for loop after declarations initializes all the elements of this
array using evaluateExpr(), and if the initialization fails for some
reason, the loop ends prematurely and returns from the function.
I analyzed a few code-paths that return true from evaluateExpr(), and
I'd like to believe that _all_ code paths that return true also
initialize the array element passed. But because there are so many
branches and function calls beneath evaluateExpr(), I think it's
better to be paranoid and initialize all the array elements to 0.
Also, it is better to initialize/clear an array at the point of
definition, like your patch does. So, +1 to the patch.
Best regards,
Gurjeet
http://Gurje.et
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Fix last unitialized memory warning
@ 2023-07-03 06:19 Peter Eisentraut <peter@eisentraut.org>
parent: Tristan Partin <tristan@neon.tech>
1 sibling, 1 reply; 17+ messages in thread
From: Peter Eisentraut @ 2023-07-03 06:19 UTC (permalink / raw)
To: Tristan Partin <tristan@neon.tech>; pgsql-hackers
On 07.06.23 16:31, Tristan Partin wrote:
> This patch is really not necessary from a functional point of view. It
> is only necessary if we want to silence a compiler warning.
>
> Tested on `gcc (GCC) 13.1.1 20230511 (Red Hat 13.1.1-2)`.
>
> After silencing this warning, all I am left with (given my build
> configuration) is:
>
> [1667/2280] 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:8349: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:8537:17: note: in expansion of macro ‘VARATT_IS_EXTERNAL_NON_EXPANDED’
> 8537 | VARATT_IS_EXTERNAL_NON_EXPANDED(DatumGetPointer(newvalue)))
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> From my perspective, this warning definitely seems like a false
> positive, but I don't know the code well-enough to say that for certain.
I cannot reproduce this warning with gcc-13. Are you using any
non-standard optimization options. Could you give your full configure
and build commands and the OS?
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Fix last unitialized memory warning
@ 2023-07-05 21:06 Tristan Partin <tristan@neon.tech>
parent: Peter Eisentraut <peter@eisentraut.org>
0 siblings, 1 reply; 17+ messages in thread
From: Tristan Partin @ 2023-07-05 21:06 UTC (permalink / raw)
To: Peter Eisentraut <peter@eisentraut.org>; pgsql-hackers
On Mon Jul 3, 2023 at 1:19 AM CDT, Peter Eisentraut wrote:
> On 07.06.23 16:31, Tristan Partin wrote:
> > This patch is really not necessary from a functional point of view. It
> > is only necessary if we want to silence a compiler warning.
> >
> > Tested on `gcc (GCC) 13.1.1 20230511 (Red Hat 13.1.1-2)`.
> >
> > After silencing this warning, all I am left with (given my build
> > configuration) is:
> >
> > [1667/2280] 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:8349: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:8537:17: note: in expansion of macro ‘VARATT_IS_EXTERNAL_NON_EXPANDED’
> > 8537 | VARATT_IS_EXTERNAL_NON_EXPANDED(DatumGetPointer(newvalue)))
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > From my perspective, this warning definitely seems like a false
> > positive, but I don't know the code well-enough to say that for certain.
>
> I cannot reproduce this warning with gcc-13. Are you using any
> non-standard optimization options. Could you give your full configure
> and build commands and the OS?
Thanks for following up. My system is Fedora 38. I can confirm this is
still happening on master.
$ gcc --version
gcc (GCC) 13.1.1 20230614 (Red Hat 13.1.1-4)
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$ meson setup build --buildtype=release
$ ninja -C build
--
Tristan Partin
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Fix last unitialized memory warning
@ 2023-07-06 08:21 Peter Eisentraut <peter@eisentraut.org>
parent: Tristan Partin <tristan@neon.tech>
0 siblings, 2 replies; 17+ messages in thread
From: Peter Eisentraut @ 2023-07-06 08:21 UTC (permalink / raw)
To: Tristan Partin <tristan@neon.tech>; pgsql-hackers
On 05.07.23 23:06, Tristan Partin wrote:
> Thanks for following up. My system is Fedora 38. I can confirm this is
> still happening on master.
>
> $ gcc --version
> gcc (GCC) 13.1.1 20230614 (Red Hat 13.1.1-4)
> Copyright (C) 2023 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions. There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> $ meson setup build --buildtype=release
This buildtype turns on -O3 warnings. We have usually opted against
chasing warnings in -O3 level because there are often some
false-positive uninitialized variable warnings with every new compiler.
Note that we have set the default build type to debugoptimized, for that
reason.
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Fix last unitialized memory warning
@ 2023-07-06 13:41 Tristan Partin <tristan@neon.tech>
parent: Peter Eisentraut <peter@eisentraut.org>
1 sibling, 1 reply; 17+ messages in thread
From: Tristan Partin @ 2023-07-06 13:41 UTC (permalink / raw)
To: Peter Eisentraut <peter@eisentraut.org>; pgsql-hackers
On Thu Jul 6, 2023 at 3:21 AM CDT, Peter Eisentraut wrote:
> On 05.07.23 23:06, Tristan Partin wrote:
> > Thanks for following up. My system is Fedora 38. I can confirm this is
> > still happening on master.
> >
> > $ gcc --version
> > gcc (GCC) 13.1.1 20230614 (Red Hat 13.1.1-4)
> > Copyright (C) 2023 Free Software Foundation, Inc.
> > This is free software; see the source for copying conditions. There is NO
> > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> > $ meson setup build --buildtype=release
>
> This buildtype turns on -O3 warnings. We have usually opted against
> chasing warnings in -O3 level because there are often some
> false-positive uninitialized variable warnings with every new compiler.
>
> Note that we have set the default build type to debugoptimized, for that
> reason.
Good to know, thanks.
Regarding the original patch, do you think it is good to be applied?
--
Tristan Partin
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Fix last unitialized memory warning
@ 2023-07-06 19:15 Andres Freund <andres@anarazel.de>
parent: Peter Eisentraut <peter@eisentraut.org>
1 sibling, 0 replies; 17+ messages in thread
From: Andres Freund @ 2023-07-06 19:15 UTC (permalink / raw)
To: Peter Eisentraut <peter@eisentraut.org>; +Cc: Tristan Partin <tristan@neon.tech>; pgsql-hackers
Hi,
On 2023-07-06 10:21:44 +0200, Peter Eisentraut wrote:
> On 05.07.23 23:06, Tristan Partin wrote:
> > Thanks for following up. My system is Fedora 38. I can confirm this is
> > still happening on master.
> >
> > $ gcc --version
> > gcc (GCC) 13.1.1 20230614 (Red Hat 13.1.1-4)
> > Copyright (C) 2023 Free Software Foundation, Inc.
> > This is free software; see the source for copying conditions. There is NO
> > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> > $ meson setup build --buildtype=release
>
> This buildtype turns on -O3 warnings. We have usually opted against chasing
> warnings in -O3 level because there are often some false-positive
> uninitialized variable warnings with every new compiler.
OTOH, -O3 is substantially faster IME in cpu bound tests than -O2. It doesn't
seem wise to me for the project to basically say that that's not advisable due
to the level of warnings created.
I've also found bugs with -O3 that -O2 didn't find. And often -O3 warnings end
up showing up with -O2 a compiler major version or three down the line, so
it's often just deferring work.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Fix last unitialized memory warning
@ 2023-07-09 07:23 Peter Eisentraut <peter@eisentraut.org>
parent: Tristan Partin <tristan@neon.tech>
0 siblings, 1 reply; 17+ messages in thread
From: Peter Eisentraut @ 2023-07-09 07:23 UTC (permalink / raw)
To: Tristan Partin <tristan@neon.tech>; pgsql-hackers
On 06.07.23 15:41, Tristan Partin wrote:
> On Thu Jul 6, 2023 at 3:21 AM CDT, Peter Eisentraut wrote:
>> On 05.07.23 23:06, Tristan Partin wrote:
>>> Thanks for following up. My system is Fedora 38. I can confirm this is
>>> still happening on master.
>>>
>>> $ gcc --version
>>> gcc (GCC) 13.1.1 20230614 (Red Hat 13.1.1-4)
>>> Copyright (C) 2023 Free Software Foundation, Inc.
>>> This is free software; see the source for copying conditions. There is NO
>>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>>> $ meson setup build --buildtype=release
>>
>> This buildtype turns on -O3 warnings. We have usually opted against
>> chasing warnings in -O3 level because there are often some
>> false-positive uninitialized variable warnings with every new compiler.
>>
>> Note that we have set the default build type to debugoptimized, for that
>> reason.
>
> Good to know, thanks.
>
> Regarding the original patch, do you think it is good to be applied?
That patch looks reasonable. But I can't actually reproduce the
warning, even with gcc-13. I do get the warning from plpgsql. Can you
show the warning you are seeing?
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Fix last unitialized memory warning
@ 2023-07-19 17:15 Tristan Partin <tristan@neon.tech>
parent: Peter Eisentraut <peter@eisentraut.org>
0 siblings, 1 reply; 17+ messages in thread
From: Tristan Partin @ 2023-07-19 17:15 UTC (permalink / raw)
To: Peter Eisentraut <peter@eisentraut.org>; pgsql-hackers
On Sun Jul 9, 2023 at 2:23 AM CDT, Peter Eisentraut wrote:
> On 06.07.23 15:41, Tristan Partin wrote:
> > On Thu Jul 6, 2023 at 3:21 AM CDT, Peter Eisentraut wrote:
> >> On 05.07.23 23:06, Tristan Partin wrote:
> >>> Thanks for following up. My system is Fedora 38. I can confirm this is
> >>> still happening on master.
> >>>
> >>> $ gcc --version
> >>> gcc (GCC) 13.1.1 20230614 (Red Hat 13.1.1-4)
> >>> Copyright (C) 2023 Free Software Foundation, Inc.
> >>> This is free software; see the source for copying conditions. There is NO
> >>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> >>> $ meson setup build --buildtype=release
> >>
> >> This buildtype turns on -O3 warnings. We have usually opted against
> >> chasing warnings in -O3 level because there are often some
> >> false-positive uninitialized variable warnings with every new compiler.
> >>
> >> Note that we have set the default build type to debugoptimized, for that
> >> reason.
> >
> > Good to know, thanks.
> >
> > Regarding the original patch, do you think it is good to be applied?
>
> That patch looks reasonable. But I can't actually reproduce the
> warning, even with gcc-13. I do get the warning from plpgsql. Can you
> show the warning you are seeing?
Here is the full warning that the original patch suppresses.
[1360/1876] Compiling C object src/bin/pgbench/pgbench.p/pgbench.c.o
In function ‘coerceToInt’,
inlined from ‘evalStandardFunc’ at ../src/bin/pgbench/pgbench.c:2607:11:
../src/bin/pgbench/pgbench.c:2032:17: warning: ‘vargs[0].type’ may be used uninitialized [-Wmaybe-uninitialized]
2032 | if (pval->type == PGBT_INT)
| ~~~~^~~~~~
../src/bin/pgbench/pgbench.c: In function ‘evalStandardFunc’:
../src/bin/pgbench/pgbench.c:2240:22: note: ‘vargs’ declared here
2240 | PgBenchValue vargs[MAX_FARGS];
| ^~~~~
In function ‘coerceToInt’,
inlined from ‘evalStandardFunc’ at ../src/bin/pgbench/pgbench.c:2607:11:
../src/bin/pgbench/pgbench.c:2034:32: warning: ‘vargs[0].u.ival’ may be used uninitialized [-Wmaybe-uninitialized]
2034 | *ival = pval->u.ival;
| ~~~~~~~^~~~~
../src/bin/pgbench/pgbench.c: In function ‘evalStandardFunc’:
../src/bin/pgbench/pgbench.c:2240:22: note: ‘vargs’ declared here
2240 | PgBenchValue vargs[MAX_FARGS];
| ^~~~~
In function ‘coerceToInt’,
inlined from ‘evalStandardFunc’ at ../src/bin/pgbench/pgbench.c:2607:11:
../src/bin/pgbench/pgbench.c:2039:40: warning: ‘vargs[0].u.dval’ may be used uninitialized [-Wmaybe-uninitialized]
2039 | double dval = rint(pval->u.dval);
| ^~~~~~~~~~~~~~~~~~
../src/bin/pgbench/pgbench.c: In function ‘evalStandardFunc’:
../src/bin/pgbench/pgbench.c:2240:22: note: ‘vargs’ declared here
2240 | PgBenchValue vargs[MAX_FARGS];
| ^~~~~
--
Tristan Partin
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Fix last unitialized memory warning
@ 2023-08-08 10:20 Peter Eisentraut <peter@eisentraut.org>
parent: Tristan Partin <tristan@neon.tech>
0 siblings, 1 reply; 17+ messages in thread
From: Peter Eisentraut @ 2023-08-08 10:20 UTC (permalink / raw)
To: Tristan Partin <tristan@neon.tech>; pgsql-hackers
On 19.07.23 19:15, Tristan Partin wrote:
> On Sun Jul 9, 2023 at 2:23 AM CDT, Peter Eisentraut wrote:
>> On 06.07.23 15:41, Tristan Partin wrote:
>> > On Thu Jul 6, 2023 at 3:21 AM CDT, Peter Eisentraut wrote:
>> >> On 05.07.23 23:06, Tristan Partin wrote:
>> >>> Thanks for following up. My system is Fedora 38. I can confirm
>> this is
>> >>> still happening on master.
>> >>>
>> >>> $ gcc --version
>> >>> gcc (GCC) 13.1.1 20230614 (Red Hat 13.1.1-4)
>> >>> Copyright (C) 2023 Free Software Foundation, Inc.
>> >>> This is free software; see the source for copying conditions.
>> There is NO
>> >>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR
>> PURPOSE.
>> >>> $ meson setup build --buildtype=release
>> >>
>> >> This buildtype turns on -O3 warnings. We have usually opted against
>> >> chasing warnings in -O3 level because there are often some
>> >> false-positive uninitialized variable warnings with every new
>> compiler.
>> >>
>> >> Note that we have set the default build type to debugoptimized, for
>> that
>> >> reason.
>> > > Good to know, thanks.
>> > > Regarding the original patch, do you think it is good to be applied?
>>
>> That patch looks reasonable. But I can't actually reproduce the
>> warning, even with gcc-13. I do get the warning from plpgsql. Can
>> you show the warning you are seeing?
>
> Here is the full warning that the original patch suppresses.
I was able to reproduce the warning now on Fedora. I agree with the patch
- PgBenchValue vargs[MAX_FARGS];
+ PgBenchValue vargs[MAX_FARGS] = { 0 };
I suggest to also do
typedef enum
{
- PGBT_NO_VALUE,
+ PGBT_NO_VALUE = 0,
to make clear that the initialization value is meant to be invalid.
I also got the plpgsql warning that you showed earlier, but I couldn't
think of a reasonable way to fix that.
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Fix last unitialized memory warning
@ 2023-08-08 15:14 Tristan Partin <tristan@neon.tech>
parent: Peter Eisentraut <peter@eisentraut.org>
0 siblings, 1 reply; 17+ messages in thread
From: Tristan Partin @ 2023-08-08 15:14 UTC (permalink / raw)
To: Peter Eisentraut <peter@eisentraut.org>; pgsql-hackers
On Tue Aug 8, 2023 at 5:20 AM CDT, Peter Eisentraut wrote:
> On 19.07.23 19:15, Tristan Partin wrote:
> > On Sun Jul 9, 2023 at 2:23 AM CDT, Peter Eisentraut wrote:
> >> On 06.07.23 15:41, Tristan Partin wrote:
> >> > On Thu Jul 6, 2023 at 3:21 AM CDT, Peter Eisentraut wrote:
> >> >> On 05.07.23 23:06, Tristan Partin wrote:
> >> >>> Thanks for following up. My system is Fedora 38. I can confirm
> >> this is
> >> >>> still happening on master.
> >> >>>
> >> >>> $ gcc --version
> >> >>> gcc (GCC) 13.1.1 20230614 (Red Hat 13.1.1-4)
> >> >>> Copyright (C) 2023 Free Software Foundation, Inc.
> >> >>> This is free software; see the source for copying conditions.
> >> There is NO
> >> >>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR
> >> PURPOSE.
> >> >>> $ meson setup build --buildtype=release
> >> >>
> >> >> This buildtype turns on -O3 warnings. We have usually opted against
> >> >> chasing warnings in -O3 level because there are often some
> >> >> false-positive uninitialized variable warnings with every new
> >> compiler.
> >> >>
> >> >> Note that we have set the default build type to debugoptimized, for
> >> that
> >> >> reason.
> >> > > Good to know, thanks.
> >> > > Regarding the original patch, do you think it is good to be applied?
> >>
> >> That patch looks reasonable. But I can't actually reproduce the
> >> warning, even with gcc-13. I do get the warning from plpgsql. Can
> >> you show the warning you are seeing?
> >
> > Here is the full warning that the original patch suppresses.
>
> I was able to reproduce the warning now on Fedora. I agree with the patch
>
> - PgBenchValue vargs[MAX_FARGS];
> + PgBenchValue vargs[MAX_FARGS] = { 0 };
>
> I suggest to also do
>
> typedef enum
> {
> - PGBT_NO_VALUE,
> + PGBT_NO_VALUE = 0,
>
> to make clear that the initialization value is meant to be invalid.
>
> I also got the plpgsql warning that you showed earlier, but I couldn't
> think of a reasonable way to fix that.
Applied in v2.
--
Tristan Partin
Neon (https://neon.tech)
Attachments:
v2-0001-Fix-last-remaining-uninitialized-memory-warnings.patch (text/x-patch, 1.2K) download
From 7e28d060871b19a8c09539e2da5e226b780431b9 Mon Sep 17 00:00:00 2001
From: Tristan Partin <tristan@neon.tech>
Date: Wed, 7 Jun 2023 09:21:59 -0500
Subject: [PATCH v2] Fix last remaining uninitialized memory warnings
gcc fails to properly analyze the code due to the loop stop condition
including `l != NULL`. Let's just help it out.
---
src/bin/pgbench/pgbench.c | 2 +-
src/bin/pgbench/pgbench.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 539c2795e2..2ba3e367c4 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -2239,7 +2239,7 @@ evalStandardFunc(CState *st,
{
/* evaluate all function arguments */
int nargs = 0;
- PgBenchValue vargs[MAX_FARGS];
+ PgBenchValue vargs[MAX_FARGS] = { 0 };
PgBenchExprLink *l = args;
bool has_null = false;
diff --git a/src/bin/pgbench/pgbench.h b/src/bin/pgbench/pgbench.h
index 957c9ca9da..f8efa4b868 100644
--- a/src/bin/pgbench/pgbench.h
+++ b/src/bin/pgbench/pgbench.h
@@ -33,7 +33,7 @@ union YYSTYPE;
*/
typedef enum
{
- PGBT_NO_VALUE,
+ PGBT_NO_VALUE = 0,
PGBT_NULL,
PGBT_INT,
PGBT_DOUBLE,
--
Tristan Partin
Neon (https://neon.tech)
* Re: Fix last unitialized memory warning
@ 2023-08-09 08:07 Peter Eisentraut <peter@eisentraut.org>
parent: Tristan Partin <tristan@neon.tech>
0 siblings, 1 reply; 17+ messages in thread
From: Peter Eisentraut @ 2023-08-09 08:07 UTC (permalink / raw)
To: Tristan Partin <tristan@neon.tech>; pgsql-hackers
On 08.08.23 17:14, Tristan Partin wrote:
>> I was able to reproduce the warning now on Fedora. I agree with the
>> patch
>>
>> - PgBenchValue vargs[MAX_FARGS];
>> + PgBenchValue vargs[MAX_FARGS] = { 0 };
>>
>> I suggest to also do
>>
>> typedef enum
>> {
>> - PGBT_NO_VALUE,
>> + PGBT_NO_VALUE = 0,
>>
>> to make clear that the initialization value is meant to be invalid.
>>
>> I also got the plpgsql warning that you showed earlier, but I couldn't
>> think of a reasonable way to fix that.
>
> Applied in v2.
committed
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Fix last unitialized memory warning
@ 2023-08-09 15:02 Peter Eisentraut <peter@eisentraut.org>
parent: Peter Eisentraut <peter@eisentraut.org>
0 siblings, 1 reply; 17+ messages in thread
From: Peter Eisentraut @ 2023-08-09 15:02 UTC (permalink / raw)
To: Tristan Partin <tristan@neon.tech>; pgsql-hackers
On 09.08.23 10:07, Peter Eisentraut wrote:
> On 08.08.23 17:14, Tristan Partin wrote:
>>> I was able to reproduce the warning now on Fedora. I agree with the
>>> patch
>>>
>>> - PgBenchValue vargs[MAX_FARGS];
>>> + PgBenchValue vargs[MAX_FARGS] = { 0 };
>>>
>>> I suggest to also do
>>>
>>> typedef enum
>>> {
>>> - PGBT_NO_VALUE,
>>> + PGBT_NO_VALUE = 0,
>>>
>>> to make clear that the initialization value is meant to be invalid.
>>>
>>> I also got the plpgsql warning that you showed earlier, but I
>>> couldn't think of a reasonable way to fix that.
>>
>> Applied in v2.
>
> committed
This patch has apparently upset one buildfarm member with a very old
compiler:
https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=lapwing&br=HEAD
Any thoughts?
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Fix last unitialized memory warning
@ 2023-08-09 15:29 Tristan Partin <tristan@neon.tech>
parent: Peter Eisentraut <peter@eisentraut.org>
0 siblings, 2 replies; 17+ messages in thread
From: Tristan Partin @ 2023-08-09 15:29 UTC (permalink / raw)
To: Peter Eisentraut <peter@eisentraut.org>; +Cc: pgsql-hackers
On Wed Aug 9, 2023 at 10:02 AM CDT, Peter Eisentraut wrote:
> On 09.08.23 10:07, Peter Eisentraut wrote:
> > On 08.08.23 17:14, Tristan Partin wrote:
> >>> I was able to reproduce the warning now on Fedora. I agree with the
> >>> patch
> >>>
> >>> - PgBenchValue vargs[MAX_FARGS];
> >>> + PgBenchValue vargs[MAX_FARGS] = { 0 };
> >>>
> >>> I suggest to also do
> >>>
> >>> typedef enum
> >>> {
> >>> - PGBT_NO_VALUE,
> >>> + PGBT_NO_VALUE = 0,
> >>>
> >>> to make clear that the initialization value is meant to be invalid.
> >>>
> >>> I also got the plpgsql warning that you showed earlier, but I
> >>> couldn't think of a reasonable way to fix that.
> >>
> >> Applied in v2.
> >
> > committed
>
> This patch has apparently upset one buildfarm member with a very old
> compiler:
> https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=lapwing&br=HEAD
>
> Any thoughts?
Best I could find is SO question[0] which links out to[1]. Try this
patch. Otherwise, a memset() would probably do too.
[0]: https://stackoverflow.com/questions/13746033/how-to-repair-warning-missing-braces-around-initializer
[1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
--
Tristan Partin
Neon (https://neon.tech)
Attachments:
v1-0001-Fix-erroneous-Werror-missing-braces-on-old-GCC.patch (text/x-patch, 1.2K) download
From ecb342a88e5fb2a88f0086dcf46fdcadb6d930ba Mon Sep 17 00:00:00 2001
From: Tristan Partin <tristan@neon.tech>
Date: Wed, 9 Aug 2023 10:12:47 -0500
Subject: [PATCH v1] Fix erroneous -Werror=missing-braces on old GCC
The buildfarm reports that this is an error on gcc (Debian 4.7.2-5)
4.7.2, 32-bit. The bug seems to be GCC bug 53119, which has obviously
been fixed for years.
---
src/bin/pgbench/pgbench.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 2ba3e367c4..d2b7fc87e4 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -2239,10 +2239,15 @@ evalStandardFunc(CState *st,
{
/* evaluate all function arguments */
int nargs = 0;
- PgBenchValue vargs[MAX_FARGS] = { 0 };
PgBenchExprLink *l = args;
bool has_null = false;
+ /*
+ * This value is double braced to workaround GCC bug 53119, which seems to
+ * exist at least on gcc (Debian 4.7.2-5) 4.7.2, 32-bit.
+ */
+ PgBenchValue vargs[MAX_FARGS] = { { 0 } };
+
for (nargs = 0; nargs < MAX_FARGS && l != NULL; nargs++, l = l->next)
{
if (!evaluateExpr(st, l->expr, &vargs[nargs]))
--
Tristan Partin
Neon (https://neon.tech)
* Re: Fix last unitialized memory warning
@ 2023-08-10 00:56 Julien Rouhaud <rjuju123@gmail.com>
parent: Tristan Partin <tristan@neon.tech>
1 sibling, 1 reply; 17+ messages in thread
From: Julien Rouhaud @ 2023-08-10 00:56 UTC (permalink / raw)
To: Tristan Partin <tristan@neon.tech>; +Cc: Peter Eisentraut <peter@eisentraut.org>; pgsql-hackers
On Wed, Aug 09, 2023 at 10:29:56AM -0500, Tristan Partin wrote:
> On Wed Aug 9, 2023 at 10:02 AM CDT, Peter Eisentraut wrote:
> >
> > This patch has apparently upset one buildfarm member with a very old
> > compiler:
> > https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=lapwing&br=HEAD
> >
> > Any thoughts?
>
> Best I could find is SO question[0] which links out to[1]. Try this patch.
> Otherwise, a memset() would probably do too.
Yes, it's a buggy warning that came up in the past a few times as I recall, for
which we previously used the {{...}} approach to silence it.
As there have been previous complaints about it, I removed the -Werror from
lapwing and forced a new run to make it green again.
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Fix last unitialized memory warning
@ 2023-08-10 08:06 Richard Guo <guofenglinux@gmail.com>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 0 replies; 17+ messages in thread
From: Richard Guo @ 2023-08-10 08:06 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: Tristan Partin <tristan@neon.tech>; Peter Eisentraut <peter@eisentraut.org>; pgsql-hackers
On Thu, Aug 10, 2023 at 8:57 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> On Wed, Aug 09, 2023 at 10:29:56AM -0500, Tristan Partin wrote:
> > On Wed Aug 9, 2023 at 10:02 AM CDT, Peter Eisentraut wrote:
> > >
> > > This patch has apparently upset one buildfarm member with a very old
> > > compiler:
> > >
> https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=lapwing&br=HEAD
> > >
> > > Any thoughts?
> >
> > Best I could find is SO question[0] which links out to[1]. Try this
> patch.
> > Otherwise, a memset() would probably do too.
>
> Yes, it's a buggy warning that came up in the past a few times as I
> recall, for
> which we previously used the {{...}} approach to silence it.
I came across this warning too on one of my VMs, with gcc 4.8.5. +1 to
silence it with {{...}}. We did that in d937904 and 6392f2a (and maybe
more).
In case it helps, here is the GCC I'm on.
$ gcc --version
gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44)
Thanks
Richard
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Fix last unitialized memory warning
@ 2023-08-10 14:59 Peter Eisentraut <peter@eisentraut.org>
parent: Tristan Partin <tristan@neon.tech>
1 sibling, 0 replies; 17+ messages in thread
From: Peter Eisentraut @ 2023-08-10 14:59 UTC (permalink / raw)
To: Tristan Partin <tristan@neon.tech>; +Cc: pgsql-hackers
On 09.08.23 17:29, Tristan Partin wrote:
> On Wed Aug 9, 2023 at 10:02 AM CDT, Peter Eisentraut wrote:
>> On 09.08.23 10:07, Peter Eisentraut wrote:
>> > On 08.08.23 17:14, Tristan Partin wrote:
>> >>> I was able to reproduce the warning now on Fedora. I agree with
>> the >>> patch
>> >>>
>> >>> - PgBenchValue vargs[MAX_FARGS];
>> >>> + PgBenchValue vargs[MAX_FARGS] = { 0 };
>> >>>
>> >>> I suggest to also do
>> >>>
>> >>> typedef enum
>> >>> {
>> >>> - PGBT_NO_VALUE,
>> >>> + PGBT_NO_VALUE = 0,
>> >>>
>> >>> to make clear that the initialization value is meant to be invalid.
>> >>>
>> >>> I also got the plpgsql warning that you showed earlier, but I >>>
>> couldn't think of a reasonable way to fix that.
>> >>
>> >> Applied in v2.
>> > > committed
>>
>> This patch has apparently upset one buildfarm member with a very old
>> compiler:
>> https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=lapwing&br=HEAD
>>
>> Any thoughts?
>
> Best I could find is SO question[0] which links out to[1]. Try this
> patch.
committed this fix
^ permalink raw reply [nested|flat] 17+ messages in thread
end of thread, other threads:[~2023-08-10 14:59 UTC | newest]
Thread overview: 17+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-06-07 14:31 Fix last unitialized memory warning Tristan Partin <tristan@neon.tech>
2023-06-09 03:16 ` Gurjeet Singh <gurjeet@singh.im>
2023-07-03 06:19 ` Peter Eisentraut <peter@eisentraut.org>
2023-07-05 21:06 ` Tristan Partin <tristan@neon.tech>
2023-07-06 08:21 ` Peter Eisentraut <peter@eisentraut.org>
2023-07-06 13:41 ` Tristan Partin <tristan@neon.tech>
2023-07-09 07:23 ` Peter Eisentraut <peter@eisentraut.org>
2023-07-19 17:15 ` Tristan Partin <tristan@neon.tech>
2023-08-08 10:20 ` Peter Eisentraut <peter@eisentraut.org>
2023-08-08 15:14 ` Tristan Partin <tristan@neon.tech>
2023-08-09 08:07 ` Peter Eisentraut <peter@eisentraut.org>
2023-08-09 15:02 ` Peter Eisentraut <peter@eisentraut.org>
2023-08-09 15:29 ` Tristan Partin <tristan@neon.tech>
2023-08-10 00:56 ` Julien Rouhaud <rjuju123@gmail.com>
2023-08-10 08:06 ` Richard Guo <guofenglinux@gmail.com>
2023-08-10 14:59 ` Peter Eisentraut <peter@eisentraut.org>
2023-07-06 19:15 ` Andres Freund <andres@anarazel.de>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox