Skip to content

build: don't add targets to link_whole_targets if we took their objects #10699

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Aug 25, 2022
Merged

build: don't add targets to link_whole_targets if we took their objects #10699

merged 4 commits into from
Aug 25, 2022

Conversation

dcbaker
Copy link
Member

@dcbaker dcbaker commented Aug 16, 2022

What happens is this:

  • liba is a convenience static library
  • libb is an installed static library
  • libb links in liba with --link-whole
  • libc links to libb
  • we generate a link line with libb and liba, even though libb is a
    strict superset of liba

This is a bug that has existed since the optimization was added to avoid
calling link-whole on convenience libraries, and to instead propagate
their dependencies up. For most linkers this is harmless, if
inefficient. However, for apple's ld64 with the addition calling ranlib -c, this ends up causing multiple copies of symbols to clash (I think
that other linkers recognize that these symbols are the same and combine
them), and linking to fail.

The fix is to stop adding libraries to a target's link_whole_targets
when we take its objects instead. This is an all around win since it
fixes this bug, shortens linker command lines, and avoids opening
archives that no new symbols will be found in anyway.

@dcbaker dcbaker added this to the 0.63.2 milestone Aug 16, 2022
@dcbaker dcbaker requested a review from jpakkane as a code owner August 16, 2022 16:52
@dcbaker
Copy link
Member Author

dcbaker commented Aug 16, 2022

See: lovell/sharp-libvips#154 for an example of this

@dcbaker dcbaker mentioned this pull request Aug 16, 2022
Merged
@codecov
Copy link

codecov bot commented Aug 16, 2022 •
edited
Loading

Codecov Report

Merging #10699 (5b3031e) into master (f8ebfdf) will decrease coverage by 9.91%.
The diff coverage is n/a.

❗ Current head 5b3031e differs from pull request most recent head 5de0d16. Consider uploading reports for the commit 5de0d16 to get more accurate results

@@            Coverage Diff             @@
##           master   #10699      +/-   ##
==========================================
- Coverage   68.95%   59.03%   -9.92%     
==========================================
  Files         406      203     -203     
  Lines       88306    44167   -44139     
  Branches    19591     9791    -9800     
==========================================
- Hits        60888    26076   -34812     
+ Misses      22836    15910    -6926     
+ Partials     4582     2181    -2401
Impacted Files Coverage Δ
scripts/run_tool.py 0.00% <0.00%> (-100.00%) ⬇️
scripts/clangtidy.py 0.00% <0.00%> (-93.34%) ⬇️
modules/cuda.py 0.00% <0.00%> (-72.64%) ⬇️
scripts/uninstall.py 0.00% <0.00%> (-71.88%) ⬇️
mconf.py 14.51% <0.00%> (-70.97%) ⬇️
templates/cstemplates.py 35.48% <0.00%> (-64.52%) ⬇️
scripts/coverage.py 0.00% <0.00%> (-64.36%) ⬇️
ast/introspection.py 15.25% <0.00%> (-63.56%) ⬇️
templates/javatemplates.py 36.66% <0.00%> (-63.34%) ⬇️
rewriter.py 17.20% <0.00%> (-63.27%) ⬇️
... and 296 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@dcbaker dcbaker force-pushed the submit/apple-ar-dont-ranlib branch from e433ca0 to 17fc6ce Compare August 16, 2022 17:44
eli-schwartz
eli-schwartz requested changes Aug 16, 2022
View reviewed changes
jpakkane
jpakkane reviewed Aug 19, 2022
View reviewed changes
@dcbaker dcbaker force-pushed the submit/apple-ar-dont-ranlib branch 2 times, most recently from 46ef337 to 9500aad Compare August 22, 2022 23:45
@dcbaker
Copy link
Member Author

dcbaker commented Aug 22, 2022

I think I have it all sorted out now, we'll see. A lot changed between this version and the last one.

@eli-schwartz
Copy link
Member 

[1/9] /usr/sbin/python3 /__w/meson/meson/meson.py --internal depscan libmainstatic.a.p/mainstatic.dat libmainstatic.a.p/depscan.dd '/__w/meson/meson/b 0c301b5fbf/libmainstatic.a.p/mainstatic-deps.json'
[2/9] gfortran -Isubprojects/static_hello/libstatic_hello.a.p -Ilibmainstatic.a.p -I. '-I../test cases/fortran/21 install static' -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -O0 -g -fPIC -Jlibmainstatic.a.p -o libmainstatic.a.p/meson-generated_.._main_lib_output.F90.o -c main_lib_output.F90
FAILED: libmainstatic.a.p/meson-generated_.._main_lib_output.F90.o libmainstatic.a.p/main_lib.mod 
gfortran -Isubprojects/static_hello/libstatic_hello.a.p -Ilibmainstatic.a.p -I. '-I../test cases/fortran/21 install static' -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -O0 -g -fPIC -Jlibmainstatic.a.p -o libmainstatic.a.p/meson-generated_.._main_lib_output.F90.o -c main_lib_output.F90
main_lib_output.F90:3:7:

    3 |   use static_hello
      |       1
Fatal Error: Cannot open module file ‘static_hello.mod’ for reading at (1): No such file or directory
compilation terminated.
[3/9] /usr/sbin/python3 /__w/meson/meson/meson.py --internal depscan main_exe.p/main_exe.dat main_exe.p/depscan.dd '/__w/meson/meson/b 0c301b5fbf/main_exe.p/main_exe-deps.json'
[4/9] /usr/sbin/python3 /__w/meson/meson/meson.py --internal depscan subprojects/static_hello/libstatic_hello.a.p/static_hello.dat subprojects/static_hello/libstatic_hello.a.p/depscan.dd '/__w/meson/meson/b 0c301b5fbf/subprojects/static_hello/libstatic_hello.a.p/static_hello-deps.json'
ninja: build stopped: subcommand failed.

This actually works with ninja -k0 || ninja. The problem is that depscan for libmainstatic.a is NOT detecting subprojects/static_hello/libstatic_hello.a.p/static_hello.mod as a dyndep, although it is added as an -I flag.

This magically worked before this PR only because there was a direct dependency on subprojects/static_hello/libstatic_hello.a

dcbaker added 2 commits August 24, 2022 14:24
@dcbaker
build: Don't attempt to link-whole with rust convenience libraries
4d92e5e 
There are two distinct cases here that need to be considered. The first
issue is mesonbuild#10723 and
mesonbuild#10724, which means that Meson
can't actually generate link-whole arguments with rust targets. The
second is that rlibs are never valid candidates for link-whole anyway.
The promotion happens to work because of another bug in the promotion
path (which is fixed in the next commit).
@dcbaker
build: add a uses_fortran() convenience method
efd255a
@dcbaker dcbaker force-pushed the submit/apple-ar-dont-ranlib branch from 9500aad to 448387a Compare August 24, 2022 22:24
dcbaker added 2 commits August 24, 2022 15:52
@dcbaker
backend/ninja: properly track objects extracted from fortran sources
25c6a2b 
We need this to ensure that .mod files are created before we start
compiling, and to ensure that the proper include directory arguments are
generated.
@dcbaker
build: don't add targets to link_whole_targets if we took their objects
5de0d16 
What happens is this:
 - liba is a convenience static library
 - libb is an installed static library
 - libb links in liba with --link-whole
 - libc links to libb
 - we generate a link line with libb *and* liba, even though libb is a
   strict superset of liba

This is a bug that has existed since the we stopped using link-whole to
combine convenience libraries, and to instead propagate their
dependencies up. For most linkers this is harmless, if inefficient.
However, for apple's ld64 with the addition calling `ranlib -c`, this
ends up causing multiple copies of symbols to clash (I think that other
linkers recognize that these symbols are the same and combine them), and
linking to fail.

The fix is to stop adding libraries to a target's `link_whole_targets`
when we take its objects instead. This is an all around win since it
fixes this bug, shortens linker command lines, and avoids opening
archives that no new symbols will be found in anyway.
@dcbaker dcbaker force-pushed the submit/apple-ar-dont-ranlib branch from 448387a to 5de0d16 Compare August 24, 2022 22:52
@eli-schwartz eli-schwartz merged commit c94c492 into mesonbuild:master Aug 25, 2022
@dcbaker dcbaker deleted the submit/apple-ar-dont-ranlib branch August 25, 2022 03:01
@eli-schwartz eli-schwartz mentioned this pull request Aug 29, 2022
Merged
@nirbheek nirbheek modified the milestones: 0.63.2, 0.63.3 Sep 2, 2022
@eli-schwartz
Copy link
Member

This is getting delayed until either the point release after next, or until 0.64.0, because it causes this regression: #10745

@anarazel anarazel mentioned this pull request Sep 19, 2022
Closed
@nirbheek nirbheek modified the milestones: 0.63.3, 0.63.4 Oct 1, 2022
@nirbheek
Copy link
Member

nirbheek commented Oct 1, 2022

Punted to 0.63.4 because #10745 is still not fixed.

@eli-schwartz eli-schwartz mentioned this pull request Nov 1, 2022
Closed
@xclaesse xclaesse mentioned this pull request Apr 30, 2023
Merged
xclaesse pushed a commit to xclaesse/meson that referenced this pull request Jul 5, 2023
@dcbaker @xclaesse
tests: Test extern'd globals on MacOS with the Apple Archiver
00552cf 
This is a regression test for:
mesonbuild#11165.

The test was initially merged as part of:
mesonbuild#10628, but had to be reverted.

A second attempt at fixing the root cause also had to be reverted:
mesonbuild#10699.

A 3rd attempt got merged but missed this unit test:
mesonbuild#11742.

Co-authored-by: Dylan Baker <dylan@pnwbakers.com>
@xclaesse xclaesse mentioned this pull request Jul 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpakkane jpakkane jpakkane left review comments

@eli-schwartz eli-schwartz eli-schwartz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.63.4
Development

Successfully merging this pull request may close these issues.
4 participants
@dcbaker @eli-schwartz @nirbheek @jpakkane