From: | Kevin Grittner <kgrittn(at)ymail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | "pgsql-committers(at)postgresql(dot)org" <pgsql-committers(at)postgresql(dot)org> |
Subject: | Re: pgsql: Fix blatantly broken record_image_cmp() logic for pass-by-value |
Date: | 2013-11-08 18:19:53 |
Message-ID: | 1383934793.44107.YahooMailNeo@web162904.mail.bf1.yahoo.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-committers pgsql-hackers |
Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
>> I don't get a warning on this with either of these compilers,
>> either with or without asserts enabled:
>
> Perhaps you built with -O0? At least in older versions of gcc, you need
> at least -O1 to get uninitialized-variable warnings. (I've heard some
> claims that the latest versions of gcc don't require that anymore.)
> I don't recommend -O0 as your default optimization level, precisely
> because of this.
This is with default configure and compile options, which results
in this:
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -I../../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o rowtypes.o rowtypes.c -MMD -MP -MF .deps/rowtypes.Po
> if (GET_4_BYTES(values1[i1]) !=
> GET_4_BYTES(values2[i2]))
> {
> cmpresult = (GET_4_BYTES(values1[i1]) <
> GET_4_BYTES(values2[i2])) ? -1 : 1;
> }
>
> if the two values are in fact equal then this falls through leaving
> cmpresult uninitialized, rather than setting it to zero as it should be;
> which is the case my patch was meant to correct.
I see it now. It is pretty disturbing that the compilers in my
Linux distro don't recognize that as leaving the value
uninitialized.
> This perhaps escaped testing because we aren't using
> record_image_cmp in a way that exposes false nonequality results.
... or the usage pattern happens to leave zero on that location in
the stack when it has mattered.
> I'm not particularly excited about the possibility that attlen might
> not be either 1,2,4, or 8; I'm pretty sure there are lots of other
> places that would go belly-up with such a problem. However, if that
> bothers you the fix is to replace the Assert with an elog(ERROR),
> not to remove the initialization.
Well, I was suggesting replacing the Assert with elog(ERROR); but I
was changing the variable declaration back to uninitialized in what
I was asking people to try so I could find out whether that was the
path causing the uninitialized variable report, to make sure I
understood the report. Nobody who was seeing the warning had
passed along enough information to make that clear.
Now that I understand the problem, I'm fine with the state of
things as currently committed.
Thanks for following up.
-Kevin
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2013-11-08 18:43:14 | Re: pgsql: Fix blatantly broken record_image_cmp() logic for pass-by-value |
Previous Message | Robert Haas | 2013-11-08 17:35:47 | pgsql: Add the notion of REPLICA IDENTITY for a table. |
From | Date | Subject | |
---|---|---|---|
Next Message | Joshua D. Drake | 2013-11-08 18:33:15 | Re: backup.sgml-cmd-v003.patch |
Previous Message | Tom Lane | 2013-11-08 17:58:05 | Re: Fix picksplit with nan values |