Build with LTO / -flto on macOS
Building with clang and -flto on macOS currently fails with errors
similar to [1]/messages/by-id/1581936537572-0.post@n3.nabble.com. This is because the --export-dynamic flag is called
-export_dynamic [2]https://opensource.apple.com/source/ld64/ld64-609/doc/man/man1/ld.1.auto.html (grep for export_dynamic) instead and we have not been passing this variant to
the linker, so far.
Attached patch fixes that for configure/make.
CC: Tom, who hit the same in [3]/messages/by-id/21800.1499270547@sss.pgh.pa.us and Andres who last touched
--export-dynamic in 9db49fc5bfdc0126be03f4b8986013e59d93b91d.
Will also create an issue upstream for meson, because the logic is
built-in there.
Would be great if this could be back-patched, since this is the same in
all live versions.
Best,
Wolfgang
[1]: /messages/by-id/1581936537572-0.post@n3.nabble.com
[2]: https://opensource.apple.com/source/ld64/ld64-609/doc/man/man1/ld.1.auto.html (grep for export_dynamic)
https://opensource.apple.com/source/ld64/ld64-609/doc/man/man1/ld.1.auto.html
(grep for export_dynamic)
[3]: /messages/by-id/21800.1499270547@sss.pgh.pa.us
Attachments:
v1-0001-Make-building-with-clang-s-LTO-work-on-macOS.patchtext/x-patch; charset=UTF-8; name=v1-0001-Make-building-with-clang-s-LTO-work-on-macOS.patchDownload
From 55175631d25af11971ec7b7f89d5bf4958ed3c8b Mon Sep 17 00:00:00 2001
From: Wolfgang Walther <walther@technowledgy.de>
Date: Sun, 2 Jun 2024 10:46:56 +0200
Subject: [PATCH v1] Make building with clang's LTO work on macOS
When building with -flto the backend binary must keep many otherwise
unused symbols to make them available to dynamically loaded modules /
extensions. This has been done via -Wl,--export-dynamic on many
platforms for years. This flag is not supported by Apple's clang,
though. Here it's called -Wl,-export_dynamic instead.
Thus, make configure pick up on this variant of the flag as well. Meson
has the logic to detect this flag built-in, but doesn't support this
variant either. This needs to be raised upstream.
Without this fix, building with -flto fails with errors similar to [1]
and [2]. This happens for all currently live versions, including 17 and
HEAD.
[1]: https://postgr.es/m/1581936537572-0.post%40n3.nabble.com
[2]: https://postgr.es/m/21800.1499270547%40sss.pgh.pa.us
---
configure | 39 +++++++++++++++++++++++++++++++++++++++
configure.ac | 2 ++
2 files changed, 41 insertions(+)
diff --git a/configure b/configure
index 7b03db56a67..4c0a1383428 100755
--- a/configure
+++ b/configure
@@ -19135,6 +19135,7 @@ fi
# For linkers that understand --export-dynamic, add that to the LDFLAGS_EX_BE
# (backend specific ldflags). One some platforms this will always fail (e.g.,
# windows), but on others it depends on the choice of linker (e.g., solaris).
+# macos uses -export_dynamic instead.
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $CC supports -Wl,--export-dynamic, for LDFLAGS_EX_BE" >&5
$as_echo_n "checking whether $CC supports -Wl,--export-dynamic, for LDFLAGS_EX_BE... " >&6; }
if ${pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl___export_dynamic+:} false; then :
@@ -19173,6 +19174,44 @@ if test x"$pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl___export_dynamic" = x"yes"; then
LDFLAGS_EX_BE="${LDFLAGS_EX_BE} -Wl,--export-dynamic"
fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $CC supports -Wl,-export_dynamic, for LDFLAGS_EX_BE" >&5
+$as_echo_n "checking whether $CC supports -Wl,-export_dynamic, for LDFLAGS_EX_BE... " >&6; }
+if ${pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl__export_dynamic+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ pgac_save_LDFLAGS=$LDFLAGS
+LDFLAGS="$pgac_save_LDFLAGS -Wl,-export_dynamic"
+if test "$cross_compiling" = yes; then :
+ pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl__export_dynamic="assuming no"
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+extern void $link_test_func (); void (*fptr) () = $link_test_func;
+int
+main ()
+{
+
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_run "$LINENO"; then :
+ pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl__export_dynamic=yes
+else
+ pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl__export_dynamic=no
+fi
+rm -f core *.core core.conftest.* gmon.out bb.out conftest$ac_exeext \
+ conftest.$ac_objext conftest.beam conftest.$ac_ext
+fi
+
+LDFLAGS="$pgac_save_LDFLAGS"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl__export_dynamic" >&5
+$as_echo "$pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl__export_dynamic" >&6; }
+if test x"$pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl__export_dynamic" = x"yes"; then
+ LDFLAGS_EX_BE="${LDFLAGS_EX_BE} -Wl,-export_dynamic"
+fi
+
# Create compiler version string
diff --git a/configure.ac b/configure.ac
index 63e7be38472..4f5053fe592 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2423,7 +2423,9 @@ fi
# For linkers that understand --export-dynamic, add that to the LDFLAGS_EX_BE
# (backend specific ldflags). One some platforms this will always fail (e.g.,
# windows), but on others it depends on the choice of linker (e.g., solaris).
+# macos uses -export_dynamic instead.
PGAC_PROG_CC_LD_VARFLAGS_OPT(LDFLAGS_EX_BE, [-Wl,--export-dynamic], $link_test_func)
+PGAC_PROG_CC_LD_VARFLAGS_OPT(LDFLAGS_EX_BE, [-Wl,-export_dynamic], $link_test_func)
AC_SUBST(LDFLAGS_EX_BE)
# Create compiler version string
--
2.45.1
On 03.06.24 16:22, Wolfgang Walther wrote:
Building with clang and -flto on macOS currently fails with errors
similar to [1]. This is because the --export-dynamic flag is called
-export_dynamic [2] instead and we have not been passing this variant to
the linker, so far.
It's probably worth clarifying that this option is needed on macOS only
if LTO is also enabled. For standard (non-LTO) builds, the
export-dynamic behavior is already the default on macOS (otherwise
nothing in PostgreSQL would work).
I don't think we explicitly offer LTO builds as part of the make build
system, so anyone trying this would do it sort of self-service, by
passing additional options to configure or make. In which case they
might as well pass the -export_dynamic option along in the same way?
I don't mind addressing this in PG18, but I would hesitate with
backpatching. With macOS, it's always hard to figure out whether these
kinds of options work the same way going versions back.
Peter Eisentraut:
It's probably worth clarifying that this option is needed on macOS only
if LTO is also enabled. For standard (non-LTO) builds, the
export-dynamic behavior is already the default on macOS (otherwise
nothing in PostgreSQL would work).
Right, man page say this:
Preserves all global symbols in main executables during LTO. Without
this option, Link Time Optimization is allowed to inline and remove
global functions. This option is used when a main executable may load a
plug-in which requires certain symbols from the main executable.
Peter:
I don't think we explicitly offer LTO builds as part of the make build
system, so anyone trying this would do it sort of self-service, by
passing additional options to configure or make. In which case they
might as well pass the -export_dynamic option along in the same way?
The challenge is that it defeats the purpose of LTO to pass this along
to everything, e.g. via CFLAGS. The Makefiles set this in LDFLAGS_EX_BE
only, so it only affects the backend binary. This is not at all obvious
and took me quite a while to figure out why LTO silently didn't strip
symbols from other binaries. It does work to explicitly set
LDFLAGS_EX_BE, though.
Also, passing the LTO flag on Linux "just works" (clang, not GCC
necessarily).
I don't mind addressing this in PG18, but I would hesitate with
backpatching. With macOS, it's always hard to figure out whether these
kinds of options work the same way going versions back.
All the versions for ld64 are in [1]https://opensource.apple.com/source/ld64/. It seems this was introduced in
ld64-224.1 [2]https://opensource.apple.com/source/ld64/ld64-224.1/doc/man/man1/ld.1.auto.html the first time. It was not there in ld64-136 [3]https://opensource.apple.com/source/ld64/ld64-136/doc/man/man1/ld.1.auto.html. Finally
the man page has **exactly** the same wording in the latest version
ld64-609 [4]https://opensource.apple.com/source/ld64/ld64-609/doc/man/man1/ld.1.auto.html.
We could go further and compare the source, but I think it's safe to
assume that this flag hasn't changed much and should not affect non-LTO
builds. And for even older versions it would just not be supported, so
configure would not use it.
Best,
Wolfgang
[1]: https://opensource.apple.com/source/ld64/
[2]: https://opensource.apple.com/source/ld64/ld64-224.1/doc/man/man1/ld.1.auto.html
https://opensource.apple.com/source/ld64/ld64-224.1/doc/man/man1/ld.1.auto.html
[3]: https://opensource.apple.com/source/ld64/ld64-136/doc/man/man1/ld.1.auto.html
https://opensource.apple.com/source/ld64/ld64-136/doc/man/man1/ld.1.auto.html
[4]: https://opensource.apple.com/source/ld64/ld64-609/doc/man/man1/ld.1.auto.html
https://opensource.apple.com/source/ld64/ld64-609/doc/man/man1/ld.1.auto.html
Wolfgang Walther:
Peter:
I don't think we explicitly offer LTO builds as part of the make build
system, so anyone trying this would do it sort of self-service, by
passing additional options to configure or make. In which case they
might as well pass the -export_dynamic option along in the same way?The challenge is that it defeats the purpose of LTO to pass this along
to everything, e.g. via CFLAGS. The Makefiles set this in LDFLAGS_EX_BE
only, so it only affects the backend binary. This is not at all obvious
and took me quite a while to figure out why LTO silently didn't strip
symbols from other binaries. It does work to explicitly set
LDFLAGS_EX_BE, though.
Oh, and more importantly: LDFLAGS_EX_BE is not available on all back
branches. It was only introduced in v16 in preparation for meson. So up
to v15, I would have to patch src/makesfiles/Makefile.darwin to set
export_dynamic.
So back-patching a change like this would certainly help to get LTO
across versions seamlessly - which is what I am trying to achieve while
packaging all versions in nixpkgs / NixOS.
Best,
Wolfgang
Hi,
On 2024-06-03 17:07:22 +0200, Wolfgang Walther wrote:
Peter Eisentraut:
It's probably worth clarifying that this option is needed on macOS only
if LTO is also enabled.� For standard (non-LTO) builds, the
export-dynamic behavior is already the default on macOS (otherwise
nothing in PostgreSQL would work).Right, man page say this:
Preserves all global symbols in main executables during LTO. Without this
option, Link Time Optimization is allowed to inline and remove global
functions. This option is used when a main executable may load a plug-in
which requires certain symbols from the main executable.
Gah. Apples tendency to just break stuff that has worked across *nix-y
platforms for decades is pretty annoying. They could just have made
--export-dynamic an alias for --export_dynamic, but no, everyone needs a
special macos thingy in their build scripts.
Peter:
I don't think we explicitly offer LTO builds as part of the make build
system, so anyone trying this would do it sort of self-service, by
passing additional options to configure or make.� In which case they
might as well pass the -export_dynamic option along in the same way?The challenge is that it defeats the purpose of LTO to pass this along to
everything, e.g. via CFLAGS. The Makefiles set this in LDFLAGS_EX_BE only,
so it only affects the backend binary. This is not at all obvious and took
me quite a while to figure out why LTO silently didn't strip symbols from
other binaries. It does work to explicitly set LDFLAGS_EX_BE, though.Also, passing the LTO flag on Linux "just works" (clang, not GCC
necessarily).
It should just work on gcc, or at least has in the recent past.
ISTM if we want to test for -export_dynamic like what you proposed, we should
do so only if --export-dynamic wasn't found. No need to incur the overhead on
!macos.
Greetings,
Andres Freund
On 03.06.24 17:07, Wolfgang Walther wrote:
I don't mind addressing this in PG18, but I would hesitate with
backpatching. With macOS, it's always hard to figure out whether
these kinds of options work the same way going versions back.All the versions for ld64 are in [1]. It seems this was introduced in
ld64-224.1 [2] the first time. It was not there in ld64-136 [3]. Finally
the man page has **exactly** the same wording in the latest version
ld64-609 [4].We could go further and compare the source, but I think it's safe to
assume that this flag hasn't changed much and should not affect non-LTO
builds. And for even older versions it would just not be supported, so
configure would not use it.
With the native compiler tooling on macOS, it is not safe to assume
anything, including that the man pages are accurate or that the
documented options actually work correctly and don't break anything
else. Unless we have actual testing on all the supported macOS
versions, I don't believe it.
Given that LTO apparently never worked on macOS, this is not a
regression, so I wouldn't backpatch it. I'm not objecting, but I don't
want to touch it.
Andres Freund:
Gah. Apples tendency to just break stuff that has worked across *nix-y
platforms for decades is pretty annoying. They could just have made
--export-dynamic an alias for --export_dynamic, but no, everyone needs a
special macos thingy in their build scripts.
Interesting enough my Linux ld does support -export_dynamic, too.. but
it doesn't say anywhere in the man pages or so.
Also, passing the LTO flag on Linux "just works" (clang, not GCC
necessarily).It should just work on gcc, or at least has in the recent past.
Well it "works" in a sense that the build succeeds and check-world as
well. But there are some symbols in all the client binaries that I know
are unused (paths to .../include etc.), and which LLVM's LTO strips out
happily - that are still in there after GCC's LTO.
GCC can remove them with -fdata-sections -ffunction-sections
-fmerge-constants and -Wl,--gc-sections. But not with -flto. At least I
didn't manage to.
ISTM if we want to test for -export_dynamic like what you proposed, we should
do so only if --export-dynamic wasn't found. No need to incur the overhead on
!macos.
Makes sense! v2 attached.
I also attached a .backpatch to show what that would look like for v15
and down.
Peter Eisentraut:
With the native compiler tooling on macOS, it is not safe to assume
anything, including that the man pages are accurate or that the
documented options actually work correctly and don't break anything
else. Unless we have actual testing on all the supported macOS
versions, I don't believe it.
Which macOS versions are "supported"?
I just set up a VM with macOS Mojave (2018) and tested both the .patch
on HEAD as well as the .backpatch on REL_12_STABLE with -flto. Build
passed, make check-world as well.
clang --version for Mojave:
Apple LLVM version 10.0.1 (clang-1001.0.46.4)
Target: x86_64-apple-darwin18.5.0
clang --version for Sonoma (where I tested before):
Apple clang version 15.0.0 (clang-1500.3.9.4)
Target: x86_64-apple-darwin@23.5.0
Since PostgreSQL 12 is from 2019 and Mojave from 2018, I think that's
far enough back?
Given that LTO apparently never worked on macOS, this is not a
regression, so I wouldn't backpatch it. I'm not objecting, but I don't
want to touch it.
Fair enough! Hopefully my testing convinces more than the man pages ;)
Best,
Wolfgang
Attachments:
v2-0001-Make-building-with-clang-s-LTO-work-on-macOS.patchtext/x-patch; charset=UTF-8; name=v2-0001-Make-building-with-clang-s-LTO-work-on-macOS.patchDownload
From 3ca5357bbdb9aae29a1785d5ca2179d6cca15cdd Mon Sep 17 00:00:00 2001
From: Wolfgang Walther <walther@technowledgy.de>
Date: Sun, 2 Jun 2024 10:46:56 +0200
Subject: [PATCH v2] Make building with clang's LTO work on macOS
When building with -flto the backend binary must keep many otherwise
unused symbols to make them available to dynamically loaded modules /
extensions. This has been done via -Wl,--export-dynamic on many
platforms for years. This flag is not supported by Apple's clang,
though. Here it's called -Wl,-export_dynamic instead.
Thus, make configure pick up on this variant of the flag as well. Meson
has the logic to detect this flag built-in, but doesn't support this
variant either. This needs to be raised upstream.
Without this fix, building with -flto fails with errors similar to [1]
and [2]. This happens for all currently live versions, including 17 and
HEAD.
[1]: https://postgr.es/m/1581936537572-0.post%40n3.nabble.com
[2]: https://postgr.es/m/21800.1499270547%40sss.pgh.pa.us
---
configure | 41 +++++++++++++++++++++++++++++++++++++++++
configure.ac | 4 ++++
2 files changed, 45 insertions(+)
diff --git a/configure b/configure
index 7b03db56a67..771dcfbdef9 100755
--- a/configure
+++ b/configure
@@ -19135,6 +19135,7 @@ fi
# For linkers that understand --export-dynamic, add that to the LDFLAGS_EX_BE
# (backend specific ldflags). One some platforms this will always fail (e.g.,
# windows), but on others it depends on the choice of linker (e.g., solaris).
+# macos uses -export_dynamic instead.
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $CC supports -Wl,--export-dynamic, for LDFLAGS_EX_BE" >&5
$as_echo_n "checking whether $CC supports -Wl,--export-dynamic, for LDFLAGS_EX_BE... " >&6; }
if ${pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl___export_dynamic+:} false; then :
@@ -19173,6 +19174,46 @@ if test x"$pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl___export_dynamic" = x"yes"; then
LDFLAGS_EX_BE="${LDFLAGS_EX_BE} -Wl,--export-dynamic"
fi
+if test x"$LDFLAGS_EX_BE" = x""; then
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether $CC supports -Wl,-export_dynamic, for LDFLAGS_EX_BE" >&5
+$as_echo_n "checking whether $CC supports -Wl,-export_dynamic, for LDFLAGS_EX_BE... " >&6; }
+if ${pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl__export_dynamic+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ pgac_save_LDFLAGS=$LDFLAGS
+LDFLAGS="$pgac_save_LDFLAGS -Wl,-export_dynamic"
+if test "$cross_compiling" = yes; then :
+ pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl__export_dynamic="assuming no"
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+extern void $link_test_func (); void (*fptr) () = $link_test_func;
+int
+main ()
+{
+
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_run "$LINENO"; then :
+ pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl__export_dynamic=yes
+else
+ pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl__export_dynamic=no
+fi
+rm -f core *.core core.conftest.* gmon.out bb.out conftest$ac_exeext \
+ conftest.$ac_objext conftest.beam conftest.$ac_ext
+fi
+
+LDFLAGS="$pgac_save_LDFLAGS"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl__export_dynamic" >&5
+$as_echo "$pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl__export_dynamic" >&6; }
+if test x"$pgac_cv_prog_cc_LDFLAGS_EX_BE__Wl__export_dynamic" = x"yes"; then
+ LDFLAGS_EX_BE="${LDFLAGS_EX_BE} -Wl,-export_dynamic"
+fi
+
+fi
# Create compiler version string
diff --git a/configure.ac b/configure.ac
index 63e7be38472..72e3c1d429b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2423,7 +2423,11 @@ fi
# For linkers that understand --export-dynamic, add that to the LDFLAGS_EX_BE
# (backend specific ldflags). One some platforms this will always fail (e.g.,
# windows), but on others it depends on the choice of linker (e.g., solaris).
+# macos uses -export_dynamic instead.
PGAC_PROG_CC_LD_VARFLAGS_OPT(LDFLAGS_EX_BE, [-Wl,--export-dynamic], $link_test_func)
+if test x"$LDFLAGS_EX_BE" = x""; then
+ PGAC_PROG_CC_LD_VARFLAGS_OPT(LDFLAGS_EX_BE, [-Wl,-export_dynamic], $link_test_func)
+fi
AC_SUBST(LDFLAGS_EX_BE)
# Create compiler version string
--
2.45.1
v2-0001-Make-building-with-clang-s-LTO-work-on-macOS.backpatchtext/plain; charset=UTF-8; name=v2-0001-Make-building-with-clang-s-LTO-work-on-macOS.backpatchDownload
--- a/src/makefiles/Makefile.darwin
+++ b/src/makefiles/Makefile.darwin
@@ -5,6 +5,8 @@ DLSUFFIX = .so
# env var name to use in place of LD_LIBRARY_PATH
ld_library_path_var = DYLD_LIBRARY_PATH
+export_dynamic = -Wl,-export_dynamic
+
ifdef PGXS
BE_DLLLIBS = -bundle_loader $(bindir)/postgres
else
Peter Eisentraut <peter@eisentraut.org> writes:
With the native compiler tooling on macOS, it is not safe to assume
anything, including that the man pages are accurate or that the
documented options actually work correctly and don't break anything
else. Unless we have actual testing on all the supported macOS
versions, I don't believe it.
Relevant to this: I wonder what we think the supported macOS versions
are, anyway. AFAICS, the buildfarm only covers current (Sonoma)
and current-1 (Ventura) major versions, and only the latest minor
versions in those OS branches.
I share Peter's unwillingness to assume that Apple hasn't randomly
fixed or broken stuff across toolchain versions. Their track record
fully justifies that lack of trust.
regards, tom lane
On 04.06.24 18:41, Tom Lane wrote:
Relevant to this: I wonder what we think the supported macOS versions
are, anyway. AFAICS, the buildfarm only covers current (Sonoma)
and current-1 (Ventura) major versions, and only the latest minor
versions in those OS branches.
For other OS lines I think we are settling on supporting what the OS
vendor supports. So for macOS at the moment this would be current,
current-1, and current-2, per
<https://en.wikipedia.org/wiki/MacOS_version_history#Releases>.
Peter Eisentraut:
On 04.06.24 18:41, Tom Lane wrote:
Relevant to this: I wonder what we think the supported macOS versions
are, anyway. AFAICS, the buildfarm only covers current (Sonoma)
and current-1 (Ventura) major versions, and only the latest minor
versions in those OS branches.For other OS lines I think we are settling on supporting what the OS
vendor supports. So for macOS at the moment this would be current,
current-1, and current-2, per
<https://en.wikipedia.org/wiki/MacOS_version_history#Releases>.
So I tested both HEAD and v12 on current and current-5, both successful.
That should cover current-1 and current-2, too. If you want me to test
any other macOS versions inbetween, or any other PG versions, I can do that.
I would really like to upstream those kind of patches and see them
backpatched - otherwise we need to carry around those patches for up to
5 years in the distros. And in light of the discussion in [1]/messages/by-id/flat/ZgdCpFThi9ODcCsJ@momjian.us my goal is
to reduce the number of patches carried to a minimum. Yes - those
patches are simple enough - but the more patches you have, the less
likely you are going to spot a malicious patch inbetween.
Best,
Wolfgang
Hi,
So I tested both HEAD and v12 on current and current-5, both successful.
That should cover current-1 and current-2, too. If you want me to test
any other macOS versions inbetween, or any other PG versions, I can do that.I would really like to upstream those kind of patches and see them
backpatched - otherwise we need to carry around those patches for up to
5 years in the distros. And in light of the discussion in [1] my goal is
to reduce the number of patches carried to a minimum. Yes - those
patches are simple enough - but the more patches you have, the less
likely you are going to spot a malicious patch inbetween.
The patch was marked as "Needs review" so I decided to take a look at it.
I tested v2-0001 on macOS Sonoma 14.5 with Autotools.
configure said:
```
checking whether gcc supports -Wl,--export-dynamic, for LDFLAGS_EX_BE... no
checking whether gcc supports -Wl,-export_dynamic, for LDFLAGS_EX_BE... yes
```
I also checked that -Wl,-export_dynamic was used when linking postgres binary.
On Linux configure says:
```
checking whether gcc supports -Wl,--export-dynamic, for LDFLAGS_EX_BE... yes
```
... and `-Wl,--export-dynamic` is used when linking postgres.
cfbot is happy with the patch too.
There is not much to say about the code. It's Autotools and it's ugly,
but it gets the job done.
It seems to me that the patch is not going to become any better and it
doesn't need any more attention from the reviewers. Thus I changed the
status of the CF entry to "Ready for Committer".
--
Best regards,
Aleksander Alekseev
Aleksander Alekseev <aleksander@timescale.com> writes:
It seems to me that the patch is not going to become any better and it
doesn't need any more attention from the reviewers. Thus I changed the
status of the CF entry to "Ready for Committer".
So ... there is quite a disconnect between what this patch actually
does (i.e., probe to see if "-Wl,-export_dynamic" is accepted) and
the title of this thread. I wouldn't have much of a problem with
the patch in isolation. However, what Apple's man page for ld(1)
says is
-export_dynamic
Preserves all global symbols in main executables during LTO.
Without this option, Link Time Optimization is allowed to inline
and remove global functions. This option is used when a main
executable may load a plug-in which requires certain symbols from
the main executable.
which agrees with Wolfgang's comment that it doesn't do much unless
you enable LTO. So that raises two questions:
1. If you're going to manually inject -flto, seems like you could
manually inject -Wl,-export_dynamic too, so why do you need this
patch?
2. Do we really want to encourage people to build with -flto?
I fear that #2 is actually a pretty serious concern. I think there
are a lot of places where we've assumed semi-implicitly that
compilation file boundaries are optimization barriers, particularly
around stuff like LWLocks and semaphores. I don't really want to
spend time chasing obscure, irreproducible bugs that may appear when
that assumption gets broken. I especially don't want to do it just
because some packager has randomly decided to inject random build
switches.
In short: if we want to support LTO, let's do it officially and not
by the back door. But I think somebody needs to make the case that
there are compelling benefits that would justify the nontrivial
amount of risk and work that may ensue. My default position here
is "sorry, we don't support that".
regards, tom lane
Re: Tom Lane
I fear that #2 is actually a pretty serious concern. I think there
are a lot of places where we've assumed semi-implicitly that
compilation file boundaries are optimization barriers, particularly
around stuff like LWLocks and semaphores. I don't really want to
spend time chasing obscure, irreproducible bugs that may appear when
that assumption gets broken. I especially don't want to do it just
because some packager has randomly decided to inject random build
switches.
Ubuntu enabled -ftlo=auto by default in 22.04, so it has been around
for some time already.
$ dpkg-buildflags
CFLAGS=-g -O2 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -ffile-prefix-map=... -flto=auto -ffat-lto-objects -fstack-protector-strong -fstack-clash-protection -Wformat -Werror=format-security -fcf-protection -fdebug-prefix-map=...
Christoph
Hi,
On 2024-07-19 11:06:47 -0400, Tom Lane wrote:
2. Do we really want to encourage people to build with -flto?
I fear that #2 is actually a pretty serious concern. I think there
are a lot of places where we've assumed semi-implicitly that
compilation file boundaries are optimization barriers, particularly
around stuff like LWLocks and semaphores. I don't really want to
spend time chasing obscure, irreproducible bugs that may appear when
that assumption gets broken. I especially don't want to do it just
because some packager has randomly decided to inject random build
switches.
I don't really buy this argument. It'd be one thing if compilation boundaries
actually provided hard guarantees - but they don't, the CPU can reorder things
as well, not just the compiler. And the CPU doesn't know about compilation
units.
If anything, compiler reorderings are *less* obscure than CPU reordering,
because the latter is heavily dependent on running on large enough machines
with specific microarchitectures.
The only case I know where we do rely on compilation units providing some
level of boundaries is on compilers where we don't know how to emit a compiler
barrier. That's probably a fallback we ought to remove one of these days...
In short: if we want to support LTO, let's do it officially and not
by the back door. But I think somebody needs to make the case that
there are compelling benefits that would justify the nontrivial
amount of risk and work that may ensue. My default position here
is "sorry, we don't support that".
FWIW, I've seen pretty substantial wins, particularly in more heavyweight
queries.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2024-07-19 11:06:47 -0400, Tom Lane wrote:
2. Do we really want to encourage people to build with -flto?
The only case I know where we do rely on compilation units providing some
level of boundaries is on compilers where we don't know how to emit a compiler
barrier. That's probably a fallback we ought to remove one of these days...
Hm. We've moved our platform/toolchain goalposts far enough in the
last few releases that that might not be too big a lift. Do you
know offhand which supported platforms still have a problem there?
(mumble AIX mumble)
regards, tom lane
Hi,
On 2024-07-19 15:36:29 -0400, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2024-07-19 11:06:47 -0400, Tom Lane wrote:
2. Do we really want to encourage people to build with -flto?
The only case I know where we do rely on compilation units providing some
level of boundaries is on compilers where we don't know how to emit a compiler
barrier. That's probably a fallback we ought to remove one of these days...Hm. We've moved our platform/toolchain goalposts far enough in the
last few releases that that might not be too big a lift. Do you
know offhand which supported platforms still have a problem there?(mumble AIX mumble)
In 16 it looks like the only case might indeed have been [drumroll] AIX with
xlc (with gcc . And there it it looks like it'd have been trivial to implement
[1]: I think it'd just be __fence(). Looks like it's been present for a while, found it in "IBM XL C/C++ for AIX, V10.1 Compiler Reference Version 10.1", which looks to be from 2008.
We've been talking about requiring 32 bit atomics and a spinlock
implementation - this imo fits in well with that, without proper barriers it's
pretty much impossible to have correct spinlocks and, even more so, any lock
free construct, of which we have a bunch.
IOW, let's rip out the fallback implementation for compiler and memory
barriers and fix the fallout, if there is any.
Greetings,
Andres Freund
[1]: I think it'd just be __fence(). Looks like it's been present for a while, found it in "IBM XL C/C++ for AIX, V10.1 Compiler Reference Version 10.1", which looks to be from 2008.
found it in "IBM XL C/C++ for AIX, V10.1 Compiler Reference Version 10.1",
which looks to be from 2008.
Hi,
So ... there is quite a disconnect between what this patch actually
does (i.e., probe to see if "-Wl,-export_dynamic" is accepted) and
the title of this thread. [...]
The thread title is indeed somewhat misleading, I was initially
puzzled by it too. The actual idea, if I understood it correctly, is
merely to do on MacOS the same we currently do on Linux.
--
Best regards,
Aleksander Alekseev
On Sat, Jul 20, 2024 at 7:56 AM Andres Freund <andres@anarazel.de> wrote:
On 2024-07-19 15:36:29 -0400, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2024-07-19 11:06:47 -0400, Tom Lane wrote:
2. Do we really want to encourage people to build with -flto?
The only case I know where we do rely on compilation units providing some
level of boundaries is on compilers where we don't know how to emit a compiler
barrier. That's probably a fallback we ought to remove one of these days...Hm. We've moved our platform/toolchain goalposts far enough in the
last few releases that that might not be too big a lift. Do you
know offhand which supported platforms still have a problem there?(mumble AIX mumble)
In 16 it looks like the only case might indeed have been [drumroll] AIX with
xlc (with gcc . And there it it looks like it'd have been trivial to implement
[1].We've been talking about requiring 32 bit atomics and a spinlock
implementation - this imo fits in well with that, without proper barriers it's
pretty much impossible to have correct spinlocks and, even more so, any lock
free construct, of which we have a bunch.IOW, let's rip out the fallback implementation for compiler and memory
barriers and fix the fallout, if there is any.
I'll incorporate that into the next version of:
/messages/by-id/3351991.1697728588@sss.pgh.pa.us
... with a view to committing in the next few days.
(Ignore the <stdatomic.h> patch, that's just an experiment for now,
but it's not part of what I plan to commit.)
On 19.07.24 12:40, Aleksander Alekseev wrote:
It seems to me that the patch is not going to become any better and it
doesn't need any more attention from the reviewers. Thus I changed the
status of the CF entry to "Ready for Committer".
I'm happy to commit this patch.
I checked that for non-LTO builds, this option does not change the
output binary, so it seems harmless in that sense.
An equivalent change has recently been merged into meson upstream, so
we'll get the same behavior on meson before long.
The argument "If you're going to manually inject -flto, seems like you
could manually inject -Wl,-export_dynamic too, so why do you need this
patch?" is true, but the behavior that the link fails unless you use
both options is pretty surprising, so this is a small quality of life
improvement. Also, it seems that LTO use is already in the wild, so it
seems sensible to make that easier to exercise during development too.
Maybe a configure --enable-lto option would be sensible, but that can be
a separate patch.
On 22.07.24 16:04, Peter Eisentraut wrote:
On 19.07.24 12:40, Aleksander Alekseev wrote:
It seems to me that the patch is not going to become any better and it
doesn't need any more attention from the reviewers. Thus I changed the
status of the CF entry to "Ready for Committer".I'm happy to commit this patch.
I checked that for non-LTO builds, this option does not change the
output binary, so it seems harmless in that sense.An equivalent change has recently been merged into meson upstream, so
we'll get the same behavior on meson before long.
Done.