Horton

Thread

Commits

  1. Error out for clang on x86-32 without SSE2 support, no -fexcess-precision.

  2. Fix 8a934d677 for libc++ and make more include order resistant.

  1. Bug fix for glibc broke freebsd build in REL_11_STABLE

    Victor Wagner <vitus@wagner.pp.ru> — 2018-09-04T13:16:24Z

    Collegues,

Few days ago commit 

 1f349aa7d9a6633e87db071390c73a39ac279ba4

Fix 8a934d67 for libc++ and make more include order resistant. 

was introduced into branch REL_11_STABLE.
It seems that it deals with GLIBC-based systems (i.e Linux and Hurd)
only, and shouldn't affect systems with non-GNU libc (such as BSD
family).

It changes code  which has a comment:

/*
 * Glibc doesn't use the builtin for clang due to a *gcc* bug in a
version
 * newer than the gcc compatibility clang claims to have. This would
cause a
 * *lot* of superfluous function calls, therefore revert when using
clang. In
 * C++ there's issues with libc++ (not libstdc++), so disable as well.
 */


However, this commit broke float8 test on 32-bit FreeBSD 11 with clang
3.8.0 compiler. Regressions.diff follows:

================== pgsql.build/src/test/regress/regression.diffs
===================
*** /usr/home/buildfarm/build-farm/REL_11_STABLE/pgsql.build/src/test/regress/expected/float8.out
Tue Sep  4 11:57:47 2018
--- /usr/home/buildfarm/build-farm/REL_11_STABLE/pgsql.build/src/test/regress/results/float8.out
Tue Sep  4 12:03:28 2018 *************** *** 418,424 **** SET f1 =
FLOAT8_TBL.f1 * '-1' WHERE FLOAT8_TBL.f1 > '0.0';
  SELECT '' AS bad, f.f1 * '1e200' from FLOAT8_TBL f;
! ERROR:  value out of range: overflow
  SELECT '' AS bad, f.f1 ^ '1e200' from FLOAT8_TBL f;
  ERROR:  value out of range: overflow
  SELECT 0 ^ 0 + 0 ^ 1 + 0 ^ 0.0 + 0 ^ 0.5;
--- 418,432 ----
     SET f1 = FLOAT8_TBL.f1 * '-1'
     WHERE FLOAT8_TBL.f1 > '0.0';
  SELECT '' AS bad, f.f1 * '1e200' from FLOAT8_TBL f;
!  bad |     ?column?     
! -----+------------------
!      |                0
!      |      -3.484e+201
!      |     -1.0043e+203
!      |        -Infinity
!      | -1.2345678901234
! (5 rows)
! 
  SELECT '' AS bad, f.f1 ^ '1e200' from FLOAT8_TBL f;
  ERROR:  value out of range: overflow
  SELECT 0 ^ 0 + 0 ^ 1 + 0 ^ 0.0 + 0 ^ 0.5; 
================================================================
Problem doesn't arise on 64-bit FreeBSD and MacOS. (we have no 32-bit
MacOs at hand).

Following patch fixes this problem:

diff --git a/src/include/port.h b/src/include/port.h
index 0ce72e50e5..7daaebf568 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -350,7 +350,7 @@ extern int  isinf(double x);
  * *lot* of superfluous function calls, therefore revert when using
clang. In
  * C++ there's issues with libc++ (not libstdc++), so disable as well.
  */
-#if defined(__clang__) && !defined(__cplusplus)
+#if defined(__clang__) && !defined(__cplusplus) && defined(__linux__)
 /* needs to be separate to not confuse other compilers */
 #if __has_builtin(__builtin_isinf)
 /* need to include before, to avoid getting overwritten */

Idea of the patch is that because patch is only GLIBC-related, we
should apply this logic only if we have linux (more precisely linux or
hurd, but adding hurd would make condition overcomplicated and no one
seems to use it) system.
--

  2. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andres Freund <andres@anarazel.de> — 2018-09-04T13:59:07Z

    
On September 4, 2018 6:16:24 AM PDT, Victor Wagner <vitus@wagner.pp.ru> wrote:
>
>Collegues,
>
>Few days ago commit 
>
> 1f349aa7d9a6633e87db071390c73a39ac279ba4
>
>Fix 8a934d67 for libc++ and make more include order resistant. 
>
>was introduced into branch REL_11_STABLE.
>It seems that it deals with GLIBC-based systems (i.e Linux and Hurd)
>only, and shouldn't affect systems with non-GNU libc (such as BSD
>family).

What libc is this using?


>It changes code  which has a comment:
>
>/*
> * Glibc doesn't use the builtin for clang due to a *gcc* bug in a
>version
> * newer than the gcc compatibility clang claims to have. This would
>cause a
> * *lot* of superfluous function calls, therefore revert when using
>clang. In
> * C++ there's issues with libc++ (not libstdc++), so disable as well.
> */
>
>
>However, this commit broke float8 test on 32-bit FreeBSD 11 with clang
>3.8.0 compiler. Regressions.diff follows:

Does this happen with a newer clang version too?

>================== pgsql.build/src/test/regress/regression.diffs
>===================
>***
>/usr/home/buildfarm/build-farm/REL_11_STABLE/pgsql.build/src/test/regress/expected/float8.out
>Tue Sep  4 11:57:47 2018
>---
>/usr/home/buildfarm/build-farm/REL_11_STABLE/pgsql.build/src/test/regress/results/float8.out
>Tue Sep  4 12:03:28 2018 *************** *** 418,424 **** SET f1 =
>FLOAT8_TBL.f1 * '-1' WHERE FLOAT8_TBL.f1 > '0.0';
>  SELECT '' AS bad, f.f1 * '1e200' from FLOAT8_TBL f;
>! ERROR:  value out of range: overflow
>  SELECT '' AS bad, f.f1 ^ '1e200' from FLOAT8_TBL f;
>  ERROR:  value out of range: overflow
>  SELECT 0 ^ 0 + 0 ^ 1 + 0 ^ 0.0 + 0 ^ 0.5;
>--- 418,432 ----
>     SET f1 = FLOAT8_TBL.f1 * '-1'
>     WHERE FLOAT8_TBL.f1 > '0.0';
>  SELECT '' AS bad, f.f1 * '1e200' from FLOAT8_TBL f;
>!  bad |     ?column?     
>! -----+------------------
>!      |                0
>!      |      -3.484e+201
>!      |     -1.0043e+203
>!      |        -Infinity
>!      | -1.2345678901234
>! (5 rows)
>! 
>  SELECT '' AS bad, f.f1 ^ '1e200' from FLOAT8_TBL f;
>  ERROR:  value out of range: overflow
>  SELECT 0 ^ 0 + 0 ^ 1 + 0 ^ 0.0 + 0 ^ 0.5; 
>================================================================
>Problem doesn't arise on 64-bit FreeBSD and MacOS. (we have no 32-bit
>MacOs at hand).

