MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="=_e49dd01442acd7e2372acb978fd637a5"
Date: Wed, 17 Jan 2018 18:02:26 +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: <18209.1516059711@sss.pgh.pa.us>
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>
Message-ID: <90ab676392c8f9c84431976147097cf0@postgrespro.ru>
User-Agent: Roundcube Webmail/1.2.4
Precedence: bulk

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

[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
--=_e49dd01442acd7e2372acb978fd637a5
Content-Transfer-Encoding: base64
Content-Type: text/x-c;
 name=int128test2.c
Content-Disposition: attachment;
 filename=int128test2.c;
 size=2270

I2luY2x1ZGUgPHN0ZGRlZi5oPg0KI2luY2x1ZGUgPHN0ZGlvLmg+DQoNCg0KI2lmbmRlZiBQR19B
TElHTl8xMjgNCiNkZWZpbmUgUEdfQUxJR05fMTI4IDgNCiNlbmRpZg0KDQovKiBHQ0MsIFN1bnBy
byBhbmQgWExDIHN1cHBvcnQgYWxpZ25lZCAqLw0KI2lmIGRlZmluZWQoX19HTlVDX18pIHx8IGRl
ZmluZWQoX19TVU5QUk9fQykgfHwgZGVmaW5lZChfX0lCTUNfXykNCiNkZWZpbmUgcGdfYXR0cmli
dXRlX2FsaWduZWQoYSkgX19hdHRyaWJ1dGVfXygoYWxpZ25lZChhKSkpDQojZW5kaWYNCg0KdHlw
ZWRlZiBfX2ludDEyOCBpbnQxMjhhDQojaWYgZGVmaW5lZChwZ19hdHRyaWJ1dGVfYWxpZ25lZCkN
CnBnX2F0dHJpYnV0ZV9hbGlnbmVkKFBHX0FMSUdOXzEyOCkNCiNlbmRpZg0KOw0KDQovKg0KICog
VGhlc2UgYXJlIGdsb2JhbHMgdG8gZGlzY291cmFnZSB0aGUgY29tcGlsZXIgZnJvbSBmb2xkaW5n
IGFsbCB0aGUNCiAqIGFyaXRobWV0aWMgdGVzdHMgZG93biB0byBjb21waWxlLXRpbWUgY29uc3Rh
bnRzLiAgV2UgZG8gbm90IGhhdmUNCiAqIGNvbnZlbmllbnQgc3VwcG9ydCBmb3IgMTI4Yml0IGxp
dGVyYWxzIGF0IHRoaXMgcG9pbnQuLi4NCiAqLw0Kc3RydWN0IGdsb2IxMjgNCnsNCglfX2ludDEy
OAlzdGFydDsNCgljaGFyCQlwYWQ7DQoJaW50MTI4YQkJYTsNCglpbnQxMjhhCQliOw0KCWludDEy
OGEJCWM7DQoJaW50MTI4YQkJZDsNCn0gZyA9IHswLCAncCcsIDQ4ODI4MTI1LCA5NzY1NjI1MCwg
MCwgMH07DQoNCmludDEyOGEgaG9sZGVyOw0KDQp2b2lkIHBhc3NfYnlfdmFsKGNoYXIgKmJ1ZmZl
ciwgaW50MTI4YSBwYXIpIHsNCiAgIGhvbGRlciA9IHBhcjsNCn0NCg0KaW50DQptYWluKCkNCnsN
CgljaGFyICptZXNzYWdlID0gIkV2ZXJ5dGhpbmcgaXMgZmluZSI7DQoJc2hvcnQgaW50IGkxNiA9
IDMxMDAwOw0KCXVuc2lnbmVkIHNob3J0IGludCB1MTYgPSA0OTAwMDsNCglpbnQgaTMyID0gNDg4
MjgxMjU7DQoJbG9uZyBpbnQgaTY0ID0gOTc2NTYyMjVMIDw8IDEyOw0KCWludDEyOGEgcTsNCglp
bnQgZmFpbHVyZXMgPSAwOw0KDQoJLyogQmFzaWMgYXJpdGhtZXRpYyB0ZXN0cyAqLw0KDQoJZy5h
ID0gKGcuYSA8PCAxMikgKyAxOwkJLyogMjAwMDAwMDAwMDAxICovDQoJZy5iID0gKGcuYiA8PCAx
MikgKyA1OwkJLyogNDAwMDAwMDAwMDA1ICovDQoJLyogdXNlIHRoZSBtb3N0IHJlbGV2YW50IGFy
aXRobWV0aWMgb3BzICovDQoJZy5jID0gZy5hICogZy5iOw0KCWcuZCA9IChnLmMgKyBnLmIpIC8g
Zy5iOw0KCS8qIHJldHVybiBkaWZmZXJlbnQgdmFsdWVzLCB0byBwcmV2ZW50IG9wdGltaXphdGlv
bnMgKi8NCglpZiAoZy5kICE9IGcuYSArIDEpDQoJew0KCQlwcmludGYoIndyb25nIGFyaXRobWV0
aWMgcmVzdWx0XG4iKTsNCgkJZmFpbHVyZXMrKzsNCgl9DQoJZWxzZQ0KCXsNCgkJcHJpbnRmKCJi
YXNpYyBhcml0bWV0aWMgT0tcbiIpOw0KCX0NCg0KCXBhc3NfYnlfdmFsKG1lc3NhZ2UsIChpbnQx
MjhhKSBpMTYpOw0KCXEgPSAoaW50MTI4YSkgaTE2Ow0KCXByaW50ZigicGFzcyBpbnQgMTYgJXNc
biIsIChxID09IGhvbGRlcikgPyAiT0siIDogIkZBSUxFRCIpOw0KCWlmIChxICE9IGhvbGRlcikN
CgkJZmFpbHVyZXMrKzsNCg0KCXBhc3NfYnlfdmFsKG1lc3NhZ2UsIChpbnQxMjhhKSB1MTYpOw0K
CXEgPSAoaW50MTI4YSkgdTE2Ow0KCXByaW50ZigicGFzcyB1aW50IDE2ICVzXG4iLCAocSA9PSBo
b2xkZXIpID8gIk9LIiA6ICJGQUlMRUQiKTsNCglpZiAocSAhPSBob2xkZXIpDQoJCWZhaWx1cmVz
Kys7DQoNCglwYXNzX2J5X3ZhbChtZXNzYWdlLCAoaW50MTI4YSkgaTMyKTsNCglxID0gKGludDEy
OGEpIGkzMjsNCglwcmludGYoInBhc3MgaW50IDMyICVzXG4iLCAocSA9PSBob2xkZXIpID8gIk9L
IiA6ICJGQUlMRUQiKTsNCglpZiAocSAhPSBob2xkZXIpDQoJCWZhaWx1cmVzKys7DQoNCglwYXNz
X2J5X3ZhbChtZXNzYWdlLCAoaW50MTI4YSkgaTY0KTsNCglxID0gKGludDEyOGEpIGk2NDsNCglw
cmludGYoInBhc3MgaW50IDY0ICVzXG4iLCAocSA9PSBob2xkZXIpID8gIk9LIiA6ICJGQUlMRUQi
KTsNCglpZiAocSAhPSBob2xkZXIpDQoJCWZhaWx1cmVzKys7DQoNCglxID0gKGludDEyOGEpIChp
NjQgPDwgMzMpOw0KCXBhc3NfYnlfdmFsKG1lc3NhZ2UsIHEpOw0KCXByaW50ZigicGFzcyBpbnQg
MTI4ICVzXG4iLCAocSA9PSBob2xkZXIpID8gIk9LIiA6ICJGQUlMRUQiKTsNCglpZiAocSAhPSBo
b2xkZXIpDQoJCWZhaWx1cmVzKys7DQoNCglyZXR1cm4gZmFpbHVyZXM7DQp9DQo=
--=_e49dd01442acd7e2372acb978fd637a5--