Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macOS linker warnings in macOS ventura #97524

Open
pablogsal opened this issue Sep 24, 2022 · 55 comments
Open

macOS linker warnings in macOS ventura #97524

pablogsal opened this issue Sep 24, 2022 · 55 comments
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes OS-mac

Comments

@pablogsal
Copy link
Member

Turns out ld shipped with Xcode 14+ emits a warning when using -undefined dynamic_lookup.`

ld: warning: -undefined dynamic_lookup may not work with chained fixups

Some investigation reveals that in fact -undefined dynamic_lookup doesn't work when:

  • Link a shared library with the option
  • Link it with a program that uses the chained-fixup introduced from macOS 12 and iOS 15

This is because -undefined dynamic_lookup uses lazy-bindings and they won't be bound while dyld fixes-up by traversing chained-fixup info.

However, we build extension modules with as bundles (-bundle) and they are loaded only through
dlopen, so it's safe to use -undefined dynamic_lookup in theory. So the warning produced by ld64 is
likely to be a false-positive.

ld64 also provides the -bundle_loader <executable> option, which allows resolving symbols defined in the executable symbol table while linking. It behaves almost the same with -undefined dynamic_lookup, but it makes the following changes:

  • Require that unresolved symbols among input objects must be defined in the executable.
  • Lazy symbol binding will lookup only the symbol table of the bundle loader executable. (-undefined dynamic_lookup lookups all symbol tables as flat namespace)

See "New Features" subsection under "Linking" section for chained fixup
developer.apple.com/documentation/xcode-release-notes/xcode-13-release-notes for more information.

Also, check the same problem in ruby where most of this information is taken from.

@pablogsal pablogsal added OS-mac 3.11 only security fixes 3.10 only security fixes 3.9 only security fixes 3.12 bugs and security fixes labels Sep 24, 2022
@pablogsal
Copy link
Member Author

CC @ambv @ned-deily

@pablogsal
Copy link
Member Author

Unfortunately this warning also propagates to all extension modules built with the given Python.

@ned-deily
Copy link
Member

@ronaldoussoren

@ronaldoussoren
Copy link
Contributor

-bundle_loader doesn't work with shared library builds (including the framework build), as the option does exactly as described in the documentation: It looks for symbols in the loader executable, not in shared library dependencies of that binary. I found this when looking into -bundle_loader to get link time errors when using non-existing symbols instead of only getting those when using an extension.

Linking with libpython is also problematic because on macOS shared libraries are referenced by absolute path which means linking with libpython ties the extension to a specific interpreter (e.g. makes it impossible to build an extension using the Python.org installer and load it using a homebrew install). For the user this will result in seemingly random crashes (due to libpython being loaded twice while only one is initialised).

I haven't spend time on looking into this particular issue yet, with some luck there's another option beyond disabling chained lookups.

@CsBigDataHub
Copy link

FYI.. https://issues.guix.gnu.org/issue/57849

Looks like using -Wl,-w will help.

@ronaldoussoren
Copy link
Contributor

FYI.. https://issues.guix.gnu.org/issue/57849

Looks like using -Wl,-w will help.

That doesn't really help, it just suppresses the warning.

@keith
Copy link

keith commented Oct 6, 2022

You can pass -Wl,-no_fixup_chains to disable them entirely for now, but that flag is undocumented and who knows if it will exist in the future.

@pablogsal
Copy link
Member Author

You can pass -Wl,-no_fixup_chains to disable them entirely for now, but that flag is undocumented and who knows if it will exist in the future.

It will likely won't because chained fixups are the new linker information for the dynamic symbol tables. So we will be just delaying the inevitable

@ronaldoussoren
Copy link
Contributor

Another workaround is to target an older deployment target (before 11.0), but that's also not a long term solution.

@ronaldoussoren
Copy link
Contributor

For static builds using -Wl,-bundle_loader,$(BUILDPYTHON) -Wl,-undefined,error appears to work. I've manually patched the Makefile for some extensions and that does result in a build without warnings and I can load those extensions. What I've not checked yet is if those extensions work with a python installed into a different prefix.

This doesn't work with dynamic builds though, even when using BLDLIBRARY= -L. -Wl,-reexport-lpython$(LDVERSION) when linking $(BUILDPYTHON).

Note that just linking extensions with libpython isn't a realistic option because that results in C extensions that target a specific python prefix and hence would loose interoperability for wheels build with our installer and homebrew.

@ronaldoussoren
Copy link
Contributor

What does work for a dynamic build (again with a tweaked $(BLDLBIRARY): -Wl,-flat_namespace -Wl,-bundle_loader,./python.exe -Wl,-undefined,error, that is add -Wl,-flat_namespace. That's not ideal though because the default two-level namespaces are more efficient (and faster).

Something similar should also work for framework builds (but using a different reexport flag), but I haven't tested this yet. Mostly because I'm not happy with the link flags I've used for a shared build.

All of this was tested by manual patching of the Makefile, I haven't looked into tweaking the configure script and makefile template yet.

@wjakob
Copy link
Contributor

wjakob commented Nov 1, 2022

Hi — this issue has also been raised in the pybind11 and CMake projects, including a fix similar to what was suggested above by @ronaldoussoren.

Links:

All of this raises a few questions (pybind/pybind11#4301 (comment)) that would be good to figure out together with somebody who is familiar with the intricacies of Darwin's dynamic linker.

@pablogsal
Copy link
Member Author

pablogsal commented Nov 1, 2022

Hi @wjakob,

We are currently waiting for some clarifications from Apple. Will follow up with anything we find.

@mathstuf
Copy link

mathstuf commented Nov 1, 2022

Anyone with Apple clout could push this PR into their attention sphere: apple-opensource/ld64#1

It basically says "here's a library; make any symbol that is resolved here dynamic_lookup and in the flat namespace". Which is…exactly what is wanted here.

@wjakob
Copy link
Contributor

wjakob commented Nov 1, 2022

It basically says "here's a library; make any symbol that is resolved here dynamic_lookup and in the flat namespace". Which is…exactly what is wanted here.

Correct me if I'm wrong, but it seems to me like this PR targets a different use case: it enables dynamic_lookup selectively for chosen symbols to avoid the "sledgehammer" solution of changing the defaults for all undefined symbols.

While the issue discussed here is that the very mechanism that drives dynamic_lookup no longer works in binaries using the new "chained fixups" linker feature (which is automatically enabled for newer macOS deployment targets).

@wjakob
Copy link
Contributor

wjakob commented Nov 1, 2022

One more data point in case it is useful: explicitly enumerating all relevant Python symbols using the -U command line arguments (wjakob/nanobind@bb211d1) also works and the warnings are gone. But it leaves me wondering:

  • Does it actually fix the issue? The manpage says that dynamic_lookup is still used in that case.
  • The linker gets a huge command line argument (3.5KB), which seems quite ugly and would need to be tweaked for every Python version. I could not find a -U_from_file <filename> argument to batch-inform ld about the symbols to accept as being undefined.

@carlocab
Copy link

carlocab commented Nov 1, 2022

I could not find a -U_from_file <filename> argument to batch-inform ld about the symbols to accept as being undefined.

ld response files might be useful here (though it is documented under Rarely used Options):

     @response_file_path
             Inserts contents of file at response_file_path into arguments. This allows for linker command line args to be store in a
             file.  Note: ld is normally invoked through clang, and clang also interprets @file on the command line.  To have clang
             ignore the @file and pass it through to ld, use -Wl,@file.

@ambv
Copy link
Contributor

ambv commented Mar 8, 2023

Thanks @wjakob, this is a clear actionable item for us. I think I prefer having an explicit list like this and retaining chained fixups.

However, I imagine we will likely want to create this symbol list ourselves "just-in-time" in our build tooling to future-proof it without creating an external dependency. 3.12 is still in flux so the list might change, and 3.13 is around the corner.

@mathstuf
Copy link

mathstuf commented Mar 8, 2023

A more future-proof way of handling missing symbols would be poking the right people at Apple to support the functionality in this PR which allows one to say "here is a library for which any symbol found should be treated as if given via -U and do not add the library to the load commands".

@pablogsal
Copy link
Member Author

However, I imagine we will likely want to create this symbol list ourselves "just-in-time" in our build tooling to future-proof it without creating an external dependency. 3.12 is still in flux so the list might change, and 3.13 is around the corner.

We can create it and package it (and use when building the interpreter) but extensions need to pick it up using whatever build system they are using, no?

@wjakob
Copy link
Contributor

wjakob commented Mar 14, 2023

We can create it and package it (and use when building the interpreter) but extensions need to pick it up using whatever build system they are using, no?

That was the reason for packaging it up in my binding project. Of course, the implication wasn't that CPython should rely on this external link.

Probably the most portable way would be if the Python binary itself could be queried to list its own exported symbols.

neverpanic referenced this issue in emacs-mirror/emacs May 31, 2023
* lisp/emacs-lisp/comp.el (native-comp-driver-options): Add "-Wl,-w"
on Darwin systems.
* etc/NEWS: Describe change.
maths22 added a commit to maths22/nokogiri that referenced this issue Aug 21, 2023
See comment in body for details on why and how it can be replaced.

The python community has been running into the same issues because
cpython extensions operate very similarly to ruby.  They got an
explanation from Apple on some of how the changes work which can be
found here and gives some additional useful context for understanding
this change:
python/cpython#97524 (comment)
@Eclips4 Eclips4 added the 3.13 bugs and security fixes label Oct 10, 2023
FlashSheridan added a commit to FlashSheridan/alive2 that referenced this issue Oct 17, 2023
…ace”

error and
ld: warning: -undefined dynamic_lookup may not work with chained fixups.

python/cpython#97524 (comment) Ronald Oussoren
> Another workaround is to target an older deployment target (before 11.0), but that's also not a long term solution.

This works and seems needed on my ARM M1 MacOS 12.7 21G816, when compiling with either Apple clang-1400.0.29.202
or Homebrew clang version 16.0.5.  The discussion there suggests that the Clang in Xcode 14.3 might fix this, but that
won’t run on my MacOS.
FlashSheridan added a commit to FlashSheridan/alive2 that referenced this issue Oct 18, 2023
chained fixups linker warning and subsequent “symbol not found in flat namespace” crash.
Thanks to Ronald Oussoren (python/cpython#97524 (comment)).

This works and seems needed on my ARM M1 MacOS 12.7 21G816, when compiling with either Apple clang-1400.0.29.202
or Homebrew clang version 16.0.5.  The discussion there suggests that the Clang in Xcode 14.3 might fix this, but that
won’t run on my MacOS.

I’m not submitting my change to the CMake file (7c642778),
since it presumably isn’t affecting most users and might be a performance hit.  Feel free to pull it if you disagree.
nunoplopes pushed a commit to AliveToolkit/alive2 that referenced this issue Oct 19, 2023
* Example for Mac with the new pass manager and a real file.

* Troubleshooting suggestion for Mac dynamic_lookup/
chained fixups linker warning and subsequent “symbol not found in flat namespace” crash.
Thanks to Ronald Oussoren (python/cpython#97524 (comment)).

This works and seems needed on my ARM M1 MacOS 12.7 21G816, when compiling with either Apple clang-1400.0.29.202
or Homebrew clang version 16.0.5.  The discussion there suggests that the Clang in Xcode 14.3 might fix this, but that
won’t run on my MacOS.

I’m not submitting my change to the CMake file (FlashSheridan@7c642778),
since it presumably isn’t affecting most users and might be a performance hit.  Feel free to pull it if you disagree.
FlashSheridan added a commit to FlashSheridan/alive2 that referenced this issue Oct 25, 2023
…ace”

error and
ld: warning: -undefined dynamic_lookup may not work with chained fixups.

python/cpython#97524 (comment) Ronald Oussoren
> Another workaround is to target an older deployment target (before 11.0), but that's also not a long term solution.

This works and seems needed on my ARM M1 MacOS 12.7 21G816, when compiling with either Apple clang-1400.0.29.202
or Homebrew clang version 16.0.5.  The discussion there suggests that the Clang in Xcode 14.3 might fix this, but that
won’t run on my MacOS.
leafcathead pushed a commit to leafcathead/ghc that referenced this issue Dec 6, 2023
Recent versions of MacOS use a version of ld where `-fixup_chains` is on by default.
This is incompatible with our usage of `-undefined dynamic_lookup`. Therefore we
explicitly disable `fixup-chains` by passing `-no_fixup_chains` to the linker on
darwin. This results in a warning of the form:

ld: warning: -undefined dynamic_lookup may not work with chained fixups

The manual explains the incompatible nature of these two flags:

     -undefined treatment
             Specifies how undefined symbols are to be treated. Options are: error, warning,
             suppress, or dynamic_lookup.  The default is error. Note: dynamic_lookup that
             depends on lazy binding will not work with chained fixups.

A relevant ticket is #22429

Here are also a few other links which are relevant to the issue:

Official comment: https://developer.apple.com/forums/thread/719961

More relevant links:

https://openradar.appspot.com/radar?id=5536824084660224

python/cpython#97524

Note in release notes: https://developer.apple.com/documentation/xcode-release-notes/xcode-13-releas    e-notes

(cherry picked from commit 8c0ea25)
@ronaldoussoren
Copy link
Contributor

However, I imagine we will likely want to create this symbol list ourselves "just-in-time" in our build tooling to future-proof it without creating an external dependency. 3.12 is still in flux so the list might change, and 3.13 is around the corner.

We can create it and package it (and use when building the interpreter) but extensions need to pick it up using whatever build system they are using, no?

With some luck most build systems would pick up flags from python3-config or the equivalent information in sysconfig.

#103306 contains a related discussion about using -U flags, although as only one of the options to drop -undefined dynamic_lookup to improve the build experience (e.g. no silent failure due to missing symbols). I'll close that other issue because this issue is a lot clearer about the way forward.

@ronaldoussoren
Copy link
Contributor

ronaldoussoren commented Jan 13, 2024

Generating the file should be easy enough, e.g.:

libpython.sym: $(LDLIBRARY)
     nm $(LDLIBRARY) | grep ' T ' | awk '{ nm = substr($3, 2); print("-U ", nm) }'  > libpython.sym

I have not yet tried to actually use this though, let alone thought about the correct way to integrate this into the installation and build system.

This will list more symbols than are technically part of the API, but keeps us closer to the current linking behaviour w.r.t. using private but exported symbols. That's not ideal, but does avoid bug reports about link errors on macOS that don't happen on other unix-y platforms.

@Dave-Allured
Copy link

I submitted an upstream patch to GNU libtool, to add -Wl,-no_fixup_chains for Mac builds. If accepted, this will hopefully propagate to many packages using libtool, and mitigate that lookup warning. I welcome any feedback, here or on that GNU ticket.

This would mean that packages wanting to use chained fixups would need to override this new libtool default.

@Dave-Allured
Copy link

Sorry, I mean to include that link:
https://savannah.gnu.org/support/?111069

rv-jenkins pushed a commit to runtimeverification/llvm-backend that referenced this issue Jul 19, 2024
This PR fixes two subtle, related issues that are blocking updates from
going through downstream in the Kasmer project. At a high level, the
issues are:
- Flat namespace linking on macOS produces incorrect symbol lookups in
dynamic libraries.
- #1097 misses a
subtle edge case related to tail-call optimisation.

The actual code changes required are small, but warrant some detailed
explanation.

## Flat Namespaces

For a long time, macOS has implemented a system known as _two-level_
namespaces, whereby undefined symbol names in a dynamic library are
prefixed with the name of the library in which the loader expects to be
able to find them at run-time. This is a conservative behaviour; even if
a symbol with the same name exists in a different library, it won't be
selected. For example, the dynamic libraries built by `llvm-kompile` in
`c` mode link against `libgmp`. Two-level namespaces produce dynamic
symbol tables that look like:
```console
$ dyld_info test/c/Output/flat-namespace.kore.tmp.dir/libtest.so -symbolic_fixups | grep gmpz_clear
           +0x2B28      bind pointer   libgmp.10.dylib/___gmpz_clear
```

This behaviour is different to Linux, which does not have a notion of
two-level namespaces. For legacy compatibility purposes, Apple supply a
linker flag `-flat_namespace` that behaves more similarly to Linux
behaviour. Its use is discouraged in new code, but we had enabled it to
work around an issue in the Python bindings
(python/cpython#97524) that should be fixed in
a future CPython / macOS combination.[^1] When enabled, the symbol table
looks something like this for the same example:
```console
$ dyld_info test/c/Output/flat-namespace.kore.tmp.dir/libtest.so -symbolic_fixups | grep gmpz_clear
           +0x2EE8      bind pointer   flat-namespace/___gmpz_clear
```

As a consequence of this, if the symbol `___gmpz_clear` exists in
multiple dynamic libraries loaded by the same process, then the order in
which they will be selected by the dynamic loader is not clearly
well-defined,[^2] and when it's referenced we could end up loading
either the correct or the incorrect symbol. This caused the initial bug
observed as follows:[^3]
- The Haskell backend statically links the `kore-rpc-booster` executable
against `libgmp`, meaning that some GMP symbols appear in that binary.
- The backend compiles shared libraries that dynamically link against
`libgmp`.
- `kore-rpc-booster` dynamically loads one of these libraries, and when
resolving symbols to load, the flat namespace environment selects the
static version for some and the dynamic version for others.
- A call to `__gmpz_clear` from a backend hook ends up referencing the
statically linked symbol, rather than the dynamically linked version.
Generally, I think this situation is harmless - GMP is very stable and
it's plausible that doing this for most symbols is not observable.
- However, the dynamically-linked GMP library has been set up to use the
KORE memory management functions. When the static version is called, it
tries to `free()` a pointer allocated by the backend's GC, and crashes.

The fix for this issue is to drop our usage of `-flat_namespace` for C
shared libraries compiled by the backend. This breaks a few places we
were relying on the old (incorrect) behaviour in the presence of C++
RTTI; having multiple instances of identically-named typeinfo symbols in
a process is known to be broken there:
- `libunwind` is actually implicitly linked via the macOS system
library; if we explicitly link it as well, then code that handles
exceptions will break.
- The `k-rule-apply` tool linked two copies of the KORE AST library,
causing `dynamic_cast` to break. #1110 addresses this.

## Tail-Call Optimisation

In #1097, we made some changes that explicitly mark K functions as
`musttail` when we know they're tail recursive. In doing so, we removed
the need to use the `-tailcallopt` flag in most cases. However, the
change in that PR missed that as well as IR-level transformations,
`-tailcallopt` sets a lower-level flag in the backend[^4] code generator
that guarantees tail-call code generation. For large programs, this
meant I could observe stack overflows when traversing large terms.

The fix is just to enforce that this internal option gets set properly;
doing so is just a restoration of the behaviour we got from
`-tailcallopt` before.

[^1]: But isn't yet fixed, unfortunately - the underlying bug is still
present on my system. Should be revisited in the future, ideally!
[^2]: It might be defined somewhere, but the initial manifestation of
this bug appeared in an apparently unrelated commit, so I think we were
just getting lucky previously. The fix in this PR is morally correct
whether or not things worked accidentally beforehand.
[^3]: I intend to write this up fully later in a separate issue.
[^4]: As in the X86 or arm backend of LLVM itself.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes OS-mac
Projects
None yet
Development

No branches or pull requests