Thread: 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.yuya@gmail.com

2019年9月27日(金) 12:00 Yuya Watari <watari.yuya@gmail.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_MAXexplicitly. 
>
> Thanks,
> Yuya Watari
> NTT Software Innovation Center
> watari.yuya@gmail.com

Hello.

At Fri, 27 Sep 2019 16:43:53 +0900, Yuya Watari <watari.yuya@gmail.com> wrote in
<CAJ2pMkaLTOxFjTim=GV8u=jG++sb9W6GNSgyFxPVDSQMVfRv5g@mail.gmail.com>
> 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;
> -----
>
> 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.

Yeah, good catch! It is surely a bogus comparison.  But I suspect
that how a number is rounded off depends on architechture. (But
not sure.)  There seems be a variant of (double/float)->int32,
say, in interval_div.

I found a trick seems workable generically (*1). (2.0 *
(PG_INT64_MAX/2 + 1)) will generate the value next to the
PG_INT64_MAX based on some assumptions
(*1). IS_DOUBLE_SAFE_IN_INT64() below would be able to check if
the value can be converted into int64 safely or not. 

====
/*
 * Check if a double value can be casted into int64.
 *
 * This macro is assuming that FLT_RADIX == 2 so that the * 2.0 trick works,
 * PG_INT64_MAX is so below DBL_MAX that the doubled value can be represented
 * in double and DBL_MANT_DIG is equal or smaller than DBL_MAX_EXP so that
 * ceil() returns expected result.
*/
#define MIN_DOUBLE_OVER_INT64_MAX (2.0 * (PG_INT64_MAX / 2 + 1))
#define MAX_DOUBLE_UNDER_INT64_MIN (2.0 * (PG_INT64_MIN / 2 - 1))

#if -PG_INT64_MAX != PG_INT64_MIN
#define IS_DOUBLE_SAFE_IN_INT64(x)          \
  ((x) < MIN_DOUBLE_OVER_INT64_MAX && ceil(x) >= PG_INT64_MIN)
#else
#define IS_DOUBLE_SAFE_IN_INT64(x)                \
  ((x) < MIN_DOUBLE_OVER_INT64_MAX && (x) > MAX_DOUBLE_UNDER_INT64_MIN)
#endif
====

I haven't fully confirmed if it is really right.

*1: https://stackoverflow.com/questions/526070/handling-overflow-when-casting-doubles-to-integers-in-c

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Horiguchi-san,

On Tue, Oct 1, 2019 at 3:41 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> I found a trick seems workable generically (*1). (2.0 *
> (PG_INT64_MAX/2 + 1)) will generate the value next to the
> PG_INT64_MAX based on some assumptions
> (*1). IS_DOUBLE_SAFE_IN_INT64() below would be able to check if
> the value can be converted into int64 safely or not.

Thanks for sharing a nice way of checking overflow. I tested your
IS_DOUBLE_SAFE_IN_INT64() macro in my environment by the simple code
(attached to this email) and confirmed that it appropriately handled
the overflow. However, further consideration is needed for different
architectures.

I attached the modified patch. In the patch, I placed the macro in
"src/include/c.h", but this may not be a good choice because c.h is
widely included from a lot of files. Do you have any good ideas about
its placement?

Thanks,
Yuya Watari
NTT Software Innovation Center
watari.yuya@gmail.com

Yuya Watari <watari.yuya@gmail.com> writes:
> I attached the modified patch. In the patch, I placed the macro in
> "src/include/c.h", but this may not be a good choice because c.h is
> widely included from a lot of files. Do you have any good ideas about
> its placement?

I agree that there's an actual bug here; it can be demonstrated with

# select extract(epoch from '256 microseconds'::interval * (2^55)::float8);
     date_part      
--------------------
 -9223372036854.775
(1 row)

which clearly is a wrong answer.

