MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="=_aa6ec392b1565857f367e9a4a18844e9"
Date: Wed, 17 Jan 2018 18:12:44 +0300
From: Marina Polyakova <m.polyakova@postgrespro.ru>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: vitus@wagner.pp.ru, Andres Freund <andres@anarazel.de>,
 pgsql-hackers@postgresql.org, Alvaro Herrera <alvherre@alvh.no-ip.org>
Subject: Re: master make check fails on Solaris 10
Organization: Postgres Professional
In-Reply-To: <90ab676392c8f9c84431976147097cf0@postgrespro.ru>
References: <0d3a9fa264cebe1cb9966f37b7c06e86@postgrespro.ru>
 <20180111173938.jimybeayp2u5mmup@alap3.anarazel.de>
 <31a342fa6a025a8bdfc5b8b19c29d56d@postgrespro.ru>
 <fc3c1b52d2d4121cccfc13635005bbd0@postgrespro.ru>
 <1232.1515780057@sss.pgh.pa.us>
 <b946893927ccfd87146d8a7c9f3bbe29@postgrespro.ru>
 <15299.1515867055@sss.pgh.pa.us>
 <a5559105243efcf6fb619d86c5702115@postgrespro.ru>
 <18209.1516059711@sss.pgh.pa.us>
 <90ab676392c8f9c84431976147097cf0@postgrespro.ru>
Message-ID: <4b5a7c377a6f7443f149049849e7bc77@postgrespro.ru>
User-Agent: Roundcube Webmail/1.2.4
Precedence: bulk

--=_aa6ec392b1565857f367e9a4a18844e9
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset=US-ASCII;
 format=flowed

Sorry, diff.patch is attached now.

