pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedOverflow hazard in pgbench
7+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2021-06-28 3fd334795e be567deb3a 203c5aaaba 01697e92a4 Don't depend on -fwrapv semantics in pgbench's random() function.
* Overflow hazard in pgbench
@ 2021-06-27 17:39 Tom Lane <tgl@sss.pgh.pa.us>
2021-06-27 20:21 ` Re: Overflow hazard in pgbench Tom Lane <tgl@sss.pgh.pa.us>
2021-06-28 08:13 ` Re: Overflow hazard in pgbench Fabien COELHO <coelho@cri.ensmp.fr>
0 siblings, 2 replies; 7+ messages in thread
From: Tom Lane @ 2021-06-27 17:39 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Fabien COELHO <coelho@cri.ensmp.fr>
moonjelly just reported an interesting failure [1]. It seems that
with the latest bleeding-edge gcc, this code is misoptimized:
/* check random range */
if (imin > imax)
{
pg_log_error("empty range given to random");
return false;
}
else if (imax - imin < 0 || (imax - imin) + 1 < 0)
{
/* prevent int overflows in random functions */
pg_log_error("random range is too large");
return false;
}
such that the second if-test doesn't fire. Now, according to the C99
spec this code is broken, because the compiler is allowed to assume
that signed integer overflow doesn't happen, whereupon the second
if-block is provably unreachable. The failure still represents a gcc
bug, because we're using -fwrapv which should disable that assumption.
However, not all compilers have that switch, so it'd be better to code
this in a spec-compliant way. I suggest applying the attached in
branches that have the required functions.
[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=moonjelly&dt=2021-06-26%2007%3A03%3A17
regards, tom lane
Attachments:
avoid-pgbench-overflow-hazard-1.patch (text/x-diff, 744B) download
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e61055b6b7..c4023bfa27 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -2450,7 +2450,8 @@ evalStandardFunc(CState *st,
case PGBENCH_RANDOM_ZIPFIAN:
{
int64 imin,
- imax;
+ imax,
+ delta;
Assert(nargs >= 2);
@@ -2464,7 +2465,8 @@ evalStandardFunc(CState *st,
pg_log_error("empty range given to random");
return false;
}
- else if (imax - imin < 0 || (imax - imin) + 1 < 0)
+ else if (pg_sub_s64_overflow(imax, imin, &delta) ||
+ pg_add_s64_overflow(delta, 1, &delta))
{
/* prevent int overflows in random functions */
pg_log_error("random range is too large");
* Re: Overflow hazard in pgbench
2021-06-27 17:39 Overflow hazard in pgbench Tom Lane <tgl@sss.pgh.pa.us>
@ 2021-06-27 20:21 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-07-09 02:10 ` Re: Overflow hazard in pgbench Andres Freund <andres@anarazel.de>
1 sibling, 1 reply; 7+ messages in thread
From: Tom Lane @ 2021-06-27 20:21 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Fabien COELHO <coelho@cri.ensmp.fr>
I wrote:
> ... according to the C99
> spec this code is broken, because the compiler is allowed to assume
> that signed integer overflow doesn't happen, whereupon the second
> if-block is provably unreachable. The failure still represents a gcc
> bug, because we're using -fwrapv which should disable that assumption.
> However, not all compilers have that switch, so it'd be better to code
> this in a spec-compliant way.
BTW, for grins I tried building today's HEAD without -fwrapv, using
gcc version 11.1.1 20210531 (Red Hat 11.1.1-3) (GCC)
which is the newest version I have at hand. Not very surprisingly,
that reproduced the failure shown on moonjelly. However, after adding
the patch I proposed, "make check-world" passed! I was not expecting
that result; I supposed we still had lots of lurking assumptions of
traditional C overflow handling.
I'm not in any hurry to remove -fwrapv, because (a) this result doesn't
show that we have no such assumptions, only that they must be lurking
in darker, poorly-tested corners, and (b) I'm not aware of any reason
to think that removing -fwrapv would provide benefits worth taking any
risks for. But we may be closer to being able to do without that
switch than I thought.
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Overflow hazard in pgbench
2021-06-27 17:39 Overflow hazard in pgbench Tom Lane <tgl@sss.pgh.pa.us>
2021-06-27 20:21 ` Re: Overflow hazard in pgbench Tom Lane <tgl@sss.pgh.pa.us>
@ 2021-07-09 02:10 ` Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 7+ messages in thread
From: Andres Freund @ 2021-07-09 02:10 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org, Fabien COELHO <coelho@cri.ensmp.fr>
Hi,
On 2021-06-27 16:21:46 -0400, Tom Lane wrote:
> BTW, for grins I tried building today's HEAD without -fwrapv, using
> gcc version 11.1.1 20210531 (Red Hat 11.1.1-3) (GCC)
> which is the newest version I have at hand. Not very surprisingly,
> that reproduced the failure shown on moonjelly. However, after adding
> the patch I proposed, "make check-world" passed! I was not expecting
> that result; I supposed we still had lots of lurking assumptions of
> traditional C overflow handling.
We did fix a lot of them a few years back...
> I'm not in any hurry to remove -fwrapv, because (a) this result doesn't
> show that we have no such assumptions, only that they must be lurking
> in darker, poorly-tested corners, and (b) I'm not aware of any reason
> to think that removing -fwrapv would provide benefits worth taking any
> risks for. But we may be closer to being able to do without that
> switch than I thought.
Lack of failures after removing frwapv itself doesn't prove that much -
very commonly the compiler won't optimize based on the improved
knowledge about value range. Additionally we probably don't exercise all
effected places in our tests.
ubsan is able to catch all signed overflows. The last time I played
around with that, tests still were hitting quite a few cases of
overflows. But most not in particularly interesting places
(e.g. cash_out, RIGHTMOST_ONE()) but also a few where it might be worth
being careful about it in case a compiler disregards -fwrapv or doesn't
implement it (e.g. _dorand48()).
It might be worth setting up a bf animal with ubsan and enabled overflow
checking...
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Overflow hazard in pgbench
2021-06-27 17:39 Overflow hazard in pgbench Tom Lane <tgl@sss.pgh.pa.us>
@ 2021-06-28 08:13 ` Fabien COELHO <coelho@cri.ensmp.fr>
2021-06-28 16:43 ` Re: Overflow hazard in pgbench Tom Lane <tgl@sss.pgh.pa.us>
2021-06-29 09:47 ` Re: Overflow hazard in pgbench Fabien COELHO <coelho@cri.ensmp.fr>
1 sibling, 2 replies; 7+ messages in thread
From: Fabien COELHO @ 2021-06-28 08:13 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Hello Tom,
> moonjelly just reported an interesting failure [1].
I noticed. I was planning to have a look at it, thanks for digging!
> It seems that with the latest bleeding-edge gcc, this code is
> misoptimized:
> else if (imax - imin < 0 || (imax - imin) + 1 < 0)
> {
> /* prevent int overflows in random functions */
> pg_log_error("random range is too large");
> return false;
> }
>
> such that the second if-test doesn't fire. Now, according to the C99
> spec this code is broken, because the compiler is allowed to assume
> that signed integer overflow doesn't happen,
Hmmm, so it is not possible to detect these with standard arithmetic as
done by this code. Note that the code was written in 2016, ISTM pre C99
Postgres. I'm unsure about what a C compiler could assume on the previous
standard wrt integer arithmetic.
> whereupon the second if-block is provably unreachable.
Indeed.
> The failure still represents a gcc bug, because we're using -fwrapv
> which should disable that assumption.
Ok, I'll report it.
I also see a good point with pgbench tests exercising edge cases.
> However, not all compilers have that switch, so it'd be better to code
> this in a spec-compliant way.
Ok.
> I suggest applying the attached in branches that have the required
> functions.
The latest gcc, recompiled yesterday, is still wrong, as shown by
moonjelly current status.
The proposed patch does fix the issue in pgbench TAP test. I'd suggest to
add unlikely() on all these conditions, as already done elsewhere. See
attached version.
I confirm that check-world passed with gcc head and its broken -fwrapv.
I also recompiled after removing manually -fwrapv: Make check, pgbench TAP
tests and check-world all passed. I'm not sure that edge case are well
enough tested in postgres to be sure that all is ok just from these runs
though.
--
Fabien.
Attachments:
avoid-pgbench-overflow-hazard-2.patch (text/x-diff, 901B) download
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e61055b6b7..8d0cabdab2 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -2450,7 +2450,8 @@ evalStandardFunc(CState *st,
case PGBENCH_RANDOM_ZIPFIAN:
{
int64 imin,
- imax;
+ imax,
+ delta;
Assert(nargs >= 2);
@@ -2459,12 +2460,13 @@ evalStandardFunc(CState *st,
return false;
/* check random range */
- if (imin > imax)
+ if (unlikely(imin > imax))
{
pg_log_error("empty range given to random");
return false;
}
- else if (imax - imin < 0 || (imax - imin) + 1 < 0)
+ else if (unlikely(pg_sub_s64_overflow(imax, imin, &delta) ||
+ pg_add_s64_overflow(delta, 1, &delta)))
{
/* prevent int overflows in random functions */
pg_log_error("random range is too large");
* Re: Overflow hazard in pgbench
2021-06-27 17:39 Overflow hazard in pgbench Tom Lane <tgl@sss.pgh.pa.us>
2021-06-28 08:13 ` Re: Overflow hazard in pgbench Fabien COELHO <coelho@cri.ensmp.fr>
@ 2021-06-28 16:43 ` Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 7+ messages in thread
From: Tom Lane @ 2021-06-28 16:43 UTC (permalink / raw)
To: Fabien COELHO <coelho@cri.ensmp.fr>; +Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> I suggest applying the attached in branches that have the required
>> functions.
> The proposed patch does fix the issue in pgbench TAP test. I'd suggest to
> add unlikely() on all these conditions, as already done elsewhere. See
> attached version.
Done that way, though I'm skeptical that it makes any measurable
difference.
> I also recompiled after removing manually -fwrapv: Make check, pgbench TAP
> tests and check-world all passed. I'm not sure that edge case are well
> enough tested in postgres to be sure that all is ok just from these runs
> though.
Yeah, I'm afraid that in most places it'd take a specifically-designed
test case to expose a problem, if there is one.
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Overflow hazard in pgbench
2021-06-27 17:39 Overflow hazard in pgbench Tom Lane <tgl@sss.pgh.pa.us>
2021-06-28 08:13 ` Re: Overflow hazard in pgbench Fabien COELHO <coelho@cri.ensmp.fr>
@ 2021-06-29 09:47 ` Fabien COELHO <coelho@cri.ensmp.fr>
2021-06-29 20:52 ` Re: Overflow hazard in pgbench Fabien COELHO <coelho@cri.ensmp.fr>
1 sibling, 1 reply; 7+ messages in thread
From: Fabien COELHO @ 2021-06-29 09:47 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
>> The failure still represents a gcc bug, because we're using -fwrapv which
>> should disable that assumption.
>
> Ok, I'll report it.
Done at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101254
--
Fabien.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Overflow hazard in pgbench
2021-06-27 17:39 Overflow hazard in pgbench Tom Lane <tgl@sss.pgh.pa.us>
2021-06-28 08:13 ` Re: Overflow hazard in pgbench Fabien COELHO <coelho@cri.ensmp.fr>
2021-06-29 09:47 ` Re: Overflow hazard in pgbench Fabien COELHO <coelho@cri.ensmp.fr>
@ 2021-06-29 20:52 ` Fabien COELHO <coelho@cri.ensmp.fr>
0 siblings, 0 replies; 7+ messages in thread
From: Fabien COELHO @ 2021-06-29 20:52 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Hello Tom,
>>> The failure still represents a gcc bug, because we're using -fwrapv which
>>> should disable that assumption.
>>
>> Ok, I'll report it.
>
> Done at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101254
Fixed at r12-1916-ga96d8d67d0073a7031c0712bc3fb7759417b2125
https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=a96d8d67d0073a7031c0712bc3fb7759417b2125
Just under 10 hours from the bug report…
--
Fabien.
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2021-07-09 02:10 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-06-27 17:39 Overflow hazard in pgbench Tom Lane <tgl@sss.pgh.pa.us>
2021-06-27 20:21 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-07-09 02:10 ` Andres Freund <andres@anarazel.de>
2021-06-28 08:13 ` Fabien COELHO <coelho@cri.ensmp.fr>
2021-06-28 16:43 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-06-29 09:47 ` Fabien COELHO <coelho@cri.ensmp.fr>
2021-06-29 20:52 ` Fabien COELHO <coelho@cri.ensmp.fr>
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