Repository navigation
Conversation
… Windows LinkerOptions.treeshake lists all symbolsToKeep in the generated module-definition file, and link.exe fails if one of them is not defined. Read the symbol tables of the input archives and only export the symbols they define.
PR HealthBreaking changes ✔️
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3726 +/- ##
==========================================
+ Coverage 87.93% 87.99% +0.05%
==========================================
Files 310 311 +1
Lines 19799 19894 +95
==========================================
+ Hits 17411 17506 +95
Misses 2388 2388
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Package publishingIf you have publishing permissions, you can use the links below to publish the changes after merging this PR.
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
…l is defined LinkerOptions.manual passes an /INCLUDE: flag per symbol to keep, which fails with LNK2001 for an undefined symbol just like an export in the generated module-definition file does, so filter both to the symbols the input archives define. If none of the symbols to keep is defined, the library would export nothing, which is almost always a mistake such as a symbol prefix the archives don't have, so log a warning instead of the info message that lists the skipped symbols.
…very OS macOS and iOS have the same problem as Windows: `-u` makes the link fail if the symbol isn't defined (ld64: "must be defined for the link to succeed"). On Linux and Android, `-u` of an undefined symbol leaves an undefined dynamic symbol in the library, which fails at load time with `-z now`. readArchiveSymbols now also reads the BSD symbol table of macOS and iOS archives (`__.SYMDEF [SORTED]`, `__.SYMDEF_64 [SORTED]`, with the `#1/<len>` member names of Apple's tools), checked against llvm-ar --format=darwin and llvm-nm --print-armap, and the filter applies to the `-u` flags, the exported symbols list and the version script, matching the leading underscore of Mach-O. The tree-shaking tests ask for an undefined symbol on every OS. Fixes #3737
mosuem
changed the title
Filtering inside the two flag-generating functions made the reader track the requested and the kept symbols through both, and threaded the architecture and logger through every signature. LinkerOptions.withSymbolsDefinedIn now returns restricted options, which CLinker.run computes once from the resolved sources; the flag generation is unchanged from before. The sources are host paths there, so this also works when the linker runs through WSL.
Archives built on macOS for Linux and Android were created with `rcS`, that is without a symbol table, because Apple's ranlib can't index ELF objects (#3437). The NDK's and LLVM's ar can, and the symbol table is what withSymbolsDefinedIn reads, so without it the Android cross-compilation tests on macOS asked the linker for the undefined symbol again. Omit the symbol table only when the archiver is Apple's ar.
This is a variant of OS-specific bindings issue in FFIgen: The current workaround is to generate bindings per host. Which is not great. Now, if some packages have a subset of symbols, it's probably desirable to generate simply the superset. If we had a proper solution for this, the record-use wouldn't report bogus symbols, and we wouldn't have to filter before passing to the native linker. (A proper solution probably requires conditional Dart compilation based on OS. 🤷 ) Until we have that we can land this workaround. => Please add a reference to the github issue of why FFIgen (and similar bindings) could contain symbols not in the native libs. Now one thing that is kind of surprising to me is that TFA does not manage to tree-shake out the symbols that don't exist at runtime. For sure you're not calling them, that would fail => Would it be beneficial to have hard
Magic. rubberstamp llm generated magic! 🙃 |
LinkerOptions.treeshakeasks the linker to keep every symbol insymbolsToKeep. If one of them isn't defined by the inputs, that fails: on Windows the generated module-definition file exports it (LNK2001); on macOS and iOS-urequires it (ld64: "must be defined for the link to succeed"); on Linux and Android-uleaves an undefined dynamic symbol, which fails at load time with-z now. Tree-shaking asks for such symbols easily, for example when bindings cover functions that a particular build or target leaves out, sopackage:boringfilters its symbols itself today.With this PR,
CLinker.runrestricts thesymbolsToKeepto the symbols that the input archives define before generating the linker flags (LinkerOptions.withSymbolsDefinedIn). The flags themselves don't change.readArchiveSymbolsreads only the symbol table of an archive: the first linker member of COFF.libfiles, which has the same format as System V/GNU.afiles, and the BSD__.SYMDEF [SORTED](or__.SYMDEF_64) table of macOS and iOS archives, including the#1/<length>names that Apple's tools use. Checked againstllvm-nm --print-armapfor archives fromllvm-lib(x64, and x86 with_-prefixed and C++ names), GNUar, andllvm-aringnu,bsdanddarwinformat.null(keep all) and an empty list (skip the library) are unchanged too.Behavior change: a symbol in
symbolsToKeepthat only a library inlibrariesdefines, not one of thesources, is no longer kept. Those are usually system libraries, so I don't expect this to matter, but I'm happy to handle it differently.Also bumps the version to 0.19.6, so that this fix and #3718's can be published after merging.
Tests: host-independent unit tests for the archive reader (COFF/GNU and BSD, 32- and 64-bit, truncated and malformed tables), for
withSymbolsDefinedInand the flags it leads to on each OS, and for the warning. The tree-shaking integration tests on every OS, including the Android and iOS cross-compilation ones, and the Windows module-definition test for every Windows architecture now also ask for an undefined symbol.This lets
package:prebuilt_code_assets(#3719) andpackage:boringdrop their own archive parsing.Fixes #3737.
Contribution guidelines:
dart format.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.