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

Restore the possibility of forcing "fat" static library links on a per-target basis #9453

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

obilaniu
Copy link
Contributor

With 0.60.0, non-installed static_library() targets are built by default as thin archives where supported, such as with ar csrT. This broke a flow where I was statically linking code into an archive, then passing the archive to a custom_target() that was expecting a "fat" archive and wasn't handling "thin" archives (specifically, it's nvcc being driven with custom flags).

Thin archives are handy, but "fattening" an archive where needed is quite involved and annoying.

Add a thin: kwarg to static_library() and default it to true, continuing the new behaviour of using thin archives in 0.60.0+. Passing the non-default thin: false restores the known-good behaviour of Meson prior to 0.60.0.

@codecov
Copy link

codecov bot commented Oct 26, 2021

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 67.30%. Comparing base (18e4b3f) to head (81d630d).
Report is 3091 commits behind head on master.

Files Patch % Lines
mesonbuild/build.py 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9453      +/-   ##
==========================================
- Coverage   67.31%   67.30%   -0.01%     
==========================================
  Files         396      396              
  Lines       85468    85474       +6     
  Branches    17661    17663       +2     
==========================================
+ Hits        57530    57532       +2     
- Misses      23249    23251       +2     
- Partials     4689     4691       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xclaesse
Copy link
Member

This PR looks good to me, just missing a unit test ideally. However, we are not supposed to add new API in a point release. Since thin archives is only a performance optimisation and I don't think it has any functional difference, I think we should for 0.60.1 revert to not use thin archives by default and add this API for 0.61.0. What do you think?

@obilaniu
Copy link
Contributor Author

@xclaesse Favourably disposed.

@xclaesse
Copy link
Member

@jpakkane @dcbaker @eli-schwartz what do you think? Should we revert the thin archive stuff and postpone it to 0.61.0?

@eli-schwartz
Copy link
Member

Well, git master is already for 0.61, the question is whether to revert it in 0.60.1 :p

In #9294 I wasn't sure whether this might sometimes be unwanted, but I wasn't expecting cases where it's unwanted because of what the meson.build does with the output lol. I wish we'd discovered this earlier... no one seemed to be quite sure what the potential pitfalls of thin archives might be.

Is this a problem for meson's own cuda compiler too?

@obilaniu
Copy link
Contributor Author

@eli-schwartz By pure luck, the Meson testcases pass for CUDA because they are shielded with an -Xlinker flag: -Xlinker=libblabla.a. This will break, however, if the testcases would have attempted to link only static_library()s together in one target without sources, because then NVCC, not seeing anything but opaque -Xlinker=... flags, thinks it's not been given any compilation/linking work to do. But if I fully manually invoke NVCC through custom_target('name', [nvcc, ...]) on a (obviously not-installed) static_library(), it vomits on me nvcc: elfLinker: something something format not recognized or not an ELF file.

@xclaesse
Copy link
Member

@obilaniu I'm not sure your test case actually test much, we have no way to verify the generated archive actually is thin or not, do we? Are there tools that can tell?

@xclaesse
Copy link
Member

@obilaniu
Copy link
Contributor Author

@xclaesse If the Filesystem's module's fs.size(filename) accepted build targets, perhaps... but even if it did, there is no guarantee that the toolchain will support thinning, so I can't assert anything more strict than <=, but that would test almost nothing.

Is !<thin> actually reliable on all platforms that support thinning?

@xclaesse
Copy link
Member

There are not that many platforms that supports thin archives in the first place... Tbh as long as it pass on CI platforms it's fine.

@jpakkane
Copy link
Member

specifically, it's nvcc being driven with custom flags

Has this issue been reported to NVidia? This seems like a thing that they should handle.

@eli-schwartz
Copy link
Member

eli-schwartz commented Oct 28, 2021

Continuing with my shower thoughts... there wouldn't happen to be some way we could detect that the archive is going to be used by a custom_target and disable generating thin archives in that case, would there be? In order to have a slightly safer/more conservative default...

obilaniu added a commit to obilaniu/meson that referenced this pull request Oct 28, 2021
Some custom_target()s may want to receive "fat" archives, so making use
of thin static_library() archives conditional only on it not being
installed, and without the possibility of an override by the user, is
undesirable. In mesonbuild#9453 I propose adding a thin: kwarg defaulting to true,
but that's a new feature that can only land in 0.61.
xclaesse pushed a commit that referenced this pull request Oct 28, 2021
Some custom_target()s may want to receive "fat" archives, so making use
of thin static_library() archives conditional only on it not being
installed, and without the possibility of an override by the user, is
undesirable. In #9453 I propose adding a thin: kwarg defaulting to true,
but that's a new feature that can only land in 0.61.
@obilaniu
Copy link
Contributor Author

@jpakkane @eli-schwartz Here is a scenario where you can quickly observe all kinds of dysfunctions of NVCC and (in some places) our CUDA support.

In this file, I compile four static_library()'s (with different flags, but this is immaterial here), and fuse them into one shared library. Obviously, the component static libraries shouldn't be installed, but the shared library perhaps.

At the top of meson.build there are two knobs: variant = 'a' or 'b' flag and a fat = true or false flag.

  • variant='a': Uses shared_library() for fusion. Fails with nvcc fatal : No input files specified; use option --help for more information because all input libraries are made opaque by their wrapping -Xlinker= flag. I don't know why we're injecting those. Whether thin-link or fat-link doesn't matter here.
  • variant='b', fat=false: Uses custom_target() for fusion, with a quasi-identical line (but see footnote) except for stripping the -Xlinker=flags. Unfortunately, that fails with both nvlink fatal : Memory allocation failure and nvlink fatal : elfLink internal error.
  • variant='b', fat=true: Idem, but sets install: true on the static libraries, thereby forcing them to be built "fat". The build works.

I have tested this with CUDA Toolkit 10.2 (old) and 11.4 (newest aside from before-before-yesterday's 11.5 release), both with similar results. The odds are 11.5 will be affected as well. Even if NVIDIA fixed this tomorrow, there's a huge legacy of slightly-broken compilers to support.

I therefore use a custom_target() to do that step of the build, and this broke with Meson 0.60 due to the switchover to thin libraries internally. And there is nothing that I can do to make it build "fat" archives currently, short of installing the temporary libraries. Fixing Meson's strange -Xlinker= prefixing habit (going from variant 'a' -> 'b') is will not be a complete solution because it would simply expose the thin-archive problem more broadly.

For this reason, I made this PR, which proposes returning control to the user through the thin: [true|false] flag if they're dealing with broken toolchains like NVCC, or other post-processing tools. Default would be thin: true in the spirit of speeding up builds.


Footnote: When using custom_target(), I normally require the use of -Xcompiler=-Wl\,foo\,bar and because the , is special to both NVCC's -Xcompiler and the linker's -Wl itself, I need to escape the backslashes, thus: -Xcompiler=-Wl\\,foo\\,bar. Unfortunately, custom_target() somehow turns \ into / (???), and this breaks the build. I didn't absolutely require an -Xcompiler= flag to demonstrate the thin-archive problem, but it's a separate issue in Meson itself.

nirbheek pushed a commit that referenced this pull request Oct 29, 2021
Some custom_target()s may want to receive "fat" archives, so making use
of thin static_library() archives conditional only on it not being
installed, and without the possibility of an override by the user, is
undesirable. In #9453 I propose adding a thin: kwarg defaulting to true,
but that's a new feature that can only land in 0.61.
@bonzini
Copy link
Contributor

bonzini commented Dec 16, 2021

This seems more like a task for a built-in option, than a keyword argument.

pbruenn added a commit to Beckhoff/ADS that referenced this pull request Nov 8, 2023
It seems newer versions of meson[1] started to generate "thin" static
libraries, which cannot be stripped by Debians package helper script
(called by bdpg).

The common workaround to avoid thin archives in meson seems to mark the
static library with "install: true", until an explicit option might
become available[2].

[1] mesonbuild/meson#9479
[2] mesonbuild/meson#9453

Signed-off-by: Patrick Bruenn <p.bruenn@beckhoff.com>
SoapGentoo added a commit to SoapGentoo/meson that referenced this pull request May 7, 2024
SoapGentoo added a commit to SoapGentoo/meson that referenced this pull request May 7, 2024
SoapGentoo added a commit to SoapGentoo/meson that referenced this pull request May 7, 2024
dcbaker pushed a commit that referenced this pull request May 8, 2024
eli-schwartz pushed a commit that referenced this pull request May 29, 2024
Bug: /pull/9453
Bug: /issues/9479#issuecomment-953485040
(cherry picked from commit b6e5683)
soumyaDghosh pushed a commit to soumyaDghosh/meson that referenced this pull request Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants