Thread: Build with LTO / -flto on macOS
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. Attached patch fixes that for configure/make. CC: Tom, who hit the same in [3] 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]: https://postgr.es/m/1581936537572-0.post%40n3.nabble.com [2]: https://opensource.apple.com/source/ld64/ld64-609/doc/man/man1/ld.1.auto.html (grep for export_dynamic) [3]: https://postgr.es/m/21800.1499270547%40sss.pgh.pa.us
Attachment
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]. 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. 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 [3]: 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
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
Attachment
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] 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 [1]: https://postgr.es/m/flat/ZgdCpFThi9ODcCsJ%40momjian.us
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].
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.
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: https://www.postgresql.org/message-id/flat/3351991.1697728588%40sss.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.