pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedRecent failures on buildfarm member hornet
12+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2020-10-07 8d2a01ae12 dc14aa038e 8b231d9753 e3868c7d59 9f3d3fb870 eb48619d01 abfce4e468 Fix optimization hazard in gram.y's makeOrderedSetArgs(), redux.
* Recent failures on buildfarm member hornet
@ 2020-10-06 00:42 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 12+ messages in thread
From: Tom Lane @ 2020-10-06 00:42 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: pgsql-hackers@lists.postgresql.org, David Rowley <dgrowleyml@gmail.com>
hornet has failed its last five runs with
2020-10-05 22:45:42.784 UTC [34734498:40] pg_regress/create_aggregate LOG: statement: create aggregate my_percentile_disc(float8 ORDER BY anyelement) (
stype = internal,
sfunc = ordered_set_transition,
finalfunc = percentile_disc_final,
finalfunc_extra = true,
finalfunc_modify = read_write
);
TRAP: FailedAssertion("variadicArgType != InvalidOid", File: "pg_aggregate.c", Line: 216, PID: 34734498)
After looking at the commits immediately preceding the first failure, and
digging around in the aggregate-related code, it seems like commit
cc99baa43 (Improve pg_list.h's linitial(), lsecond() and co macros)
must've broke it somehow. The nearest thing that I can see to a theory
is that where DefineAggregate does
numDirectArgs = intVal(lsecond(args));
it's coming out with the wrong result, leading to a failure of the
numDirectArgs-vs-numArgs sanity check in AggregateCreate. But how could
that be? I hesitate to blame the compiler twice in one week. OTOH, it's
a not-very-mainstream compiler on a not-very-mainstream architecture.
Noah, can you poke into this in a little more detail and try to verify
what is happening?
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Recent failures on buildfarm member hornet
@ 2020-10-07 01:45 Noah Misch <noah@leadboat.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 12+ messages in thread
From: Noah Misch @ 2020-10-07 01:45 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org, David Rowley <dgrowleyml@gmail.com>
On Mon, Oct 05, 2020 at 08:42:15PM -0400, Tom Lane wrote:
> hornet has failed its last five runs with
>
> 2020-10-05 22:45:42.784 UTC [34734498:40] pg_regress/create_aggregate LOG: statement: create aggregate my_percentile_disc(float8 ORDER BY anyelement) (
> stype = internal,
> sfunc = ordered_set_transition,
> finalfunc = percentile_disc_final,
> finalfunc_extra = true,
> finalfunc_modify = read_write
> );
> TRAP: FailedAssertion("variadicArgType != InvalidOid", File: "pg_aggregate.c", Line: 216, PID: 34734498)
>
> After looking at the commits immediately preceding the first failure, and
> digging around in the aggregate-related code, it seems like commit
> cc99baa43 (Improve pg_list.h's linitial(), lsecond() and co macros)
> must've broke it somehow. The nearest thing that I can see to a theory
> is that where DefineAggregate does
> numDirectArgs = intVal(lsecond(args));
> it's coming out with the wrong result, leading to a failure of the
> numDirectArgs-vs-numArgs sanity check in AggregateCreate.
Building the tree with -O0 suppresses the problem. (xlc does not have -O1.)
Building just aggregatecmds.c and pg_aggregate.c that way does not, so I
suppose the damage arose elsewhere.
> But how could
> that be? I hesitate to blame the compiler twice in one week. OTOH, it's
> a not-very-mainstream compiler on a not-very-mainstream architecture.
A compiler bug is plausible. The compiler is eight years old, and we're not
seeing the problem on 32-bit (mandrill) or on 2019-vintage xlc (hoverfly).
Shall I make this animal use -O0 on v14+?
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Recent failures on buildfarm member hornet
@ 2020-10-07 01:56 Tom Lane <tgl@sss.pgh.pa.us>
parent: Noah Misch <noah@leadboat.com>
0 siblings, 1 reply; 12+ messages in thread
From: Tom Lane @ 2020-10-07 01:56 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: pgsql-hackers@lists.postgresql.org, David Rowley <dgrowleyml@gmail.com>
Noah Misch <noah@leadboat.com> writes:
> On Mon, Oct 05, 2020 at 08:42:15PM -0400, Tom Lane wrote:
>> hornet has failed its last five runs with
>> TRAP: FailedAssertion("variadicArgType != InvalidOid", File: "pg_aggregate.c", Line: 216, PID: 34734498)
> Building the tree with -O0 suppresses the problem. (xlc does not have -O1.)
OK, that's pretty unsurprising.
> Building just aggregatecmds.c and pg_aggregate.c that way does not, so I
> suppose the damage arose elsewhere.
Now that *is* surprising. Could you poke a little further to determine
which module is getting miscompiled? (gram.o seems like the next most
likely bet.)
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Recent failures on buildfarm member hornet
@ 2020-10-07 02:19 Noah Misch <noah@leadboat.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 12+ messages in thread
From: Noah Misch @ 2020-10-07 02:19 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org, David Rowley <dgrowleyml@gmail.com>
On Tue, Oct 06, 2020 at 09:56:49PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Mon, Oct 05, 2020 at 08:42:15PM -0400, Tom Lane wrote:
> >> hornet has failed its last five runs with
> >> TRAP: FailedAssertion("variadicArgType != InvalidOid", File: "pg_aggregate.c", Line: 216, PID: 34734498)
>
> > Building the tree with -O0 suppresses the problem. (xlc does not have -O1.)
>
> OK, that's pretty unsurprising.
>
> > Building just aggregatecmds.c and pg_aggregate.c that way does not, so I
> > suppose the damage arose elsewhere.
>
> Now that *is* surprising. Could you poke a little further to determine
> which module is getting miscompiled? (gram.o seems like the next most
> likely bet.)
gram.o was it. (The way I rebuilt gram.o also rebuilt parser.o and scan.o,
but those seem far less likely.)
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Recent failures on buildfarm member hornet
@ 2020-10-07 17:13 Tom Lane <tgl@sss.pgh.pa.us>
parent: Noah Misch <noah@leadboat.com>
0 siblings, 1 reply; 12+ messages in thread
From: Tom Lane @ 2020-10-07 17:13 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: pgsql-hackers@lists.postgresql.org, David Rowley <dgrowleyml@gmail.com>
Noah Misch <noah@leadboat.com> writes:
> On Tue, Oct 06, 2020 at 09:56:49PM -0400, Tom Lane wrote:
>> Now that *is* surprising. Could you poke a little further to determine
>> which module is getting miscompiled? (gram.o seems like the next most
>> likely bet.)
> gram.o was it. (The way I rebuilt gram.o also rebuilt parser.o and scan.o,
> but those seem far less likely.)
This suggests that the problem is misoptimization of gram.y's
makeOrderedSetArgs:
/* don't merge into the next line, as list_concat changes directargs */
ndirectargs = list_length(directargs);
return list_make2(list_concat(directargs, orderedargs),
makeInteger(ndirectargs));
I think that if the compiler did what the comment says not to, it'd match
this symptom. However, I'm baffled as to why our recent pg_list.h changes
would've affected that.
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Recent failures on buildfarm member hornet
@ 2020-10-07 20:15 David Rowley <dgrowleyml@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 12+ messages in thread
From: David Rowley @ 2020-10-07 20:15 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Noah Misch <noah@leadboat.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Thanks for investigating this, Noah.
On Thu, 8 Oct 2020 at 06:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Noah Misch <noah@leadboat.com> writes:
> > gram.o was it. (The way I rebuilt gram.o also rebuilt parser.o and scan.o,
> > but those seem far less likely.)
>
> This suggests that the problem is misoptimization of gram.y's
> makeOrderedSetArgs:
It would be interesting to see gram.s from both cc99baa4~1 and cc99baa4.
David
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Recent failures on buildfarm member hornet
@ 2020-10-07 21:20 Noah Misch <noah@leadboat.com>
parent: David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Noah Misch @ 2020-10-07 21:20 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
On Thu, Oct 08, 2020 at 09:15:12AM +1300, David Rowley wrote:
> On Thu, 8 Oct 2020 at 06:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Noah Misch <noah@leadboat.com> writes:
> > > gram.o was it. (The way I rebuilt gram.o also rebuilt parser.o and scan.o,
> > > but those seem far less likely.)
> >
> > This suggests that the problem is misoptimization of gram.y's
> > makeOrderedSetArgs:
>
> It would be interesting to see gram.s from both cc99baa4~1 and cc99baa4.
Attached. Both generated like this:
(cd src/backend/parser && xlc_r -D_LARGE_FILES=1 -qnoansialias -g -O2 -S -qmaxmem=33554432 -I. -I. -I../../../src/include -c gram.c; )
(Again, the compiler is eight years old, and we're not seeing the problem on
32-bit (mandrill) or on 2019-vintage xlc (hoverfly). As soon as the situation
no longer piques your curiosity, I'm happy to make hoverfly use -O0 on v14+.)
Attachments:
gram-cc99baa~1.s.gz (application/x-gunzip, 394.8K) download
gram-cc99baa.s.gz (application/x-gunzip, 394.7K) download
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Recent failures on buildfarm member hornet
@ 2020-10-07 21:48 Tom Lane <tgl@sss.pgh.pa.us>
parent: Noah Misch <noah@leadboat.com>
0 siblings, 1 reply; 12+ messages in thread
From: Tom Lane @ 2020-10-07 21:48 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: David Rowley <dgrowleyml@gmail.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Noah Misch <noah@leadboat.com> writes:
> On Thu, Oct 08, 2020 at 09:15:12AM +1300, David Rowley wrote:
>> It would be interesting to see gram.s from both cc99baa4~1 and cc99baa4.
> Attached. Both generated like this:
Hm. I'm too lazy to go bone up on PPC64 ABI conventions, but this does
look suspiciously like the compiler is doing what I feared:
GOOD:
lwa r31,4(r27) # fetching list_length(directargs) ?
.line 16295
ori r3,r27,0x0000
bl .list_concat{PR}
ori r0,r0,0x0000
std r3,112(SP)
.line 16295
extsw r3,r31 # ... and passing it to makeInteger
bl .makeInteger{PR}
ori r0,r0,0x0000
BAD:
ori r3,r31,0x0000
bl .list_concat{PR}
ori r0,r0,0x0000
std r3,112(SP)
.line 16288
lwa r3,4(r31) # fetching list_length(directargs) ?
bl .makeInteger{PR}
ori r0,r0,0x0000
(I'm confused about why the line numbers don't match up, since cc99baa4
did not touch gram.y. But whatever.)
I'm tempted to propose the attached small code rearrangement, which
might dissuade the compiler from thinking it can get away with this.
While I concur with your point that an old xlc version might not be
that exciting, there could be other compilers doing the same thing
in the future.
regards, tom lane
Attachments:
rearrange-makeOrderedSetArgs.patch (text/x-diff, 905B) download
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 0d101d8171..480d168346 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -16291,7 +16291,7 @@ makeOrderedSetArgs(List *directargs, List *orderedargs,
core_yyscan_t yyscanner)
{
FunctionParameter *lastd = (FunctionParameter *) llast(directargs);
- int ndirectargs;
+ Value *ndirectargs;
/* No restriction unless last direct arg is VARIADIC */
if (lastd->mode == FUNC_PARAM_VARIADIC)
@@ -16315,10 +16315,10 @@ makeOrderedSetArgs(List *directargs, List *orderedargs,
}
/* don't merge into the next line, as list_concat changes directargs */
- ndirectargs = list_length(directargs);
+ ndirectargs = makeInteger(list_length(directargs));
return list_make2(list_concat(directargs, orderedargs),
- makeInteger(ndirectargs));
+ ndirectargs);
}
/* insertSelectOptions()
* Re: Recent failures on buildfarm member hornet
@ 2020-10-07 22:03 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 12+ messages in thread
From: Tom Lane @ 2020-10-07 22:03 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: David Rowley <dgrowleyml@gmail.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
I wrote:
> I'm tempted to propose the attached small code rearrangement, which
> might dissuade the compiler from thinking it can get away with this.
> While I concur with your point that an old xlc version might not be
> that exciting, there could be other compilers doing the same thing
> in the future.
After thinking about it a bit more, I'm not even convinced that what
xlc seems to be doing is illegal per C spec. There are no sequence
points within
return list_make2(list_concat(directargs, orderedargs),
makeInteger(ndirectargs));
and therefore there's an argument to be made that the compiler
doesn't have to care whether any side-effects of list_concat() occur
before or after the evaluation of makeInteger(ndirectargs).
If the potential side-effects of list_concat() can be disregarded
until the end of this statement, then the code change is perfectly legal.
Maybe some very careful language-lawyering could prove differently,
but it's not as open-and-shut as one could wish. So now I'm thinking
that we need this patch anyway, xlc or not.
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Recent failures on buildfarm member hornet
@ 2020-10-07 22:12 Noah Misch <noah@leadboat.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 12+ messages in thread
From: Noah Misch @ 2020-10-07 22:12 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Rowley <dgrowleyml@gmail.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
On Wed, Oct 07, 2020 at 06:03:16PM -0400, Tom Lane wrote:
> I wrote:
> > I'm tempted to propose the attached small code rearrangement, which
> > might dissuade the compiler from thinking it can get away with this.
That patch does get hornet's -O2 build to again pass "make check". It doesn't
harm the code, so we may as well use it.
> > While I concur with your point that an old xlc version might not be
> > that exciting, there could be other compilers doing the same thing
> > in the future.
>
> After thinking about it a bit more, I'm not even convinced that what
> xlc seems to be doing is illegal per C spec. There are no sequence
> points within
>
> return list_make2(list_concat(directargs, orderedargs),
> makeInteger(ndirectargs));
There is, however, a sequence point between list_length(directargs) and
list_concat(), and the problem arises because xlc reorders those two. It's
true that makeInteger() could run before or after list_concat(), but that
alone would not have been a problem.
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Recent failures on buildfarm member hornet
@ 2020-10-07 22:22 Tom Lane <tgl@sss.pgh.pa.us>
parent: Noah Misch <noah@leadboat.com>
0 siblings, 1 reply; 12+ messages in thread
From: Tom Lane @ 2020-10-07 22:22 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: David Rowley <dgrowleyml@gmail.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Noah Misch <noah@leadboat.com> writes:
> On Wed, Oct 07, 2020 at 06:03:16PM -0400, Tom Lane wrote:
>> After thinking about it a bit more, I'm not even convinced that what
>> xlc seems to be doing is illegal per C spec. There are no sequence
>> points within
>>
>> return list_make2(list_concat(directargs, orderedargs),
>> makeInteger(ndirectargs));
> There is, however, a sequence point between list_length(directargs) and
> list_concat(), and the problem arises because xlc reorders those two. It's
> true that makeInteger() could run before or after list_concat(), but that
> alone would not have been a problem.
Yeah, that is the theory on which the existing code is built,
specifically that the list_length fetch must occur before list_concat
runs. What I am wondering about is a more aggressive interpretation of
"sequence point", namely that the compiler is free to disregard exactly
when list_concat's side-effects occur between this statement's sequence
points. I'm not sure that the C spec allows that interpretation, but
I'm not sure it doesn't, either.
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Recent failures on buildfarm member hornet
@ 2020-10-07 22:36 Noah Misch <noah@leadboat.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 12+ messages in thread
From: Noah Misch @ 2020-10-07 22:36 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Rowley <dgrowleyml@gmail.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
On Wed, Oct 07, 2020 at 06:22:04PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Wed, Oct 07, 2020 at 06:03:16PM -0400, Tom Lane wrote:
> >> After thinking about it a bit more, I'm not even convinced that what
> >> xlc seems to be doing is illegal per C spec. There are no sequence
> >> points within
> >>
> >> return list_make2(list_concat(directargs, orderedargs),
> >> makeInteger(ndirectargs));
>
> > There is, however, a sequence point between list_length(directargs) and
> > list_concat(), and the problem arises because xlc reorders those two. It's
> > true that makeInteger() could run before or after list_concat(), but that
> > alone would not have been a problem.
>
> Yeah, that is the theory on which the existing code is built,
> specifically that the list_length fetch must occur before list_concat
> runs. What I am wondering about is a more aggressive interpretation of
> "sequence point", namely that the compiler is free to disregard exactly
> when list_concat's side-effects occur between this statement's sequence
> points. I'm not sure that the C spec allows that interpretation, but
> I'm not sure it doesn't, either.
C does not allow that, because the list_length() happens in a different "full
expression" (different statement):
ndirectargs = list_length(directargs);/* noah adds: a sequence point is here */
return list_make2(list_concat(directargs, orderedargs),
makeInteger(ndirectargs));
A compiler may implement like this:
ndirectargs = list_length(directargs);
a := list_concat(directargs, orderedargs);
b := makeInteger(ndirectargs);
return list_make2(a, b);
Or this:
ndirectargs = list_length(directargs);
b := makeInteger(ndirectargs);
a := list_concat(directargs, orderedargs);
return list_make2(a, b);
But not like this:
a := list_concat(directargs, orderedargs);
ndirectargs = list_length(directargs);
b := makeInteger(ndirectargs);
return list_make2(a, b);
^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2020-10-07 22:36 UTC | newest]
Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-10-06 00:42 Recent failures on buildfarm member hornet Tom Lane <tgl@sss.pgh.pa.us>
2020-10-07 01:45 ` Noah Misch <noah@leadboat.com>
2020-10-07 01:56 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-10-07 02:19 ` Noah Misch <noah@leadboat.com>
2020-10-07 17:13 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-10-07 20:15 ` David Rowley <dgrowleyml@gmail.com>
2020-10-07 21:20 ` Noah Misch <noah@leadboat.com>
2020-10-07 21:48 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-10-07 22:03 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-10-07 22:12 ` Noah Misch <noah@leadboat.com>
2020-10-07 22:22 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-10-07 22:36 ` Noah Misch <noah@leadboat.com>
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