I do not however like any of the proposed patches.  We already have one
place that deals with this problem correctly, in int8.c's dtoi8():

    /*
     * Range check.  We must be careful here that the boundary values are
     * expressed exactly in the float domain.  We expect PG_INT64_MIN to be an
     * exact power of 2, so it will be represented exactly; but PG_INT64_MAX
     * isn't, and might get rounded off, so avoid using it.
     */
    if (unlikely(num < (float8) PG_INT64_MIN ||
                 num >= -((float8) PG_INT64_MIN) ||
                 isnan(num)))
        ereport(ERROR,
                (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
                 errmsg("bigint out of range")));

We should adopt that coding technique not invent new ones.

I do concur with creating a macro that encapsulates a correct version
of this test, maybe like

#define DOUBLE_FITS_IN_INT64(num) \
    ((num) >= (double) PG_INT64_MIN && \
     (num) < -((double) PG_INT64_MIN))

(or s/double/float8/ ?)

c.h is probably a reasonable place, seeing that we define the constants
there.

            regards, tom lane

At Mon, 04 Nov 2019 12:53:48 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> Yuya Watari <watari.yuya@gmail.com> writes:
> > I attached the modified patch. In the patch, I placed the macro in
> > "src/include/c.h", but this may not be a good choice because c.h is
> > widely included from a lot of files. Do you have any good ideas about
> > its placement?
> 
> I agree that there's an actual bug here; it can be demonstrated with
> 
> # select extract(epoch from '256 microseconds'::interval * (2^55)::float8);
>      date_part      
> --------------------
>  -9223372036854.775
> (1 row)
> 
> which clearly is a wrong answer.
> 
> I do not however like any of the proposed patches.  We already have one
> place that deals with this problem correctly, in int8.c's dtoi8():
> 
>     /*
>      * Range check.  We must be careful here that the boundary values are
>      * expressed exactly in the float domain.  We expect PG_INT64_MIN to be an
>      * exact power of 2, so it will be represented exactly; but PG_INT64_MAX
>      * isn't, and might get rounded off, so avoid using it.
>      */
>     if (unlikely(num < (float8) PG_INT64_MIN ||
>                  num >= -((float8) PG_INT64_MIN) ||
>                  isnan(num)))
>         ereport(ERROR,
>                 (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
>                  errmsg("bigint out of range")));
> 
> We should adopt that coding technique not invent new ones.
> 
> I do concur with creating a macro that encapsulates a correct version
> of this test, maybe like
> 
> #define DOUBLE_FITS_IN_INT64(num) \
>     ((num) >= (double) PG_INT64_MIN && \
>      (num) < -((double) PG_INT64_MIN))

# I didn't noticed the existing bit above.

Agreed. it is equivalent to the trick AFAICS thus no need to add
another one to warry with.

> (or s/double/float8/ ?)

Maybe.

> c.h is probably a reasonable place, seeing that we define the constants
> there.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello Tom and Horiguchi-san,

On Tue, Nov 5, 2019 at 1:59 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> At Mon, 04 Nov 2019 12:53:48 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in
> > I do concur with creating a macro that encapsulates a correct version
> > of this test, maybe like
> >
> > #define DOUBLE_FITS_IN_INT64(num) \
> >       ((num) >= (double) PG_INT64_MIN && \
> >        (num) < -((double) PG_INT64_MIN))

Thank you for your comments. The proposed macro "DOUBLE_FITS_IN_INT64"
is a good and simple way to check the overflow. According to that, I
revised the patch, which includes regression tests.

In the patch, I additionally modified other occurrences as follows.

=========

+#define FLOAT8_FITS_IN_INT32(num) \
+   ((num) >= (float8) PG_INT32_MIN && (num) < -((float8) PG_INT32_MIN))

=========

-   if (unlikely(num < (float8) PG_INT32_MIN ||
-                num >= -((float8) PG_INT32_MIN) ||
-                isnan(num)))
+   /* Range check */
+   if (unlikely(!FLOAT8_FITS_IN_INT32(num)))

=========

The added macro FLOAT8_FITS_IN_INT32() does not check NaN explicitly,
but it sufficiently handles the case.

Best regards,
Yuya Watari
NTT Software Innovation Center
watari.yuya@gmail.com

Yuya Watari <watari.yuya@gmail.com> writes:
> The added macro FLOAT8_FITS_IN_INT32() does not check NaN explicitly,
> but it sufficiently handles the case.