On 17-01-2018 18:02, Marina Polyakova wrote:
> [I added Victor Wagner as co-researcher of this problem]
> 
> On 13-01-2018 21:10, Tom Lane wrote:
>> In the end this might just be an instance of the old saw about
>> avoiding dot-zero releases.  Have you tried a newer gcc?
>> (Digging in their bugzilla finds quite a number of __int128 bugs
>> fixed in 5.4.x, though none look to be specifically about
>> misaligned data.)
> 
> gcc 5.5.0 (from [1]) did not fix the problem..
> 
> On 16-01-2018 2:41, Tom Lane wrote:
>> Marina Polyakova <m.polyakova@postgrespro.ru> writes:
>>> On 13-01-2018 21:10, Tom Lane wrote:
>>>> I'm not sure there's much we can do about this.  Dropping the use
>>>> of the alignment spec isn't a workable option.  If there were a
>>>> simple way for configure to detect that the compiler generates bad
>>>> code for that, we could have it do so and reject use of __int128,
>>>> but it'd be up to you to come up with a workable test.
>> 
>>> I'll think about it..
>> 
>> Attached is a possible test program.  I can confirm it passes on a
>> machine with working __int128, but I have no idea whether it will
>> detect the problem on yours.  If not, maybe you can tweak it?
> 
> Thank you! Using gcc 5.5.0 it prints that everything is ok. But,
> investigating the regression diffs, we found out that the error occurs
> when we pass int128 as not the first argument to the function (perhaps
> its value is replaced by the value of some address):
> 
> -- Use queries from random.sql
> SELECT count(*) FROM onek; -- Everything is ok
> ...
> SELECT random, count(random) FROM RANDOM_TBL
>   GROUP BY random HAVING count(random) > 3; -- Everything is ok
> 
> postgres=# SELECT * FROM RANDOM_TBL ORDER BY random; -- Print current 
> data
>  random
> --------
>      78
>      86
>      98
>      98
> (4 rows)
> 
> postgres=# SELECT AVG(random) FROM RANDOM_TBL
> postgres-#   HAVING AVG(random) NOT BETWEEN 80 AND 120; -- Oops!
>               avg
> -------------------------------
>  79446934848446476698976780288
> (1 row)
> 
> Debug output from the last query (see attached diff.patch, it is based
> on commit 9c7d06d60680c7f00d931233873dee81fdb311c6 of master):
> 
> makeInt128AggState
> int8_avg_accum val 98
> int8_avg_accum val_int128 as 2 x int64: 0 98
> int8_avg_accum val_int128 bytes: 00000000000000000000000000000062
> int8_avg_accum state 100e648d8
> int8_avg_accum 1007f2e94
> do_int128_accum int128 newval as 2 x int64: 4306826968 0
> do_int128_accum int128 newval bytes: 0000000100B4F6D80000000000000000
> do_int128_accum state 100e648d8
> do_int128_accum 1007f1e30
> int8_avg_accum val 86
> int8_avg_accum val_int128 as 2 x int64: 0 86
> int8_avg_accum val_int128 bytes: 00000000000000000000000000000056
> int8_avg_accum state 100e648d8
> int8_avg_accum 1007f2e94
> do_int128_accum int128 newval as 2 x int64: 4306826968 0
> do_int128_accum int128 newval bytes: 0000000100B4F6D80000000000000000
> do_int128_accum state 100e648d8
> do_int128_accum 1007f1e30
> int8_avg_accum val 98
> int8_avg_accum val_int128 as 2 x int64: 0 98
> int8_avg_accum val_int128 bytes: 00000000000000000000000000000062
> int8_avg_accum state 100e648d8
> int8_avg_accum 1007f2e94
> do_int128_accum int128 newval as 2 x int64: 4306826968 0
> do_int128_accum int128 newval bytes: 0000000100B4F6D80000000000000000
> do_int128_accum state 100e648d8
> do_int128_accum 1007f1e30
> int8_avg_accum val 78
> int8_avg_accum val_int128 as 2 x int64: 0 78
> int8_avg_accum val_int128 bytes: 0000000000000000000000000000004E
> int8_avg_accum state 100e648d8
> int8_avg_accum 1007f2e94
> do_int128_accum int128 newval as 2 x int64: 4306826968 0
> do_int128_accum int128 newval bytes: 0000000100B4F6D80000000000000000
> do_int128_accum state 100e648d8
> do_int128_accum 1007f1e30
> numeric_poly_avg
> int128_to_numericvar
> int128_to_numericvar int128 val as 2 x int64: 17227307872 0
> int128_to_numericvar int128 val bytes: 0000000402D3DB600000000000000000
> 
> (val_int128 in the function int8_avg_accum is correct, but newval in
> the function do_int128_accum is not equal to it. val in the function
> int128_to_numericvar is (4 * 4306826968).)
> 
> Based on this, we modified the test program (see attached). Here is
> its output on Solaris 10 for different alignments requirements for
> int128 (on my machine where make check-world passes everything is OK)
> (ALIGNOF_PG_INT128_TYPE is 16 on Solaris 10):
> 
> $ gcc -D PG_ALIGN_128=16 -m64 -o int128test2 int128test2.c
> $ ./int128test2
> basic aritmetic OK
> pass int 16 OK
> pass uint 16 OK
> pass int 32 OK
> pass int 64 OK
> pass int 128 OK
> $ gcc -D PG_ALIGN_128=8 -m64 -o int128test2 int128test2.c
> $ ./int128test2
> basic aritmetic OK
> pass int 16 FAILED
> pass uint 16 FAILED
> pass int 32 FAILED
> pass int 64 FAILED
> pass int 128 OK
> 
> Maybe some pass test from int128test2.c can be used to test __int128?
> 
> P.S. I suppose, g.b should be 97656250 to get 400000000005:
> 
>> struct glob128
>> {
>> 	__int128	start;
>> 	char		pad;
>> 	int128a		a;
>> 	int128a		b;
>> 	int128a		c;
>> 	int128a		d;
>> } g = {0, 'p', 48828125, 97656255, 0, 0};
>> ...
>> g.b = (g.b << 12) + 5;		/* 400000000005 */
> 
> [1] https://www.opencsw.org

-- 
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--=_aa6ec392b1565857f367e9a4a18844e9
Content-Transfer-Encoding: base64
Content-Type: text/x-diff;
 name=diff.patch
