-
Notifications
You must be signed in to change notification settings - Fork 13.4k
ld.lld crashing when linking GraphicsMagick #134843
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
Comments
@llvm/issue-subscribers-lld-coff Author: None (lb90)
Here's the crash log:
Output from See https://gitlab.com/inkscape/inkscape/-/merge_requests/7066 |
It looks like this is a duplicate of #131807. When trying to reproduce this issue with a very recent nightly build, I get this error instead (since #134443):
|
Great! :-) Shouldn't the link succeed? That's described in this blog post by Raymond Chen: https://devblogs.microsoft.com/oldnewthing/20060726-00/?p=30363 |
So if we're referencing the symbol But in this case, Now in #109082 we did attempt to fix this; if we don't have I kinda understand how you get there though; you build a library that diligently uses dllexport attributes while building, and diligently uses dllimport attributes in the public headers for users of the library - but they you actually link against a static library. That will get you the error (on 19.x and git main). If the static library doesn't contain dllexport attributes, it should work on the 20.x releases so far, but if it does, then it crashes instead. Now I'm a little curious how people end up hitting this bug so much - is there a build setup where this succeeded with LLD 19.x that now no longer works? |
Yes, the main issue is prebuilt static libraries, e,g as shipped by MSYS2: Suppose MSYS2 provides library A, which in turn uses library B. Now, a developer builds project P1 on MSYS2 and chooses to link A statically, but B dynamically. Another developer builds project P2 on MSYS2 and chooses to link A and B statically. Now, one of the two projects gets dllimport wrong, since MSYS2 has built A for one of the two configurations (it's not clear which one). Would CLang fail to link in that case?
I'll try building GraphicsMagick with CLang 19 Many thanks! |
CC @jeremyd2019 @mati865 @lazka
So, there are a couple of aspects here. The main complication comes from the fact that libraries are provided in two forms, static and dynamic, with only one set of headers, and expecting to be able to pick either of them at link time. Ideally, the static libraries should never contain dllexport attributes in that case. Linking in a static library in your own library, and that suddenly starts exporting APIs from the linked in library, is almost never what you want to happen. The secondly, if you have one set of headers for use with either a static or dynamic library, it can either be made configurable in the headers whether it mark APIs with dllimport or not, depending on which way you want to link it. Or you can just skip the dllimport attributes entirely. Calling a function without a dllimport attribute, when the function is imported from another DLL always works (also on MSVC, see the link you provided earlier). Referencing a data symbol from another DLL without dllimport, doesn't work with MSVC, but it works with all mingw toolchains thanks to a mingw specific (somewhat quirky) feature called autoimports. So for mingw environments, just omitting all dllimports in headers should generally work. There's a tiny bit of extra overhead on calling a dllimported function without the dllimport attribute, but it's quite miniscule. And it makes the same headers work for both static and dynamic libraries. And if the library doesn't directly provide data symbols, the same also works for MSVC style environments.
I would expect it to hit the same error. But the more interesting thing is how this behaves if you'd compile it with GCC/GNU ld. Within mingw environments, we try to align the behaviours between those two, so if you have one setup where you successfully can build it with GCC/GNU ld, but can't do it the same way with Clang/lld, we'd want to fix that. |
I wanted to comment a bit more in detail about that aspect. Besides the crashes, the problem I was having with #109082 is that it makes finding symbols non-deterministic. A given set of inputs will always yield a deterministic outcome; however changing the order of inputs, or slightly changing a single input might pull completly different sections from a different set of objects/archives. This could inadvertently lead to bugs such as described in #82050, even if we'd fixed that particular instance. In contrast, MSVC implicitly mandates a strict order for the parsing of directives, through the second paragraph in https://learn.microsoft.com/en-us/cpp/build/reference/link-input-files?view=msvc-170:
The above essentially says that I have split #85290 into several smaller patches, which I will send PRs for soon, and one of those patches moves parsing of directives a bit earlier in the process, to accomodate for the |
Hi @mstorsjo,
I confirm that the same project builds successfully with GCC /GNU ld Here are the build steps: https://gitlab.com/inkscape/inkscape/-/blob/master/buildtools/msys2installdeps.sh#L149-152 |
Ok, thanks for checking! And thanks for providing repro instructions. I'll see if I have time to try it out for myself in a couple of days hopefully For continuing the diving in here; in the link repro example, you had
And this linked against In the successful build with GCC and GNU ld, does |
Here's the output with binutils nm:
Good question! There are two
Here are their dependencies:
For 2, the only import from
|
Thanks for the investigation! I tried it out myself now, and now I see the issue. Deep down, this is a libtool issue - https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27866. Libtool is not very actively maintained... (That bug report was posted in 2017, but there was no active libtool maintainer for many years. A couple years ago there was some sort of acting maintainer that did respond and try to sort some things out, for a little while, but he also vanished. Now there's another maintainer, who has worked on things again a couple months ago, but this bug hasn't received attention yet.) The bug is worked around in msys2 with patches for libtool, see https://github.com/msys2/MSYS2-packages/blob/master/libtool/0011-Pick-up-clang_rt-static-archives-compiler-internal-l.patch and https://github.com/msys2/MSYS2-packages/blob/master/libtool/0013-Allow-statically-linking-compiler-support-libraries-.patch. Due to how libtool is bundled with the source packages, users need to upgrade the libtool bundled in each source package to work around the bug. So in an msys2 shell (with the right autotools and libtool packages installed) you can do Before fixing this, when attempting to build this package with Clang/LLD, I get these messages while building:
So the build is set up to build a shared library, but when coming to the actual linking phase, libtool decides not to create a shared library after all, and only create a static library. That's why we have the odd inconsistency between dllimport/exports and what we actually link. By running |
Wow, thank you very much! That explain a lot Best Regards! |
Here's the crash log:
Output from
ld.lld --reproduce
: repro.tar.gzSee https://gitlab.com/inkscape/inkscape/-/merge_requests/7066
The text was updated successfully, but these errors were encountered: