Date: Mon, 28 Jun 2021 10:13:11 +0200 (CEST)
From: Fabien COELHO <coelho@cri.ensmp.fr>
To: Tom Lane <tgl@sss.pgh.pa.us>
cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Overflow hazard in pgbench
In-Reply-To: <73927.1624815543@sss.pgh.pa.us>
Message-ID: <alpine.DEB.2.22.394.2106272024180.482873@pseudo>
References: <73927.1624815543@sss.pgh.pa.us>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="2056645051-2073406906-1624868000=:1401908"
Archived-At: 
 <https://www.postgresql.org/message-id/alpine.DEB.2.22.394.2106272024180.482873%40pseudo>
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--2056645051-2073406906-1624868000=:1401908
Content-Type: text/plain; format=flowed; charset=US-ASCII


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.
--2056645051-2073406906-1624868000=:1401908
Content-Type: text/x-diff; name=avoid-pgbench-overflow-hazard-2.patch
Content-Transfer-Encoding: BASE64
Content-ID: <alpine.DEB.2.22.394.2106281013110.1401908@pseudo>
Content-Description: 
Content-Disposition: attachment;
 filename=avoid-pgbench-overflow-hazard-2.patch

ZGlmZiAtLWdpdCBhL3NyYy9iaW4vcGdiZW5jaC9wZ2JlbmNoLmMgYi9zcmMv
YmluL3BnYmVuY2gvcGdiZW5jaC5jDQppbmRleCBlNjEwNTViNmI3Li44ZDBj
YWJkYWIyIDEwMDY0NA0KLS0tIGEvc3JjL2Jpbi9wZ2JlbmNoL3BnYmVuY2gu
Yw0KKysrIGIvc3JjL2Jpbi9wZ2JlbmNoL3BnYmVuY2guYw0KQEAgLTI0NTAs
NyArMjQ1MCw4IEBAIGV2YWxTdGFuZGFyZEZ1bmMoQ1N0YXRlICpzdCwNCiAJ
CWNhc2UgUEdCRU5DSF9SQU5ET01fWklQRklBTjoNCiAJCQl7DQogCQkJCWlu
dDY0CQlpbWluLA0KLQkJCQkJCQlpbWF4Ow0KKwkJCQkJCQlpbWF4LA0KKwkJ
CQkJCQlkZWx0YTsNCiANCiAJCQkJQXNzZXJ0KG5hcmdzID49IDIpOw0KIA0K
QEAgLTI0NTksMTIgKzI0NjAsMTMgQEAgZXZhbFN0YW5kYXJkRnVuYyhDU3Rh
dGUgKnN0LA0KIAkJCQkJcmV0dXJuIGZhbHNlOw0KIA0KIAkJCQkvKiBjaGVj
ayByYW5kb20gcmFuZ2UgKi8NCi0JCQkJaWYgKGltaW4gPiBpbWF4KQ0KKwkJ
CQlpZiAodW5saWtlbHkoaW1pbiA+IGltYXgpKQ0KIAkJCQl7DQogCQkJCQlw
Z19sb2dfZXJyb3IoImVtcHR5IHJhbmdlIGdpdmVuIHRvIHJhbmRvbSIpOw0K
IAkJCQkJcmV0dXJuIGZhbHNlOw0KIAkJCQl9DQotCQkJCWVsc2UgaWYgKGlt
YXggLSBpbWluIDwgMCB8fCAoaW1heCAtIGltaW4pICsgMSA8IDApDQorCQkJ
CWVsc2UgaWYgKHVubGlrZWx5KHBnX3N1Yl9zNjRfb3ZlcmZsb3coaW1heCwg
aW1pbiwgJmRlbHRhKSB8fA0KKwkJCQkJCSAgcGdfYWRkX3M2NF9vdmVyZmxv
dyhkZWx0YSwgMSwgJmRlbHRhKSkpDQogCQkJCXsNCiAJCQkJCS8qIHByZXZl
bnQgaW50IG92ZXJmbG93cyBpbiByYW5kb20gZnVuY3Rpb25zICovDQogCQkJ
CQlwZ19sb2dfZXJyb3IoInJhbmRvbSByYW5nZSBpcyB0b28gbGFyZ2UiKTsN
Cg==

--2056645051-2073406906-1624868000=:1401908--