Really?  I don't think anything is guaranteed about how a NaN will
compare when using C's non-NaN-aware comparison operators.

My thought about this was to annotate the macros with a reminder
to also check for NaN if there's any possibility that the value
is NaN.

            regards, tom lane

Hello Tom,

Thank you for replying.

On Wed, Nov 6, 2019 at 12:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Yuya Watari <watari.yuya@gmail.com> writes:
> > The added macro FLOAT8_FITS_IN_INT32() does not check NaN explicitly,
> > but it sufficiently handles the case.
>
> Really?  I don't think anything is guaranteed about how a NaN will
> compare when using C's non-NaN-aware comparison operators.
>
> My thought about this was to annotate the macros with a reminder
> to also check for NaN if there's any possibility that the value
> is NaN.

I agree with your opinion. Thank you for pointing it out.

If the platform satisfies IEEE-754 standard, all comparisons (except
for "not equals") between NaN and other floating values are "false".
[1] In this case, the proposed FLOAT8_FITS_IN_INT32() macro handles
NaN.

[1] https://en.wikipedia.org/wiki/NaN#Comparison_with_NaN

However, this behavior depends on the platform architecture. As you
have said, C language does not always follow IEEE-754. I think adding
explicit checking of NaN is necessary.

I modified the patch and attached it.

Best regards,
Yuya Watari
NTT Software Innovation Center
watari.yuya@gmail.com

On Wed, Nov 6, 2019 at 3:33 PM Yuya Watari <watari.yuya@gmail.com> wrote:
> However, this behavior depends on the platform architecture. As you
> have said, C language does not always follow IEEE-754. I think adding
> explicit checking of NaN is necessary.

I'm curious about this point.  C may not require IEEE 754 (for
example, on current IBM mainframe and POWER hardware you can opt for
IBM hex floats, and on some IBM platforms that is the default, and the
C compiler isn't breaking any rules by doing that; the only other
floating point format I've heard of is VAX format, long gone, but
perhaps allowed by C).  But PostgreSQL effectively requires IEEE 754
since commit 02ddd499322ab6f2f0d58692955dc9633c2150fc, right?

Thomas Munro <thomas.munro@gmail.com> writes:
> On Wed, Nov 6, 2019 at 3:33 PM Yuya Watari <watari.yuya@gmail.com> wrote:
>> However, this behavior depends on the platform architecture. As you
>> have said, C language does not always follow IEEE-754. I think adding
>> explicit checking of NaN is necessary.

> I'm curious about this point.  C may not require IEEE 754 (for
> example, on current IBM mainframe and POWER hardware you can opt for
> IBM hex floats, and on some IBM platforms that is the default, and the
> C compiler isn't breaking any rules by doing that; the only other
> floating point format I've heard of is VAX format, long gone, but
> perhaps allowed by C).  But PostgreSQL effectively requires IEEE 754
> since commit 02ddd499322ab6f2f0d58692955dc9633c2150fc, right?

That commit presumes that floats follow the IEEE bitwise representation,
I think; but it's a long way from there to assuming that float comparisons
do something that is explicitly *not* promised by C99.  The C spec goes no
further than to state that comparisons on NaNs might raise an exception,
and that's already bad enough.  I believe that the assumption Yuya-san was
making about "comparisons on NaNs return false" is only guaranteed by C99
if you use the new-in-C99 macros isless(x, y) and so on, not if you write
x < y.

There's a separate discussion to be had here about whether
    !isnan(x) && !isnan(y) && x < y
is more or less efficient, or portable, than
    isless(x, y)
but I'm not really in any hurry to start using the latter macros.

            regards, tom lane

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 >> But PostgreSQL effectively requires IEEE 754 since commit
 >> 02ddd499322ab6f2f0d58692955dc9633c2150fc, right?

 Tom> That commit presumes that floats follow the IEEE bitwise
 Tom> representation, I think;

Correct. (It notably does _not_ make any assumptions about how floating
point arithmetic or comparisons work - all the computation is done in
integers.)

 Tom> but it's a long way from there to assuming that float comparisons
 Tom> do something that is explicitly *not* promised by C99.