This seems like something that needs to be understood.  If a compiler intrinsic doesn't work, it's serious.


>Following patch fixes this problem:
>
>diff --git a/src/include/port.h b/src/include/port.h
>index 0ce72e50e5..7daaebf568 100644
>--- a/src/include/port.h
>+++ b/src/include/port.h
>@@ -350,7 +350,7 @@ extern int  isinf(double x);
>  * *lot* of superfluous function calls, therefore revert when using
>clang. In
>  * C++ there's issues with libc++ (not libstdc++), so disable as well.
>  */
>-#if defined(__clang__) && !defined(__cplusplus)
>+#if defined(__clang__) && !defined(__cplusplus) && defined(__linux__)
> /* needs to be separate to not confuse other compilers */
> #if __has_builtin(__builtin_isinf)
> /* need to include before, to avoid getting overwritten */
>
>Idea of the patch is that because patch is only GLIBC-related, we
>should apply this logic only if we have linux (more precisely linux or
>hurd, but adding hurd would make condition overcomplicated and no one
>seems to use it) system.

This seems too restrictive. There's nothing Linux specific in the issue - causes slowdowns on other platforms too. 

Thanks!

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  3. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-04T16:38:50Z

    >>>>> "Andres" == Andres Freund <andres@anarazel.de> writes:

 >> However, this commit broke float8 test on 32-bit FreeBSD 11 with
 >> clang 3.8.0 compiler. Regressions.diff follows:

 Andres> Does this happen with a newer clang version too?

float8 test (and all other tests) passes for me on clang 3.9.1 on fbsd11
on 32-bit ARM, and on -m32 builds on amd64.

I also confirmed that without #define isinf(x) __builtin_isinf(x), on
both 32bit and 64bit fbsd isinf() compiles as a function call, so the
OP's proposed change would not be desirable.

-- 
Andrew (irc:RhodiumToad)

  4. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Thomas Munro <thomas.munro@enterprisedb.com> — 2018-09-04T18:06:56Z

    On Tue, Sep 4, 2018 at 9:39 AM Andrew Gierth
<andrew@tao11.riddles.org.uk> wrote:
>
> >>>>> "Andres" == Andres Freund <andres@anarazel.de> writes:
>
>  >> However, this commit broke float8 test on 32-bit FreeBSD 11 with
>  >> clang 3.8.0 compiler. Regressions.diff follows:
>
>  Andres> Does this happen with a newer clang version too?
>
> float8 test (and all other tests) passes for me on clang 3.9.1 on fbsd11
> on 32-bit ARM, and on -m32 builds on amd64.
>
> I also confirmed that without #define isinf(x) __builtin_isinf(x), on
> both 32bit and 64bit fbsd isinf() compiles as a function call, so the
> OP's proposed change would not be desirable.

I installed FreeBSD 11.2 i386 on a virtual machine.  I couldn't
reproduce the problem with either the base cc (clang 6.0.0) or clang38
(clang 3.8.1) installed via pkg.

The OP reported clang 3.8.0, so a minor version behind what I tested.

I did learn that "make check" fails in rolenames if your Unix user is
called "user".

-- 
Thomas Munro
http://www.enterprisedb.com

  5. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-04T19:48:24Z

    Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Tue, Sep 4, 2018 at 9:39 AM Andrew Gierth
> <andrew@tao11.riddles.org.uk> wrote:
>> I also confirmed that without #define isinf(x) __builtin_isinf(x), on
>> both 32bit and 64bit fbsd isinf() compiles as a function call, so the
>> OP's proposed change would not be desirable.

Presumably what we are looking at here is a compiler codegen bug for
__builtin_isinf().  The proposed change dodges it precisely by
substituting the library function instead --- at a performance penalty.
I agree that this isn't a real desirable fix, and we definitely ought
not cause it to happen on platforms that haven't got the bug.

> I installed FreeBSD 11.2 i386 on a virtual machine.  I couldn't
> reproduce the problem with either the base cc (clang 6.0.0) or clang38
> (clang 3.8.1) installed via pkg.
> The OP reported clang 3.8.0, so a minor version behind what I tested.

I tried to reproduce the problem, without success, on a few different
FreeBSD images I had laying about:

FreeBSD 11.0/x86_64, clang version 3.8.0
	(this confirms OP's report that x86_64 is OK)

FreeBSD 10.3/ppc, gcc 4.2.1

FreeBSD 12.0-CURRENT (from around mid-May)/arm64, clang version 6.0.0

(I was testing PG HEAD, not the 11 branch, but I don't see a reason
to think that'd make a difference.)

I also looked for evidence of a bug of this sort in the clang bugzilla.
I couldn't find anything, but it's possible that "isinf" isn't what
I should have searched on.

Anyway, my estimation is that this is a compiler bug that's been
repaired, and it probably isn't widespread enough to justify our
inserting some klugy workaround.

			regards, tom lane

  6. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Victor Wagner <vitus@wagner.pp.ru> — 2018-09-04T19:52:49Z

    В Tue, 4 Sep 2018 11:06:56 -0700
Thomas Munro <thomas.munro@enterprisedb.com> пишет:

> On Tue, Sep 4, 2018 at 9:39 AM Andrew Gierth
> <andrew@tao11.riddles.org.uk> wrote:
> >  
> > >>>>> "Andres" == Andres Freund <andres@anarazel.de> writes:  
> >  
> >  >> However, this commit broke float8 test on 32-bit FreeBSD 11 with
> >  >> clang 3.8.0 compiler. Regressions.diff follows:  
> >  
> >  Andres> Does this happen with a newer clang version too?  
> >
> > float8 test (and all other tests) passes for me on clang 3.9.1 on
> > fbsd11 on 32-bit ARM, and on -m32 builds on amd64.
> >
> > I also confirmed that without #define isinf(x) __builtin_isinf(x),
> > on both 32bit and 64bit fbsd isinf() compiles as a function call,
> > so the OP's proposed change would not be desirable.  
> 
> I installed FreeBSD 11.2 i386 on a virtual machine.  I couldn't
> reproduce the problem with either the base cc (clang 6.0.0) or clang38
> (clang 3.8.1) installed via pkg.

Do you reproducing problem in REL_11_STABLE? It doesn't exist in master.
Also may be it might depend on some configure options. I usually
compile postgres with as much --with-something enabled as possible

Ive put my cconfigure/build/check logs to 

https://wagner.pp.ru/~vitus/stuff/freebsd-i386-logs.tar.gz

Unfortunately there is a bit too much of them to attach to the list
message.

These logs are produced after I've upgraded my outdated system
to current 11.2 with clang 6.0.0. It haven't eliminated problem.

I can publish my KVM images both of old system (11.0 witth clang 3.8.0)
and new one (current 11.2 with clang 6.0.0)

> 
> The OP reported clang 3.8.0, so a minor version behind what I tested.
> 
> I did learn that "make check" fails in rolenames if your Unix user is
> called "user".
> 



-- 
                                   Victor Wagner <vitus@wagner.pp.ru>

  7. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Victor Wagner <vitus@wagner.pp.ru> — 2018-09-04T19:59:24Z

    В Tue, 04 Sep 2018 15:48:24 -0400
Tom Lane <tgl@sss.pgh.pa.us> пишет:


> I tried to reproduce the problem, without success, on a few different
> FreeBSD images I had laying about:
> 
> FreeBSD 11.0/x86_64, clang version 3.8.0
> 	(this confirms OP's report that x86_64 is OK)
> 
> FreeBSD 10.3/ppc, gcc 4.2.1
> 
> FreeBSD 12.0-CURRENT (from around mid-May)/arm64, clang version 6.0.0
> 
> (I was testing PG HEAD, not the 11 branch, but I don't see a reason
> to think that'd make a difference.)

Alas, it does. First thing I've done after discovering this bug, it is
to look if it exists in master. And master passes this test on the same
machine where problem was discovered.


> I also looked for evidence of a bug of this sort in the clang
> bugzilla. I couldn't find anything, but it's possible that "isinf"
> isn't what I should have searched on.
> 
> Anyway, my estimation is that this is a compiler bug that's been
> repaired, and it probably isn't widespread enough to justify our
> inserting some klugy workaround.

It doesn't look so, as bug persists after I've upgraded system to
current 11.2-RELEASE with clang 6.0.0.



-- 
                                   Victor Wagner <vitus@wagner.pp.ru>

  8. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Thomas Munro <thomas.munro@enterprisedb.com> — 2018-09-04T20:06:38Z

    On Tue, Sep 4, 2018 at 12:59 PM Victor Wagner <vitus@wagner.pp.ru> wrote:
>
> В Tue, 04 Sep 2018 15:48:24 -0400
> Tom Lane <tgl@sss.pgh.pa.us> пишет:
>
>
> > I tried to reproduce the problem, without success, on a few different
> > FreeBSD images I had laying about:
> >
> > FreeBSD 11.0/x86_64, clang version 3.8.0
> >       (this confirms OP's report that x86_64 is OK)
> >
> > FreeBSD 10.3/ppc, gcc 4.2.1
> >
> > FreeBSD 12.0-CURRENT (from around mid-May)/arm64, clang version 6.0.0
> >
> > (I was testing PG HEAD, not the 11 branch, but I don't see a reason
> > to think that'd make a difference.)
>
> Alas, it does. First thing I've done after discovering this bug, it is
> to look if it exists in master. And master passes this test on the same
> machine where problem was discovered.
>
>
> > I also looked for evidence of a bug of this sort in the clang
> > bugzilla. I couldn't find anything, but it's possible that "isinf"
> > isn't what I should have searched on.
> >
> > Anyway, my estimation is that this is a compiler bug that's been
> > repaired, and it probably isn't widespread enough to justify our
> > inserting some klugy workaround.
>
> It doesn't look so, as bug persists after I've upgraded system to
> current 11.2-RELEASE with clang 6.0.0.

Ah, right it fails for me too on 32 bit FreeBSD 11.2 with
REL_11_STABLE.  Investigating...

-- 
Thomas Munro
http://www.enterprisedb.com

  9. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-04T20:13:45Z

    Victor Wagner <vitus@wagner.pp.ru> writes:
> Tom Lane <tgl@sss.pgh.pa.us> пишет:
>> (I was testing PG HEAD, not the 11 branch, but I don't see a reason
>> to think that'd make a difference.)

> Alas, it does. First thing I've done after discovering this bug, it is
> to look if it exists in master. And master passes this test on the same
> machine where problem was discovered.

Oh really ... I'll go try again.  Now I agree with Andres, we need
to understand *exactly* what is failing here rather than guess.

My first thought was that the C99 changeover might explain the
difference.  But clang doesn't require any special switch to select
C99 mode, so in principle that should've made no difference to clang
builds.

			regards, tom lane

  10. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-04T20:16:05Z

    >>>>> "Victor" == Victor Wagner <vitus@wagner.pp.ru> writes:

 Victor> Do you reproducing problem in REL_11_STABLE? It doesn't exist
 Victor> in master.

Oh, I missed that. It is reproducible on REL_11_STABLE with clang 3.9.1,
I'll dig into it more.

-- 
Andrew (irc:RhodiumToad)

  11. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andres Freund <andres@anarazel.de> — 2018-09-04T20:25:21Z

    On 2018-09-04 16:13:45 -0400, Tom Lane wrote:
> Victor Wagner <vitus@wagner.pp.ru> writes:
> > Tom Lane <tgl@sss.pgh.pa.us> пишет:
> >> (I was testing PG HEAD, not the 11 branch, but I don't see a reason
> >> to think that'd make a difference.)
> 
> > Alas, it does. First thing I've done after discovering this bug, it is
> > to look if it exists in master. And master passes this test on the same
> > machine where problem was discovered.
> 
> Oh really ... I'll go try again.  Now I agree with Andres, we need
> to understand *exactly* what is failing here rather than guess.
> 
> My first thought was that the C99 changeover might explain the
> difference.  But clang doesn't require any special switch to select
> C99 mode, so in principle that should've made no difference to clang
> builds.

Thomas and I are sitting in a cafe and are trying to figure out what's
going on...

Greetings,

Andres Freund

  12. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-04T20:59:59Z

    >>>>> "Andres" == Andres Freund <andres@anarazel.de> writes:

 Andres> Thomas and I are sitting in a cafe and are trying to figure out
 Andres> what's going on...

I have a standalone test case:

#include <stdio.h>
#include <math.h>

int main(int argc, char **argv)
{
    double d1 = (argc ? 1e180 : 0);
    double d2 = (argv ? 1e200 : 0);
    int r2 = __builtin_isinf(d1 * d2);
    int r1 = isinf(d1 * d2);
    printf("r1 = %d, r2 = %d\n", r1, r2);
    return 0;
}

Note that swapping the r1 and r2 lines makes the problem disappear (!).

on amd64, clang 3.9.1:

cc -O2 -m32 flttst.c && ./a.out
r1 = 1, r2 = 0

cc -O2 flttst.c && ./a.out
r1 = 1, r2 = 1

Can't reproduce on 32-bit arm.

-- 
Andrew (irc:RhodiumToad)

  13. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-04T21:14:19Z

    >>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
>>>>> "Andres" == Andres Freund <andres@anarazel.de> writes:

 Andres> Thomas and I are sitting in a cafe and are trying to figure out
 Andres> what's going on...

 Andrew> I have a standalone test case:

 Andrew> #include <stdio.h>
 Andrew> #include <math.h>

 Andrew> int main(int argc, char **argv)
 Andrew> {
 Andrew>     double d1 = (argc ? 1e180 : 0);
 Andrew>     double d2 = (argv ? 1e200 : 0);
 Andrew>     int r2 = __builtin_isinf(d1 * d2);
 Andrew>     int r1 = isinf(d1 * d2);
 Andrew>     printf("r1 = %d, r2 = %d\n", r1, r2);
 Andrew>     return 0;
 Andrew> }

 Andrew> Note that swapping the r1 and r2 lines makes the problem
 Andrew> disappear (!).

And that's the clue to why it happens.

The reason it behaves oddly is this: on i387 FPU (and NOT on arm32 or on
32-bit i386 with a modern architecture specified to the compiler), the
result of 1e200 * 1e180 is not in fact infinite, because it fits in an
80-bit long double. So __builtin_isinf reports that it is finite; but if
it gets stored to memory as a double (e.g. to pass as a parameter to a
function), it then becomes infinite.

 Andrew> cc -O2 -m32 flttst.c && ./a.out
 Andrew> r1 = 1, r2 = 0

Specifying a recent microarch makes it use 64-bit FP registers rather
than 80-bit ones:

cc -O2 -m32 -march=skylake flttst.c && ./a.out
r1 = 1, r2 = 1

-- 
Andrew (irc:RhodiumToad)

  14. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-04T21:46:29Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> The reason it behaves oddly is this: on i387 FPU (and NOT on arm32 or on
> 32-bit i386 with a modern architecture specified to the compiler), the
> result of 1e200 * 1e180 is not in fact infinite, because it fits in an
> 80-bit long double. So __builtin_isinf reports that it is finite; but if
> it gets stored to memory as a double (e.g. to pass as a parameter to a
> function), it then becomes infinite.

Ah-hah.  Can we fix it by explicitly casting the argument of isinf
to double?

			regards, tom lane

  15. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-04T22:06:07Z

    >>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

 Andrew> Specifying a recent microarch makes it use 64-bit FP registers
 Andrew> rather than 80-bit ones:

 Andrew> cc -O2 -m32 -march=skylake flttst.c && ./a.out
 Andrew> r1 = 1, r2 = 1

where "recent" means "since about 2000 or so":

cc -O2 -m32 -march=pentium4 flttst.c && ./a.out
r1 = 1, r2 = 1

(the actual requirement is for SSE2 support)

-- 
Andrew (irc:RhodiumToad)

  16. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-04T22:10:12Z

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

 >> The reason it behaves oddly is this: on i387 FPU (and NOT on arm32
 >> or on 32-bit i386 with a modern architecture specified to the
 >> compiler), the result of 1e200 * 1e180 is not in fact infinite,
 >> because it fits in an 80-bit long double. So __builtin_isinf reports
 >> that it is finite; but if it gets stored to memory as a double (e.g.
 >> to pass as a parameter to a function), it then becomes infinite.

 Tom> Ah-hah. Can we fix it by explicitly casting the argument of isinf
 Tom> to double?

No; the generated code doesn't change. Presumably the compiler regards
the value as already being of type "double", just one that happens to be
stored in a longer register.

-- 
Andrew (irc:RhodiumToad)

  17. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andres Freund <andres@anarazel.de> — 2018-09-04T22:16:27Z

    On 2018-09-04 17:46:29 -0400, Tom Lane wrote:
> Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> > The reason it behaves oddly is this: on i387 FPU (and NOT on arm32 or on
> > 32-bit i386 with a modern architecture specified to the compiler), the
> > result of 1e200 * 1e180 is not in fact infinite, because it fits in an
> > 80-bit long double. So __builtin_isinf reports that it is finite; but if
> > it gets stored to memory as a double (e.g. to pass as a parameter to a
> > function), it then becomes infinite.
> 
> Ah-hah.  Can we fix it by explicitly casting the argument of isinf
> to double?

No, that doesn't work, afaict the cast has vanished long before the
optimizer stage.  Note that we've previously encountered similar issues
on gcc, which is why we've tried to force gcc's hand with
-fexcess-precision=standard.

The apparent reason this doesn't fail on master is that there
check_float8_val() is an inline function, rather than a macro, but the
inline function doesn't get inlined (probably due to the number of
callsites). As the parameter passing convention doesn't do 80bit FPU
registers, the value is infinity by the time it gets to
__builtin_isinf().

I think this is pretty clearly a C99 violation by clang (note how gcc
automatically enables -fexcess-precision=standard in C99 mode).  I'll
report something, but I've little hope this getting fixed quickly - and
it'll definitely not get fixed for such old clang versions as the OP used.

While we obviously could just neuter the isinf() macro hackery in this
edge case, my concern is that it seems likely that further such issues
are hidden in code that doesn't immediately cause regression test
failure.

I kinda wonder if we should add -mno-x87 or such in configure when we
detect clang, obviously it doesn't deal correctly with this. SSE2 based
floating point math is far from new...  Or just error out in configure,
although that seems a bit harder.


I'm now looking at how it comes that clang on linux doesn't default to
x87 math, but freebsd does.


Greetings,

Andres Freund

  18. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andres Freund <andres@anarazel.de> — 2018-09-04T22:44:36Z

    On 2018-09-04 15:16:27 -0700, Andres Freund wrote:
> I'm now looking at how it comes that clang on linux doesn't default to
> x87 math, but freebsd does.

Oh well. I present you, without comments, the following:

  switch (Triple.getOS()) {
  case llvm::Triple::FreeBSD:
  case llvm::Triple::NetBSD:
  case llvm::Triple::OpenBSD:
    return "i486";
  case llvm::Triple::Haiku:
    return "i586";
  default:
    // Fallback to p4.
    return "pentium4";
  }

Greetings,

Andres Freund

  19. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andres Freund <andres@anarazel.de> — 2018-09-04T22:59:29Z

    Hi,

On 2018-09-04 15:16:27 -0700, Andres Freund wrote:
> I think this is pretty clearly a C99 violation by clang (note how gcc
> automatically enables -fexcess-precision=standard in C99 mode).  I'll
> report something, but I've little hope this getting fixed quickly - and
> it'll definitely not get fixed for such old clang versions as the OP used.

https://bugs.llvm.org/show_bug.cgi?id=38833

Greetings,

Andres Freund

  20. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-04T23:02:02Z

    Andres Freund <andres@anarazel.de> writes:
> ... Note that we've previously encountered similar issues
> on gcc, which is why we've tried to force gcc's hand with
> -fexcess-precision=standard.

Right.  Annoying that clang doesn't have that.  We can't realistically
program around an issue that might or might not show up depending on the
whims of the compiler's register allocation.

> I kinda wonder if we should add -mno-x87 or such in configure when we
> detect clang, obviously it doesn't deal correctly with this.

Seems worth looking into, but what happens if someone tries to compile
for x87 hardware?  Or do we care anymore?

			regards, tom lane

  21. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andres Freund <andres@anarazel.de> — 2018-09-04T23:18:48Z

    On 2018-09-04 19:02:02 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > ... Note that we've previously encountered similar issues
> > on gcc, which is why we've tried to force gcc's hand with
> > -fexcess-precision=standard.
> 
> Right.  Annoying that clang doesn't have that.  We can't realistically
> program around an issue that might or might not show up depending on the
> whims of the compiler's register allocation.

Right.


> > I kinda wonder if we should add -mno-x87 or such in configure when we
> > detect clang, obviously it doesn't deal correctly with this.
> 
> Seems worth looking into, but what happens if someone tries to compile
> for x87 hardware?  Or do we care anymore?

It generates linker errors:

clang-8 -std=gnu99 -march=i386 -O2 -m32 -mno-x87 ~/tmp/flttst.c -o ~/tmp/flttst && ~/tmp/flttst
/usr/bin/ld: /tmp/flttst-ba93f5.o: in function `main':
flttst.c:(.text+0x37): undefined reference to `__muldf3'
/usr/bin/ld: flttst.c:(.text+0x67): undefined reference to `__gedf2'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

I think we only should add the flag when detecting clang, so a user with
just a < pentium-4 could resort to gcc.  Although honestly, I don't
think that's really something usefully to cater for.

The one hangup I have with this is that clang on *bsd defaults to i486
(and thus x87) when compiling 32bit binaries. That's obviously
*terrible* for performance, but it'd still be annoying for some users.
While I don't immediately know how, that seems like an argument for
doing this in configure, and suggesting a better compiler flag.

Greetings,

Andres Freund

  22. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-05T00:47:52Z

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

 >> I kinda wonder if we should add -mno-x87 or such in configure when
 >> we detect clang, obviously it doesn't deal correctly with this.

 Tom> Seems worth looking into, but what happens if someone tries to
 Tom> compile for x87 hardware? Or do we care anymore?

Already discussed this one on IRC with Andres, but to put this on record
for future reference: we can't use -mno-x87 on 32bit intel, even with an
-march= option with an SSE2 capable CPU, because the 32-bit ABI requires
floats to be returned in the x87 registers and breaking that either
results in silently wrong results or in clang dying with "fatal error:
error in backend: X87 register return with X87 disabled" or similar.

-- 
Andrew (irc:RhodiumToad)

  23. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andres Freund <andres@anarazel.de> — 2018-09-05T00:51:30Z

    On 2018-09-05 01:47:52 +0100, Andrew Gierth wrote:
> >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> 
>  >> I kinda wonder if we should add -mno-x87 or such in configure when
>  >> we detect clang, obviously it doesn't deal correctly with this.
> 
>  Tom> Seems worth looking into, but what happens if someone tries to
>  Tom> compile for x87 hardware? Or do we care anymore?
> 
> Already discussed this one on IRC with Andres, but to put this on record
> for future reference: we can't use -mno-x87 on 32bit intel, even with an
> -march= option with an SSE2 capable CPU, because the 32-bit ABI requires
> floats to be returned in the x87 registers and breaking that either
> results in silently wrong results or in clang dying with "fatal error:
> error in backend: X87 register return with X87 disabled" or similar.

My current proposal is thus to do add a check that does
#if defined(__clang__) && defined(__i386__) && !defined(__SSE2_MATH__)
something-that-fails
#endif
in an autoconf test, and have configure complain if that
fails. Something roughly along the lines of
"Compiling PostgreSQL with clang, on 32bit x86, requires SSE2 support. Use -msse2 or use gcc."

Greetings,

Andres Freund

  24. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Victor Wagner <vitus@wagner.pp.ru> — 2018-09-05T06:21:30Z

    On Tue, 4 Sep 2018 17:51:30 -0700
Andres Freund <andres@anarazel.de> wrote:


> #endif
> in an autoconf test, and have configure complain if that
> fails. Something roughly along the lines of
> "Compiling PostgreSQL with clang, on 32bit x86, requires SSE2
> support. Use -msse2 or use gcc."

Adding CFLAGS=-msse2 to configure environment solved my problem 
both on the upgraded virtual machine with clang 6.0.0 and on old one
with clang 3.8.0

Although I now don't have 32-bit build system with clang on Linux.
--

  25. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-09-05T07:23:40Z

    On 05/09/2018 02:51, Andres Freund wrote:
> My current proposal is thus to do add a check that does
> #if defined(__clang__) && defined(__i386__) && !defined(__SSE2_MATH__)
> something-that-fails
> #endif
> in an autoconf test, and have configure complain if that
> fails. Something roughly along the lines of
> "Compiling PostgreSQL with clang, on 32bit x86, requires SSE2 support. Use -msse2 or use gcc."

Would this not be invalidated if the bug you have filed gets fixed?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  26. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-05T14:05:26Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 05/09/2018 02:51, Andres Freund wrote:
>> My current proposal is thus to do add a check that does
>> #if defined(__clang__) && defined(__i386__) && !defined(__SSE2_MATH__)
>> something-that-fails
>> #endif
>> in an autoconf test, and have configure complain if that fails.

> Would this not be invalidated if the bug you have filed gets fixed?

Perhaps, but how would autoconf tell that?  I wouldn't have any confidence
in a runtime test, as it might or might not trigger the bug depending on
all sorts of factors like -O level.

In any case, it seems like "use -msse2 on x86" is good advice from a
performance standpoint even if it doesn't prevent a compiler bug.

One thought is that maybe we should provide a way to override this,
in case somebody really can't or doesn't want to use -msse2, and
is willing to put up with platform-dependent float behavior.

			regards, tom lane

  27. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andres Freund <andres@anarazel.de> — 2018-09-05T16:42:06Z

    Hi,

On 2018-09-05 10:05:26 -0400, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> > On 05/09/2018 02:51, Andres Freund wrote:
> >> My current proposal is thus to do add a check that does
> >> #if defined(__clang__) && defined(__i386__) && !defined(__SSE2_MATH__)
> >> something-that-fails
> >> #endif
> >> in an autoconf test, and have configure complain if that fails.
> 
> > Would this not be invalidated if the bug you have filed gets fixed?

Realistically we're going to be running into old versions of clang for a
long time.  And the importance of running i386 without SSE2 surely isn't
increasing.  So I don't really see an urgent need to do anything about
it. And if it gets fixed, and we care, we can just add a clang version
check to the test.


> Perhaps, but how would autoconf tell that?  I wouldn't have any confidence
> in a runtime test, as it might or might not trigger the bug depending on
> all sorts of factors like -O level.
> 
> In any case, it seems like "use -msse2 on x86" is good advice from a
> performance standpoint even if it doesn't prevent a compiler bug.

Indeed.


> One thought is that maybe we should provide a way to override this,
> in case somebody really can't or doesn't want to use -msse2, and
> is willing to put up with platform-dependent float behavior.

IDK, people that are capable of making that decision can just hack
configure.

Greetings,

Andres Freund

  28. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-09-05T16:55:34Z

    On 05/09/2018 18:42, Andres Freund wrote:
> Realistically we're going to be running into old versions of clang for a
> long time.  And the importance of running i386 without SSE2 surely isn't
> increasing.  So I don't really see an urgent need to do anything about
> it. And if it gets fixed, and we care, we can just add a clang version
> check to the test.

Another option perhaps is to let this be and accept it as alternative
floating point behavior.  We already have some of those.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  29. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andres Freund <andres@anarazel.de> — 2018-09-05T16:58:37Z

    On 2018-09-05 18:55:34 +0200, Peter Eisentraut wrote:
> On 05/09/2018 18:42, Andres Freund wrote:
> > Realistically we're going to be running into old versions of clang for a
> > long time.  And the importance of running i386 without SSE2 surely isn't
> > increasing.  So I don't really see an urgent need to do anything about
> > it. And if it gets fixed, and we care, we can just add a clang version
> > check to the test.
> 
> Another option perhaps is to let this be and accept it as alternative
> floating point behavior.  We already have some of those.

-many.  We'd directly violate our own error rules.  I'm actually
personally in favor of not throwing error when float overflows - it's
imo not actually useful and costs performance - but sometimes throwing
an error depending on the specific register allocator behaviour of a
specific version of a compiler is bad.  It's really weird to return
[+-]Infinity depending on just *how much* you overflowed.

Greetings,

Andres Freund

  30. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-05T17:06:36Z

    Andres Freund <andres@anarazel.de> writes:
> On 2018-09-05 18:55:34 +0200, Peter Eisentraut wrote:
>> Another option perhaps is to let this be and accept it as alternative
>> floating point behavior.  We already have some of those.

> -many.  We'd directly violate our own error rules.

I think that just from a regression-test-maintenance standpoint, this
approach would be a mess.  We could expect for instance that such
problems would come and go in weird places in the geometric tests.

			regards, tom lane

  31. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-05T18:08:11Z

    Andres Freund <andres@anarazel.de> writes:
> On 2018-09-05 10:05:26 -0400, Tom Lane wrote:
>> One thought is that maybe we should provide a way to override this,
>> in case somebody really can't or doesn't want to use -msse2, and
>> is willing to put up with platform-dependent float behavior.

> IDK, people that are capable of making that decision can just hack
> configure.

"Just hacking configure" is a pretty high bar for most folk.

If you wanted to argue that the set of people who still want to run PG
on pre-SSE2 hardware is the empty set, that'd be an easier sell really.
But what I'm really concerned about here, given that we're apparently
talking about an ABI change, is that someone might want to run on a
platform where the libraries insist on the x87 way.  I'm actually a bit
surprised to hear that you can just randomly add -msse2 on BSDen.  Do
they ship separate copies of libc et al to make that work?

			regards, tom lane

  32. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andres Freund <andres@anarazel.de> — 2018-09-05T18:15:49Z

    On 2018-09-05 14:08:11 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2018-09-05 10:05:26 -0400, Tom Lane wrote:
> >> One thought is that maybe we should provide a way to override this,
> >> in case somebody really can't or doesn't want to use -msse2, and
> >> is willing to put up with platform-dependent float behavior.
> 
> > IDK, people that are capable of making that decision can just hack
> > configure.
> 
> "Just hacking configure" is a pretty high bar for most folk.

Sure. But being able to accurately judge whether you're ok that math
behaviour doesn't work as documented seems like a high bar too.  And
this'd only be relevant if they insist on using clang rather than gcc.


> If you wanted to argue that the set of people who still want to run PG
> on pre-SSE2 hardware is the empty set, that'd be an easier sell
> really.

I think it's an empty set, yea, especially with clang.


> But what I'm really concerned about here, given that we're apparently
> talking about an ABI change, is that someone might want to run on a
> platform where the libraries insist on the x87 way.
> I'm actually a bit surprised to hear that you can just randomly add
> -msse2 on BSDen.  Do they ship separate copies of libc et al to make
> that work?

I don't think we're talking about an ABI change - with -msse2 the
calling conventions are unchanged, the stuff just gets moved out of the
x87 registers into SSE ones / xmm for the actual math. Which in turn
solves our "80bit register" problem.  There really shouldn't be any need
for libc6 itself to care.

Greetings,

Andres Freund

  33. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-05T19:01:05Z

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

 Tom> If you wanted to argue that the set of people who still want to
 Tom> run PG on pre-SSE2 hardware is the empty set, that'd be an easier
 Tom> sell really.

At the very minimum, I believe FreeBSD project policy would require that
the i386 packages for postgresql be built to run without SSE2.

 Tom> But what I'm really concerned about here, given that we're
 Tom> apparently talking about an ABI change,

No, there's no ABI change involved. 

 Tom> I'm actually a bit surprised to hear that you can just randomly
 Tom> add -msse2 on BSDen. Do they ship separate copies of libc et al to
 Tom> make that work?

-msse2 just tells the compiler to assume that the SSE2 instructions and
registers are available for use (and if so, it will always use them for
floats in preference to the x87 registers where possible), it doesn't
change the ABI: all function results are still bounced through the x87
register stack (and float/double parameters are not passed in registers
anyway).

-- 
Andrew (irc:RhodiumToad)

  34. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-05T19:05:31Z

    >>>>> "Peter" == Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

 > On 05/09/2018 18:42, Andres Freund wrote:
 >> Realistically we're going to be running into old versions of clang
 >> for a long time. And the importance of running i386 without SSE2
 >> surely isn't increasing. So I don't really see an urgent need to do
 >> anything about it. And if it gets fixed, and we care, we can just
 >> add a clang version check to the test.

 Peter> Another option perhaps is to let this be and accept it as
 Peter> alternative floating point behavior. We already have some of
 Peter> those.

If it was only a matter of error handling, then the best fix would
likely to be just avoiding __builtin_isinf if (clang AND i386 AND not
sse2).

The problem is that if we're relying on -fexcess-precision=standard
semantics in places besides infinity checks, then we won't get those
semantics on clang/i386/no-sse2 since it has no comparable option. (What
are we doing about compilers for x86-32 other than clang and gcc?)

-- 
Andrew (irc:RhodiumToad)

  35. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-05T19:05:42Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>  Tom> If you wanted to argue that the set of people who still want to
>  Tom> run PG on pre-SSE2 hardware is the empty set, that'd be an easier
>  Tom> sell really.

> At the very minimum, I believe FreeBSD project policy would require that
> the i386 packages for postgresql be built to run without SSE2.

Well, can we tell them they have to use gcc to compile, or will that
be against their policy too?

			regards, tom lane

  36. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-05T19:15:58Z

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

 >> At the very minimum, I believe FreeBSD project policy would require
 >> that the i386 packages for postgresql be built to run without SSE2.

 Tom> Well, can we tell them they have to use gcc to compile, or will
 Tom> that be against their policy too?

That's no problem, lots of ports have USE_GCC=yes (or some specified
version or minimum version of gcc).

-- 
Andrew (irc:RhodiumToad)

  37. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-05T19:16:17Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> The problem is that if we're relying on -fexcess-precision=standard
> semantics in places besides infinity checks, then we won't get those
> semantics on clang/i386/no-sse2 since it has no comparable option. (What
> are we doing about compilers for x86-32 other than clang and gcc?)

Well, the bigger problem is that we don't really know exactly where
nonstandard precision semantics might cause visible behavior changes.
It's just about a dead certainty, IMO, that there are some other
places we don't know about because the regression tests don't expose
them; or that future code rearrangements might create new problems.

Given that (if I understand Andres correctly) -fexcess-precision=standard
behavior is required by C99, it's hard to get excited about expending
a whole lot of work here.  I'm fine with adding some compiler options
or telling the user to do so, but I don't think we should do anything
much beyond that.

			regards, tom lane

  38. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andres Freund <andres@anarazel.de> — 2018-09-13T21:29:59Z

    On 2018-09-04 17:51:30 -0700, Andres Freund wrote:
> My current proposal is thus to do add a check that does
> #if defined(__clang__) && defined(__i386__) && !defined(__SSE2_MATH__)
> something-that-fails
> #endif
> in an autoconf test, and have configure complain if that
> fails. Something roughly along the lines of
> "Compiling PostgreSQL with clang, on 32bit x86, requires SSE2 support. Use -msse2 or use gcc."

Here's a patch along those lines.

Greetings,

Andres Freund

  39. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andres Freund <andres@anarazel.de> — 2018-09-21T01:29:40Z

    Hi,

On 2018-09-13 14:29:59 -0700, Andres Freund wrote:
> On 2018-09-04 17:51:30 -0700, Andres Freund wrote:
> > My current proposal is thus to do add a check that does
> > #if defined(__clang__) && defined(__i386__) && !defined(__SSE2_MATH__)
> > something-that-fails
> > #endif
> > in an autoconf test, and have configure complain if that
> > fails. Something roughly along the lines of
> > "Compiling PostgreSQL with clang, on 32bit x86, requires SSE2 support. Use -msse2 or use gcc."
> 
> Here's a patch along those lines.

And pushed. I assume we should note this somewhat prominently in the
minor release notes.

- Andres

  40. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-12-01T17:06:36Z

    Andres Freund <andres@anarazel.de> writes:
> On 2018-09-04 17:51:30 -0700, Andres Freund wrote:
>> My current proposal is thus to do add a check that does
>> #if defined(__clang__) && defined(__i386__) && !defined(__SSE2_MATH__)
>> something-that-fails
>> #endif
>> in an autoconf test, and have configure complain if that
>> fails. Something roughly along the lines of
>> "Compiling PostgreSQL with clang, on 32bit x86, requires SSE2 support. Use -msse2 or use gcc."

> Here's a patch along those lines.

I've been having an off-list discussion with the submitter of bug #14913
[1], in which the percentile_disc regression test returned unexpected
results.  The upshot of that is that he's using gcc 3.4.6 on x86 hardware,
and it is doing something that changes the roundoff behavior in this
line in orderedsetaggs.c:

                int64        row = (int64) ceil(p * rowcount);

gcc 3.4.6 is too old to have -fexcess-precision=standard of course, but
adding -msse2 to CFLAGS fixes the problem.  So it now seems to me that
we were too narrow-minded in thinking that only clang has this issue.

Looking at the buildfarm, our only extant member that is on x86, and
is not using clang, and doesn't have -fexcess-precision=standard, is
dromedary which is using Apple's old toolchain.  So the fact that it
isn't showing the problem isn't very good evidence about how widespread
this issue might be with older gcc versions.

I wonder whether we shouldn't remove the clang aspect of the test
you added, ie just make it read "if on x86, you must have either
-fexcess-precision=standard or -msse2".  Or should we go so far
as to have configure add -msse2 automatically?

			regards, tom lane

[1] https://www.postgresql.org/message-id/20171116224401.1466.68649%40wrigleys.postgresql.org

  41. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-12-01T18:30:10Z

    I wrote:
> Looking at the buildfarm, our only extant member that is on x86, and
> is not using clang, and doesn't have -fexcess-precision=standard, is
> dromedary which is using Apple's old toolchain.  So the fact that it
> isn't showing the problem isn't very good evidence about how widespread
> this issue might be with older gcc versions.

... and, in fact, a bit of investigation says that that compiler has
-msse2 on by default.  So actually we have *no* buildfarm coverage
of x87 math.

I tried forcing the issue with -mfpmath=387, but failed to reproduce
the reported failure.  Still, seeing that it's a considerably different
version of gcc, that's not so surprising.

			regards, tom lane

  42. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andres Freund <andres@anarazel.de> — 2018-12-01T21:20:56Z

    Hi,m

On 2018-12-01 12:06:36 -0500, Tom Lane wrote:
> I wonder whether we shouldn't remove the clang aspect of the test
> you added, ie just make it read "if on x86, you must have either
> -fexcess-precision=standard or -msse2".

That sounds reasonable.


> Or should we go so far as to have configure add -msse2 automatically?

Probably not - there's several OSs / distributions that do not yet
require an SSE2 capable CPU. While I personally don't care much about
such machines, I don't think it's worth desupportem them as long as
mainstream compilers with -fexcess-precision=standard exist.

Greetings,

Andres Freund

  43. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-12-01T22:28:54Z

    Andres Freund <andres@anarazel.de> writes:
> On 2018-12-01 12:06:36 -0500, Tom Lane wrote:
>> I wonder whether we shouldn't remove the clang aspect of the test
>> you added, ie just make it read "if on x86, you must have either
>> -fexcess-precision=standard or -msse2".

> That sounds reasonable.

>> Or should we go so far as to have configure add -msse2 automatically?

> Probably not - there's several OSs / distributions that do not yet
> require an SSE2 capable CPU. While I personally don't care much about
> such machines, I don't think it's worth desupportem them as long as
> mainstream compilers with -fexcess-precision=standard exist.

I guess I wasn't precise enough: I meant add -msse2 if on x86 and
compiler doesn't take -fexcess-precision=standard.

			regards, tom lane

  44. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Andres Freund <andres@anarazel.de> — 2018-12-01T22:54:19Z

    Hi,

On 2018-12-01 17:28:54 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2018-12-01 12:06:36 -0500, Tom Lane wrote:
> >> Or should we go so far as to have configure add -msse2 automatically?
> 
> > Probably not - there's several OSs / distributions that do not yet
> > require an SSE2 capable CPU. While I personally don't care much about
> > such machines, I don't think it's worth desupportem them as long as
> > mainstream compilers with -fexcess-precision=standard exist.
> 
> I guess I wasn't precise enough: I meant add -msse2 if on x86 and
> compiler doesn't take -fexcess-precision=standard.

Hm, I still don't like that: It'd silently bump the minimum required
architecture. Like in the case of x86-32 freebsd, which doesn't require
sse2 and uses clang, the maintainer wouldn't notice that they would have
to switch to gcc to continue supporting their baseline.

It's not like there's that many people compiling for such platforms with
insufficient compiler support, so forcing them to specify -msse2 if
that's the desired escape hatch doesn't sound terrible to me.

Greetings,

Andres Freund

  45. Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-12-03T19:16:23Z

    Andres Freund <andres@anarazel.de> writes:
> On 2018-12-01 17:28:54 -0500, Tom Lane wrote:
>> I guess I wasn't precise enough: I meant add -msse2 if on x86 and
>> compiler doesn't take -fexcess-precision=standard.

> Hm, I still don't like that: It'd silently bump the minimum required
> architecture. Like in the case of x86-32 freebsd, which doesn't require
> sse2 and uses clang, the maintainer wouldn't notice that they would have
> to switch to gcc to continue supporting their baseline.

Yeah, that's a fair point.

> It's not like there's that many people compiling for such platforms with
> insufficient compiler support, so forcing them to specify -msse2 if
> that's the desired escape hatch doesn't sound terrible to me.

I spent some time over the weekend investigating this, including going
so far as to build gcc 3.4.6 from source to try to reproduce the report
I had off-list.  No luck; passed all regression tests just fine.  In
fact, I was only able to produce any test failures at all with clang.
My conclusion is that it's really hard to get gcc to exhibit this type of
problem, even when using old versions that don't have -fexcess-precision.

I am now thinking that the right thing to do is nothing.  If we insist
on having -msse2 or -fexcess-precision, the net effect is going to be
to break the build on more old platforms than we fix anything on.  Yes,
updating to a newer compiler or moving your hardware baseline are both
recommendable things to do, but why should we force that on people who
aren't seeing a problem?  Especially in stable back branches?
(In HEAD this should be moot anyway, given that C99 implies
-fexcess-precision=standard.)

			regards, tom lane