Content-Disposition: attachment;
 filename=diff.patch;
 size=2923

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3V0aWxzL2FkdC9udW1lcmljLmMgYi9zcmMvYmFja2Vu
ZC91dGlscy9hZHQvbnVtZXJpYy5jCmluZGV4IDViMzRiYWQuLjY0ZDEzMWYgMTAwNjQ0Ci0tLSBh
L3NyYy9iYWNrZW5kL3V0aWxzL2FkdC9udW1lcmljLmMKKysrIGIvc3JjL2JhY2tlbmQvdXRpbHMv
YWR0L251bWVyaWMuYwpAQCAtMzk1Myw2ICszOTUzLDggQEAgbWFrZUludDEyOEFnZ1N0YXRlKEZ1
bmN0aW9uQ2FsbEluZm8gZmNpbmZvLCBib29sIGNhbGNTdW1YMikKIAlNZW1vcnlDb250ZXh0IGFn
Z19jb250ZXh0OwogCU1lbW9yeUNvbnRleHQgb2xkX2NvbnRleHQ7CiAKKwlmcHJpbnRmKHN0ZGVy
ciwgIm1ha2VJbnQxMjhBZ2dTdGF0ZVxuIik7CisKIAlpZiAoIUFnZ0NoZWNrQ2FsbENvbnRleHQo
ZmNpbmZvLCAmYWdnX2NvbnRleHQpKQogCQllbG9nKEVSUk9SLCAiYWdncmVnYXRlIGZ1bmN0aW9u
IGNhbGxlZCBpbiBub24tYWdncmVnYXRlIGNvbnRleHQiKTsKIApAQCAtMzk4Nyw2ICszOTg5LDE4
IEBAIG1ha2VJbnQxMjhBZ2dTdGF0ZUN1cnJlbnRDb250ZXh0KGJvb2wgY2FsY1N1bVgyKQogc3Rh
dGljIHZvaWQKIGRvX2ludDEyOF9hY2N1bShJbnQxMjhBZ2dTdGF0ZSAqc3RhdGUsIGludDEyOCBu
ZXd2YWwpCiB7CisJZnByaW50ZihzdGRlcnIsICJkb19pbnQxMjhfYWNjdW0gaW50MTI4IG5ld3Zh
bCBhcyAyIHggaW50NjQ6ICVsZCAlbGRcbiIsCisJCQkoaW50NjQpIChuZXd2YWwgPj4gNjQpLCAo
aW50NjQpIG5ld3ZhbCk7CisKKwlmcHJpbnRmKHN0ZGVyciwgImRvX2ludDEyOF9hY2N1bSBpbnQx
MjggbmV3dmFsIGJ5dGVzOiAiKTsKKwl1bnNpZ25lZCBjaGFyKiBieXRlX2FycmF5ID0gKHZvaWQg
KikgJm5ld3ZhbDsKKwlmb3IgKGludCBpbmRleCA9IDA7IGluZGV4IDwgMTY7ICsraW5kZXgpCisJ
CWZwcmludGYoc3RkZXJyLCAiJTAyWCIsICh1bnNpZ25lZClieXRlX2FycmF5W2luZGV4XSk7CisJ
ZnByaW50ZihzdGRlcnIsICJcbiIpOworCisJZnByaW50ZihzdGRlcnIsICJkb19pbnQxMjhfYWNj
dW0gc3RhdGUgJXBcbiIsIHN0YXRlKTsKKwlmcHJpbnRmKHN0ZGVyciwgImRvX2ludDEyOF9hY2N1
bSAlcFxuIiwgJmRvX2ludDEyOF9hY2N1bSk7CisKIAlpZiAoc3RhdGUtPmNhbGNTdW1YMikKIAkJ
c3RhdGUtPnN1bVgyICs9IG5ld3ZhbCAqIG5ld3ZhbDsKIApAQCAtNDMwNywxMCArNDMyMSwyNyBA
QCBpbnQ4X2F2Z19hY2N1bShQR19GVU5DVElPTl9BUkdTKQogCWlmIChzdGF0ZSA9PSBOVUxMKQog
CQlzdGF0ZSA9IG1ha2VQb2x5TnVtQWdnU3RhdGUoZmNpbmZvLCBmYWxzZSk7CiAKKwlpbnQ2NCB2
YWwgPSBQR19HRVRBUkdfSU5UNjQoMSk7CisJZnByaW50ZihzdGRlcnIsICJpbnQ4X2F2Z19hY2N1
bSB2YWwgJWxkXG4iLCB2YWwpOworCisJaW50MTI4IHZhbF9pbnQxMjggPSAoaW50MTI4KSB2YWw7
CisKKwlmcHJpbnRmKHN0ZGVyciwgImludDhfYXZnX2FjY3VtIHZhbF9pbnQxMjggYXMgMiB4IGlu
dDY0OiAlbGQgJWxkXG4iLAorCQkJKGludDY0KSAodmFsX2ludDEyOCA+PiA2NCksIChpbnQ2NCkg
dmFsX2ludDEyOCk7CisKKwlmcHJpbnRmKHN0ZGVyciwgImludDhfYXZnX2FjY3VtIHZhbF9pbnQx
MjggYnl0ZXM6ICIpOworCXVuc2lnbmVkIGNoYXIqIGJ5dGVfYXJyYXkgPSAodm9pZCAqKSAmdmFs
X2ludDEyODsKKwlmb3IgKGludCBpbmRleCA9IDA7IGluZGV4IDwgMTY7ICsraW5kZXgpCisJCWZw
cmludGYoc3RkZXJyLCAiJTAyWCIsICh1bnNpZ25lZClieXRlX2FycmF5W2luZGV4XSk7CisJZnBy
aW50ZihzdGRlcnIsICJcbiIpOworCisJZnByaW50ZihzdGRlcnIsICJpbnQ4X2F2Z19hY2N1bSBz
dGF0ZSAlcFxuIiwgc3RhdGUpOworCWZwcmludGYoc3RkZXJyLCAiaW50OF9hdmdfYWNjdW0gJXBc
biIsICZpbnQ4X2F2Z19hY2N1bSk7CisKIAlpZiAoIVBHX0FSR0lTTlVMTCgxKSkKIAl7CiAjaWZk
ZWYgSEFWRV9JTlQxMjgKLQkJZG9faW50MTI4X2FjY3VtKHN0YXRlLCAoaW50MTI4KSBQR19HRVRB
UkdfSU5UNjQoMSkpOworCQlkb19pbnQxMjhfYWNjdW0oc3RhdGUsIHZhbF9pbnQxMjgpOwogI2Vs
c2UKIAkJTnVtZXJpYwkJbmV3dmFsOwogCkBAIC00NjQwLDYgKzQ2NzEsNyBAQCBudW1lcmljX3Bv
bHlfc3VtKFBHX0ZVTkNUSU9OX0FSR1MpCiBEYXR1bQogbnVtZXJpY19wb2x5X2F2ZyhQR19GVU5D
VElPTl9BUkdTKQogeworCWZwcmludGYoc3RkZXJyLCAibnVtZXJpY19wb2x5X2F2Z1xuIik7CiAj
aWZkZWYgSEFWRV9JTlQxMjgKIAlQb2x5TnVtQWdnU3RhdGUgKnN0YXRlOwogCU51bWVyaWNWYXIJ
cmVzdWx0OwpAQCAtNjM2NSw2ICs2Mzk3LDE3IEBAIGludDEyOF90b19udW1lcmljdmFyKGludDEy
OCB2YWwsIE51bWVyaWNWYXIgKnZhcikKIAlOdW1lcmljRGlnaXQgKnB0cjsKIAlpbnQJCQluZGln
aXRzOwogCisJZnByaW50ZihzdGRlcnIsICJpbnQxMjhfdG9fbnVtZXJpY3ZhclxuIik7CisKKwlm
cHJpbnRmKHN0ZGVyciwgImludDEyOF90b19udW1lcmljdmFyIGludDEyOCB2YWwgYXMgMiB4IGlu
dDY0OiAlbGQgJWxkXG4iLAorCQkJKGludDY0KSAodmFsID4+IDY0KSwgKGludDY0KSB2YWwpOwor
CisJZnByaW50ZihzdGRlcnIsICJpbnQxMjhfdG9fbnVtZXJpY3ZhciBpbnQxMjggdmFsIGJ5dGVz
OiAiKTsKKwl1bnNpZ25lZCBjaGFyKiBieXRlX2FycmF5ID0gKHZvaWQgKikgJnZhbDsKKwlmb3Ig
KGludCBpbmRleCA9IDA7IGluZGV4IDwgMTY7ICsraW5kZXgpCisJCWZwcmludGYoc3RkZXJyLCAi
JTAyWCIsICh1bnNpZ25lZClieXRlX2FycmF5W2luZGV4XSk7CisJZnByaW50ZihzdGRlcnIsICJc
biIpOworCiAJLyogaW50MTI4IGNhbiByZXF1aXJlIGF0IG1vc3QgMzkgZGVjaW1hbCBkaWdpdHM7
IGFkZCBvbmUgZm9yIHNhZmV0eSAqLwogCWFsbG9jX3Zhcih2YXIsIDQwIC8gREVDX0RJR0lUUyk7
CiAJaWYgKHZhbCA8IDApCg==
--=_aa6ec392b1565857f367e9a4a18844e9--