I agree.

-- 
Andrew (irc:RhodiumToad)

Hello Tom, Thomas, and Andrew,

>  Tom> That commit presumes that floats follow the IEEE bitwise
>  Tom> representation, I think;
>
> Correct. (It notably does _not_ make any assumptions about how floating
> point arithmetic or comparisons work - all the computation is done in
> integers.)
>
>  Tom> but it's a long way from there to assuming that float comparisons
>  Tom> do something that is explicitly *not* promised by C99.
>
> I agree.

Thank you for your comments. I agree that we should not assume
anything that is not guaranteed in the language specification. The
modified patch (attached in the previous e-mail) checks NaN explicitly
if needed.

Best regards,
Yuya Watari
NTT Software Innovation Center
watari.yuya@gmail.com

At Wed, 6 Nov 2019 13:56:46 +0900, Yuya Watari <watari.yuya@gmail.com> wrote in 
> Hello Tom, Thomas, and Andrew,
> 
> >  Tom> That commit presumes that floats follow the IEEE bitwise
> >  Tom> representation, I think;
> >
> > Correct. (It notably does _not_ make any assumptions about how floating
> > point arithmetic or comparisons work - all the computation is done in
> > integers.)
> >
> >  Tom> but it's a long way from there to assuming that float comparisons
> >  Tom> do something that is explicitly *not* promised by C99.
> >
> > I agree.
> 
> Thank you for your comments. I agree that we should not assume
> anything that is not guaranteed in the language specification. The
> modified patch (attached in the previous e-mail) checks NaN explicitly
> if needed.

Mmm? See the bit in the patch cited below (v5).

+    /* Range check */
+    if (unlikely(!FLOAT8_FITS_IN_INT32(num)) || isnan(num))

If compiler doesn't any fancy, num is fed to an arithmetic before
checking if it is NaN. That seems have a chance of exception.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hello Horiguchi-san,

On Thu, Nov 7, 2019 at 3:10 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> Mmm? See the bit in the patch cited below (v5).
>
> +       /* Range check */
> +       if (unlikely(!FLOAT8_FITS_IN_INT32(num)) || isnan(num))
>
> If compiler doesn't any fancy, num is fed to an arithmetic before
> checking if it is NaN. That seems have a chance of exception.

Thank you for pointing it out. That's my mistake. I fixed it and
attached the patch.

Best regards,
Yuya Watari
NTT Software Innovation Center
watari.yuya@gmail.com

Yuya Watari <watari.yuya@gmail.com> writes:
> On Thu, Nov 7, 2019 at 3:10 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
>> +       if (unlikely(!FLOAT8_FITS_IN_INT32(num)) || isnan(num))
>> If compiler doesn't any fancy, num is fed to an arithmetic before
>> checking if it is NaN. That seems have a chance of exception.

> Thank you for pointing it out. That's my mistake. I fixed it and
> attached the patch.

Actually, that mistake is very old --- the existing functions tested
isnan() last for a long time.  I agree that testing isnan() first
is safer, but it seems that the behavior of throwing an exception
for comparisons on NaN is rarer than one might guess from the C spec.

Another issue in the patch as it stands is that the FITS_IN_ macros
require the input to have already been rounded with rint(), else they'll
give the wrong answer for values just a bit smaller than -PG_INTnn_MIN.
The existing uses of the technique did that, and interval_mul already
did too, but I had to adjust pgbench.  This is largely a documentation
failure: not only did you fail to add any commentary about the new macros,
but you removed most of the commentary that had been in-line in the
existing usages.

I fixed those things and pushed it.

            regards, tom lane

Hello Tom,

Thank you for your comments.

On Fri, Nov 8, 2019 at 1:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> failure: not only did you fail to add any commentary about the new macros,
> but you removed most of the commentary that had been in-line in the
> existing usages.

I apologize for the insufficient comments. I had to add more
information about these macros.

> I fixed those things and pushed it.

Thank you very much for the commit!

Best regards,
Yuya Watari
NTT Software Innovation Center
watari.yuya@gmail.com