Thread: Bug fix for glibc broke freebsd build in REL_11_STABLE
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.
--
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.
>>>>> "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)
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
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
В 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>
В 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>
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
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
>>>>> "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)
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
>>>>> "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)
>>>>> "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)
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
>>>>> "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)
>>>>> "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)
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
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
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
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
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
>>>>> "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)
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
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. --
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
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
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
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
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
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
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
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
>>>>> "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)
>>>>> "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)
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
>>>>> "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)
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
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
Attachment
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
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
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
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
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
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
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