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>
Cc: pgsql-hackers@postgresql.org, Alvaro Herrera <alvherre@alvh.no-ip.org>
Subject: Re: master make check fails on Solaris 10
Date: Wed, 17 Jan 2018 18:12:44 +0300
Message-ID: <4b5a7c377a6f7443f149049849e7bc77@postgrespro.ru> (raw)
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>
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
Attachments:diff.patch (text/x-diff, 2.9K) downloaddiff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.cindex 5b34bad..64d131f 100644--- a/src/backend/utils/adt/numeric.c+++ b/src/backend/utils/adt/numeric.c@@ -3953,6 +3953,8 @@ makeInt128AggState(FunctionCallInfo fcinfo, bool calcSumX2)
MemoryContext agg_context;
MemoryContext old_context;
+ fprintf(stderr, "makeInt128AggState\n");+
if (!AggCheckCallContext(fcinfo, &agg_context))
elog(ERROR, "aggregate function called in non-aggregate context");
@@ -3987,6 +3989,18 @@ makeInt128AggStateCurrentContext(bool calcSumX2)
static void
do_int128_accum(Int128AggState *state, int128 newval)
{
+ fprintf(stderr, "do_int128_accum int128 newval as 2 x int64: %ld %ld\n",+ (int64) (newval >> 64), (int64) newval);++ fprintf(stderr, "do_int128_accum int128 newval bytes: ");+ unsigned char* byte_array = (void *) &newval;+ for (int index = 0; index < 16; ++index)+ fprintf(stderr, "%02X", (unsigned)byte_array[index]);+ fprintf(stderr, "\n");++ fprintf(stderr, "do_int128_accum state %p\n", state);+ fprintf(stderr, "do_int128_accum %p\n", &do_int128_accum);+
if (state->calcSumX2)
state->sumX2 += newval * newval;
@@ -4307,10 +4321,27 @@ int8_avg_accum(PG_FUNCTION_ARGS)
if (state == NULL)
state = makePolyNumAggState(fcinfo, false);
+ int64 val = PG_GETARG_INT64(1);+ fprintf(stderr, "int8_avg_accum val %ld\n", val);++ int128 val_int128 = (int128) val;++ fprintf(stderr, "int8_avg_accum val_int128 as 2 x int64: %ld %ld\n",+ (int64) (val_int128 >> 64), (int64) val_int128);++ fprintf(stderr, "int8_avg_accum val_int128 bytes: ");+ unsigned char* byte_array = (void *) &val_int128;+ for (int index = 0; index < 16; ++index)+ fprintf(stderr, "%02X", (unsigned)byte_array[index]);+ fprintf(stderr, "\n");++ fprintf(stderr, "int8_avg_accum state %p\n", state);+ fprintf(stderr, "int8_avg_accum %p\n", &int8_avg_accum);+
if (!PG_ARGISNULL(1))
{
#ifdef HAVE_INT128
- do_int128_accum(state, (int128) PG_GETARG_INT64(1));+ do_int128_accum(state, val_int128);
#else
Numeric newval;
@@ -4640,6 +4671,7 @@ numeric_poly_sum(PG_FUNCTION_ARGS)
Datum
numeric_poly_avg(PG_FUNCTION_ARGS)
{
+ fprintf(stderr, "numeric_poly_avg\n");
#ifdef HAVE_INT128
PolyNumAggState *state;
NumericVar result;
@@ -6365,6 +6397,17 @@ int128_to_numericvar(int128 val, NumericVar *var)
NumericDigit *ptr;
int ndigits;
+ fprintf(stderr, "int128_to_numericvar\n");++ fprintf(stderr, "int128_to_numericvar int128 val as 2 x int64: %ld %ld\n",+ (int64) (val >> 64), (int64) val);++ fprintf(stderr, "int128_to_numericvar int128 val bytes: ");+ unsigned char* byte_array = (void *) &val;+ for (int index = 0; index < 16; ++index)+ fprintf(stderr, "%02X", (unsigned)byte_array[index]);+ fprintf(stderr, "\n");+
/* int128 can require at most 39 decimal digits; add one for safety */
alloc_var(var, 40 / DEC_DIGITS);
if (val < 0)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: m.polyakova@postgrespro.ru, tgl@sss.pgh.pa.us, andres@anarazel.de, alvherre@alvh.no-ip.org
Subject: Re: master make check fails on Solaris 10
In-Reply-To: <4b5a7c377a6f7443f149049849e7bc77@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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