Re: Keep compiler silence (clang 10, implicit conversion from 'long' to 'double' )

Hello,

I add further information. This issue also has a problem about
*overflow checking*.

The original code is as follows.

src/backend/utils/adt/timestamp.c:3222
-----
if (result_double > PG_INT64_MAX || result_double < PG_INT64_MIN)
ereport(ERROR,
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("interval out of range")));
result->time = (int64) result_double;
-----

Here, the code checks whether "result_double" fits 64-bit integer size
before casting it.

However, as I have mentioned in the previous email, PG_INT64_MAX is
cast to double and the value becomes 9223372036854775808 due to lack
of precision.
Therefore, the above code is identical to "result_double >
9223372036854775808.0". This checking does not cover the case when
result_double is equal to 9223372036854775808. In this case, "(int64)
result_double" will be -9223372036854775808, which is wrong.

The next code confirms what I explained.

===
#include <stdio.h>
#include <stdint.h>
int main(void)
{
double value = (double) INT64_MAX;
printf("INT64_MAX = %ld\n", INT64_MAX);
printf("value = %lf\n", value);
printf("(value > (double) INT64_MAX) == %d\n", value > (double) INT64_MAX);
printf("(long int) value == %ld\n", (long int) value);
}
===
Output:
INT64_MAX = 9223372036854775807
value = 9223372036854775808.000000
(value > (double) INT64_MAX) == 0
(long int) value == -9223372036854775808
===

I think the code should be "result_double >= (double) PG_INT64_MAX",
that is we have to use >= rather than >. I attached the modified
patch.

Thanks,
Yuya Watari
NTT Software Innovation Center
watari(dot)yuya(at)gmail(dot)com

2019年9月27日(金) 12:00 Yuya Watari <watari(dot)yuya(at)gmail(dot)com>:
>
> Hello,
>
> I found the problem that clang compiler introduces warnings when building PostgreSQL. Attached patch fixes it.
>
> ===
> Compiler version
> ===
> clang version 10.0.0-svn372772-1~exp1+0~20190924181208.2504~1.gbpb209ff (trunk)
>
> Older versions of clang may not generate this warning.
>
> ===
> Warning
> ===
>
> timestamp.c:3236:22: warning: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-int-float-conversion]
> if (result_double > PG_INT64_MAX || result_double < PG_INT64_MIN)
> ~ ^~~~~~~~~~~~
> ../../../../src/include/c.h:444:22: note: expanded from macro 'PG_INT64_MAX'
> #define PG_INT64_MAX INT64CONST(0x7FFFFFFFFFFFFFFF)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../../../../src/include/c.h:381:25: note: expanded from macro 'INT64CONST'
> #define INT64CONST(x) (x##L)
> ^~~~
> <scratch space>:234:1: note: expanded from here
> 0x7FFFFFFFFFFFFFFFL
> ^~~~~~~~~~~~~~~~~~~
> 1 warning generated.
> pgbench.c:1657:30: warning: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-int-float-conversion]
> if (dval < PG_INT64_MIN || PG_INT64_MAX < dval)
> ^~~~~~~~~~~~ ~
> ../../../src/include/c.h:444:22: note: expanded from macro 'PG_INT64_MAX'
> #define PG_INT64_MAX INT64CONST(0x7FFFFFFFFFFFFFFF)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../../../src/include/c.h:381:25: note: expanded from macro 'INT64CONST'
> #define INT64CONST(x) (x##L)
> ^~~~
> <scratch space>:252:1: note: expanded from here
> 0x7FFFFFFFFFFFFFFFL
> ^~~~~~~~~~~~~~~~~~~
> 1 warning generated.
>
> ===
>
> This warning is due to implicit conversion from PG_INT64_MAX to double, which drops the precision as described in the warning. This drop is not a problem in this case, but we have to get rid of useless warnings. Attached patch casts PG_INT64_MAX explicitly.
>
> Thanks,
> Yuya Watari
> NTT Software Innovation Center
> watari(dot)yuya(at)gmail(dot)com