Dependent libraries should not be processed in the "whole archive" way because that leads to pulling unused archive members and may result in triggering "duplicate symbol" errors.
Details
Details
Diff Detail
Diff Detail
Unit Tests
Unit Tests
Event Timeline
| lld/ELF/Driver.cpp | ||
|---|---|---|
| 1432 | How about moving it to addDependentLibrary? Then you can have much shorter comment. | |
| lld/ELF/Driver.cpp | ||
|---|---|---|
| 1432 | inWholeArchive is a private member of LinkerDriver, it cannot be accessed from addDependentLibrary(). | |
Comment Actions
In the thread "[llvm-dev] RFC: ELF Autolinking", @psmith said:
I'm less convinced about --whole-archive as I think this tends to be a way of structuring the build and would be best made explicit in the build system. Moreover, what if someone wants to not use --whole-archive, for their autolink, but one already exists. This could be quite difficult to check with a large project. Personally I'd have the user be explicit in the .autolink whether they were intending it to be whole-archive or not.
@bd1976llvm replied:
Right.. definitely possible to implement. So the trade offs are that it is possibly confusing if options like --whole-archive start applying to the "invisible" autolinked inputs. OTOH why not allow command line options to affect the autolinked inputs? It gives developers some more control at no cost (apart form the possible confusion).
From the thread it seems that no conclusion on this point was made.
| lld/ELF/Driver.cpp | ||
|---|---|---|
| 1423 | If we are going to do this This comment can be greatly simplified. The first sentence can be removed. You can just say: "dependent libraries are linked in --no-whole-archive mode". (Prefer the double dash form, which appears to be more common than the single dash form) | |
Comment Actions
- Simplified the comment.
Even if that was not decided in the discussion, I am pretty sure that --whole-archive cannot be used with dependent libraries, at least as the feature is implemented currently. The whole idea of the feature is that you can place #pragma comment(lib,...) in each and every source file which depends on the library. This results in multiple references to the library from many input object files, and if --whole-archive is active, this leads the linker to fail with the "duplicate symbol" error.
Comment Actions
I don't have any objections. My comment in the thread was more along the lines that dependent libraries could interact with whole-archive in hard to predict ways. I'm happy to have dependent libraries ignore whole-archive. I'm fine with MaskRay accepting when he is happy.
Comment Actions
I'm fine with bd1976llvm accepting when he is happy.
| lld/test/ELF/deplibs-ignore-whole-archive.s | ||
|---|---|---|
| 18 | --whole-archive (which is more common) | |
Comment Actions
Sorry for the late reply.
Firstly, is this a real problem? In the RFC thread http://lists.llvm.org/pipermail/llvm-dev/2019-March/131105.html Rui said:
I'm less convinced about --whole-archive as
I think this tends to be a way of structuring the build and would be
best made explicit in the build system. Moreover, what if someone
wants to not use --whole-archive, for their autolink, but one already
exists.Then they can specify --no-whole-archive on the end of the command
line, no?
I think we want to be sure that users can't simply adjust their linker command line before we complicate autolinking. Given that autolinking is a new feature on ELF it doesn't seem unfair that users should understand the feature and consider adjusting their linker command lines. Maybe we could add some diagnostics to help them.. e.g. lld could advise users that "--whole-archive was active" if a duplicate symbol error arises during auto-linking?
I also want to be certain that the GNU linkers can implement whatever we decide as that was as important design consideration.
As to --whole-archive not being compatible with autolinking.. maybe the set of autolinked inputs should be deduplicated. This was discussed in the original RFC thread and would also solve https://bugs.llvm.org/show_bug.cgi?id=42460.
Comment Actions
The problem is real. We have a report from our customer, so the issue is already observed in the wild.
In the RFC thread http://lists.llvm.org/pipermail/llvm-dev/2019-March/131105.html Rui said:
Then they can specify --no-whole-archive on the end of the command line, no?
Unfortunately, sometimes that is not that simple as it sounds. In our case, the users who need autolinking are different from a unit that provides tools to adjust command lines. There is no option to add anything at the very end.
I think we want to be sure that users can't simply adjust their linker command line before we complicate autolinking. Given that autolinking is a new feature on ELF it doesn't seem unfair that users should understand the feature and consider adjusting their linker command lines. Maybe we could add some diagnostics to help them.. e.g. lld could advise users that "--whole-archive was active" if a duplicate symbol error arises during auto-linking?
As to --whole-archive not being compatible with autolinking.. maybe the set of autolinked inputs should be deduplicated. This was discussed in the original RFC thread and would also solve https://bugs.llvm.org/show_bug.cgi?id=42460.
Duplicate symbols are one part of the issue, the most noticeable because it prevents linking. Another, hidden part is that if the library is referenced only once, the linker fetches unneeded unreferenced members. It would be fair if the linker works in an expected way.
As I understand the idea of the --whole-archive switch, it meant to instruct a linker which libraries should be included as a whole, not how you prefer the output to be linked. In the latter case that would be simple a global switch that would affects all input libraries.
You know how they say, explicit is better than implicit. What can be less explicit than a switch in the command line which affects linking a library referenced far away from it, in a source file header? If there will ever be a use case for autolinking in a --whole-archive way, it will be much better to add an explicit option in the pragma itself or provide a distinct linker switch, something like --whole-autolink-library.
| lld/test/ELF/deplibs-ignore-whole-archive.s | ||
|---|---|---|
| 18 | Thanks! | |
Comment Actions
Thanks for supplying this detail. To me this sounds like you need to file a bug against the tools unit? I want to evaluate your proposal on it's own merits rather accepting it to work around problems caused by other tools.
I think we want to be sure that users can't simply adjust their linker command line before we complicate autolinking. Given that autolinking is a new feature on ELF it doesn't seem unfair that users should understand the feature and consider adjusting their linker command lines. Maybe we could add some diagnostics to help them.. e.g. lld could advise users that "--whole-archive was active" if a duplicate symbol error arises during auto-linking?
As to --whole-archive not being compatible with autolinking.. maybe the set of autolinked inputs should be deduplicated. This was discussed in the original RFC thread and would also solve https://bugs.llvm.org/show_bug.cgi?id=42460.
Duplicate symbols are one part of the issue, the most noticeable because it prevents linking. Another, hidden part is that if the library is referenced only once, the linker fetches unneeded unreferenced members. It would be fair if the linker works in an expected way.
As I understand the idea of the --whole-archive switch, it meant to instruct a linker which libraries should be included as a whole, not how you prefer the output to be linked. In the latter case that would be simple a global switch that would affects all input libraries.
You know how they say, explicit is better than implicit. What can be less explicit than a switch in the command line which affects linking a library referenced far away from it, in a source file header? If there will ever be a use case for autolinking in a --whole-archive way, it will be much better to add an explicit option in the pragma itself or provide a distinct linker switch, something like --whole-autolink-library.
Currently, the mental model for auto-linking is very simple to reason about. LLD acts as if it didn't do auto-linking and its command line was "lld <options> <auto-linked inputs>". You want to change this to: "lld <options> --no-whole-archive <auto-linked inputs>". Whilst I agree that --whole-archive applying can be confusing I dislike having special cases. Looking at my notes from when I was thinking about auto-linking I identified these options...
L library path command line options
whole-archive
start-group, end-group
start-lib, end-lib
as-needed, https://gcc.gnu.org/ml/gcc-help/2010-12/msg00338.html and other shared object options.
...as needing special consideration w.r.t auto-linking (maybe there are others?). I wonder if it would be more consistent to reset all of these to their defaults before the auto-linked inputs are processed. Rather than adding special auto-link versions of command line options we could have one option --no-autolink-reset-cmdline (there must be a better name!) which reverts to the current behaviour (for powerusers that want to apply these options to auto-linking). OTOH perhaps order independent options e.g. your --whole-autolink-library are better if there are build systems that are unable to ensure that options are appended to the "end" of the command line, are such build systems common?
Sorry, if this all sounds pedantic. A lot of effort went into making the current auto-linking feature ("dependent libraries") useful, minimal and simple to understand. I just want to make sure we have a good design before making changes.
Comment Actions
Hmm, that starts looking like a discussion for the mailing list, but I guess that all interested people are already here and the conversation will be better preserved for further references if being linked to the review.
And for the tool unit folks that looks like an issue in the linker. Why should they work around problems caused by the linker? Why not fix them in the source? After all, build systems are many and reasoning for fixing them is vague. Closing "--whole-archive" range is more like tidiness, which was not that strictly required before (or without) the autolinking feature.
Currently, the mental model for auto-linking is very simple to reason about. LLD acts as if it didn't do auto-linking and its command line was "lld <options> <auto-linked inputs>".
The problem with the model is that it looks natural for very few people in the entire world because it just describes how the feature is implemented in LLD. I guess a developer that just uses the pragma would usually expect a library to be included in the link at the place where it is mentioned, i.e. when the object file that references it is processed.
You want to change this to: "lld <options> --no-whole-archive <auto-linked inputs>".
Not exactly. I want auto-linked inputs to be processed independently of any temporal options of the command line.
Whilst I agree that --whole-archive applying can be confusing I dislike having special cases. Looking at my notes from when I was thinking about auto-linking I identified these options...
L library path command line options
whole-archive
start-group, end-group
start-lib, end-lib
as-needed, https://gcc.gnu.org/ml/gcc-help/2010-12/msg00338.html and other shared object options....as needing special consideration w.r.t auto-linking (maybe there are others?). I wonder if it would be more consistent to reset all of these to their defaults before the auto-linked inputs are processed.
My point exactly. I prefer LinkerDriver::createFiles() to restore the temporal options to their default state.
Rather than adding special auto-link versions of command line options we could have one option --no-autolink-reset-cmdline (there must be a better name!) which reverts to the current behaviour (for powerusers that want to apply these options to auto-linking). OTOH perhaps order independent options e.g. your --whole-autolink-library are better if there are build systems that are unable to ensure that options are appended to the "end" of the command line, are such build systems common?
I would prefer not to add any special options for that. If some libraries needed to be treated in a special way, that should be mentioned in the #pragma and stored along with the name of the library.
Sorry, if this all sounds pedantic. A lot of effort went into making the current auto-linking feature ("dependent libraries") useful, minimal and simple to understand. I just want to make sure we have a good design before making changes.
BTW, I have one more thing to consider...
As far as I can remember, the GNU linkers process input files right when they encounter them in the linking process. They are unable to postpone processing the dependent libraries until after all the command line is processed, at least because they cannot resolve back references. Thus, they will not be able to mimic the approach with the state of temporal options as-it-would-be-at-the-end-of-the-command-line. Rather than, they most probably would use the state of the options at the moment a particular object file is processed, which would diverge from LLD and, most importantly, would be very hard to reproduce at LLD's side. Thus, the simplest common way to process dependent libraries would be to ignore the temporal state of temporal options.
Comment Actions
I think I agree that build systems might not easily be able to arrange for appending options to the end of the linker command line. Therefore, for the "dependent libraries" feature (I changed the name in https://reviews.llvm.org/D60274 because autolinking was too general) it would be reasonable to reset any temporal options that the build system would otherwise have to handle by appending an argument to the very end of the command line (e.g. --whole-archives). Where appropriate we can create new order-independent options to apply the options that are reset to the dependent libraries. This seems like an improvement over what we have now.
Currently, the mental model for auto-linking is very simple to reason about. LLD acts as if it didn't do auto-linking and its command line was "lld <options> <auto-linked inputs>".
The problem with the model is that it looks natural for very few people in the entire world because it just describes how the feature is implemented in LLD. I guess a developer that just uses the pragma would usually expect a library to be included in the link at the place where it is mentioned, i.e. when the object file that references it is processed.
You want to change this to: "lld <options> --no-whole-archive <auto-linked inputs>".
Not exactly. I want auto-linked inputs to be processed independently of any temporal options of the command line.
Whilst I agree that --whole-archive applying can be confusing I dislike having special cases. Looking at my notes from when I was thinking about auto-linking I identified these options...
L library path command line options
whole-archive
start-group, end-group
start-lib, end-lib
as-needed, https://gcc.gnu.org/ml/gcc-help/2010-12/msg00338.html and other shared object options....as needing special consideration w.r.t auto-linking (maybe there are others?). I wonder if it would be more consistent to reset all of these to their defaults before the auto-linked inputs are processed.
My point exactly. I prefer LinkerDriver::createFiles() to restore the temporal options to their default state.
Rather than adding special auto-link versions of command line options we could have one option --no-autolink-reset-cmdline (there must be a better name!) which reverts to the current behaviour (for powerusers that want to apply these options to auto-linking). OTOH perhaps order independent options e.g. your --whole-autolink-library are better if there are build systems that are unable to ensure that options are appended to the "end" of the command line, are such build systems common?
I would prefer not to add any special options for that. If some libraries needed to be treated in a special way, that should be mentioned in the #pragma and stored along with the name of the library.
I understand why you would want those features; however, I don't think they fit with dependent libraries. If you read the RFC you can see that we were deliberate in avoiding these complications. I think that it might be worth looking into completing the linker options scheme discussed here: http://lists.llvm.org/pipermail/llvm-dev/2019-March/131360.html.
Sorry, if this all sounds pedantic. A lot of effort went into making the current auto-linking feature ("dependent libraries") useful, minimal and simple to understand. I just want to make sure we have a good design before making changes.
BTW, I have one more thing to consider...
As far as I can remember, the GNU linkers process input files right when they encounter them in the linking process. They are unable to postpone processing the dependent libraries until after all the command line is processed, at least because they cannot resolve back references. Thus, they will not be able to mimic the approach with the state of temporal options as-it-would-be-at-the-end-of-the-command-line. Rather than, they most probably would use the state of the options at the moment a particular object file is processed, which would diverge from LLD and, most importantly, would be very hard to reproduce at LLD's side. Thus, the simplest common way to process dependent libraries would be to ignore the temporal state of temporal options.
Rui detailed how GNU linkers could support dependent libraries here: http://lists.llvm.org/pipermail/llvm-dev/2019-March/131186.html.
LGTM to this change. I'm happy to tackle the other range options as problems are encountered, rather than doing them all at once now. I'm also happy to add position independent options to apply range options to the dependent libraries later (as they are requested, via bug reports etc).
| lld/test/ELF/deplibs-ignore-whole-archive.s | ||
|---|---|---|
| 8 | If we implement deduplication for dependent libraries then this test may spuriously pass. I would rewrite the test so that it uses --trace to check that unreferenced archive members are not added to the link. I would also add a comment to say that --whole-archive applying to dependent libraries is confusing and build-systems are not always able to add an option (--no-whole-archive) to the very end of the command line. | |
Comment Actions
Regarding resetting any temporal options, I think users may not want to reset --as-needed to --no-as-needed. Unclosed --as-needed may be used: --as-needed generally works with system libraries added by GCC/clang drivers.
Regarding the build system, --whole-archive --no-whole-archive pairs are usually used together and localized. I'd still want to understand why there may be an unclosed --whole-archive.
If there is an unclosed --whole-archive, with -static-libgcc, libgcc.a and libgcc_eh.a will be linked in --whole-archive mode. Usually there will be multiple definition issues in libgcc.a.
Similarly, with -static-libstdc++, libstdc++/libc++ will be linked in --whole-archive mode.
Comment Actions
Fully agreed. Thanks.
I would prefer not to add any special options for that. If some libraries needed to be treated in a special way, that should be mentioned in the #pragma and stored along with the name of the library.
I understand why you would want those features; however, I don't think they fit with dependent libraries. If you read the RFC you can see that we were deliberate in avoiding these complications. I think that it might be worth looking into completing the linker options scheme discussed here: http://lists.llvm.org/pipermail/llvm-dev/2019-March/131360.html.
I guess that that is better to be discussed when there is a request for that.
As far as I can remember, the GNU linkers process input files right when they encounter them in the linking process. They are unable to postpone processing the dependent libraries until after all the command line is processed, at least because they cannot resolve back references. Thus, they will not be able to mimic the approach with the state of temporal options as-it-would-be-at-the-end-of-the-command-line. Rather than, they most probably would use the state of the options at the moment a particular object file is processed, which would diverge from LLD and, most importantly, would be very hard to reproduce at LLD's side. Thus, the simplest common way to process dependent libraries would be to ignore the temporal state of temporal options.
Rui detailed how GNU linkers could support dependent libraries here: http://lists.llvm.org/pipermail/llvm-dev/2019-March/131186.html.
Well, Rui's proposal can be seemed like adding the ability to resolve back references into GNU linkers. That would change how the linkers resolve references in the general case, not only for dependent libraries. That might seem a bit revolutionary.
LGTM to this change. I'm happy to tackle the other range options as problems are encountered, rather than doing them all at once now. I'm also happy to add position independent options to apply range options to the dependent libraries later (as they are requested, via bug reports etc).
Thanks! I have no plans for the other changes without requests.
I would discuss that when we have a good practical example.
Regarding the build system, --whole-archive --no-whole-archive pairs are usually used together and localized. I'd still want to understand why there may be an unclosed --whole-archive.
If there is an unclosed --whole-archive, with -static-libgcc, libgcc.a and libgcc_eh.a will be linked in --whole-archive mode. Usually there will be multiple definition issues in libgcc.a.
Similarly, with -static-libstdc++, libstdc++/libc++ will be linked in --whole-archive mode.
We are targeting a platform, which is not public. It has lots of its own specifics, and it does not use libgcc and friends.
| lld/test/ELF/deplibs-ignore-whole-archive.s | ||
|---|---|---|
| 8 | I have extended the comment. As for a different implementation which might also make the test pass, I do not think the way the issue is fixed really matters. Note also that the test already checks that bar, the symbol which is not referenced, is not included in the output. What advantages does checking for absence with --trace have? | |
| lld/test/ELF/deplibs-ignore-whole-archive.s | ||
|---|---|---|
| 8 | Sorry, I misread the test. You can ignore my --trace comment. Checking for the presence of symbol "bar" is sufficient. However, I would remove the parts of the test that check the behaviour for duplicate libraries on the command line as checking for "bar" is sufficient to show that --whole-archive is not active. Also, there is no need to describe the effect of --whole-archive. You can rewrite the comment like this...
...and remove one of the ".asciz "lib.a" lines." | |
| 18 | NIT : You could create %t.dir at the beginning of the test and cd into it. That would simplify some of the test lines slightly e.g. "llvm-mc -filetype=obj -triple=x86_64 - -o %tfoo.o" becomes "llvm-mc -filetype=obj -triple=x86_64 - -o foo.o" | |
| 26–28 | NIT: You could drop these three lines and add "-u foo" to the link line instead, | |
| lld/test/ELF/deplibs-ignore-whole-archive.s | ||
|---|---|---|
| 8 | My "##"s became 1., 2. 3. :( | |
Comment Actions
I'm fine with bd1976llvm accepting when he is happy.
Sorry but I'll withdraw my previous comment after more thinking.
I am still concerned that we go down this route (workaround in the linker rather than teach the user to add --no-whole-archive just because of convenience adding logic in the linker). Among --push-state pushed states, --as-needed and -Bstatic may be states users want to keep for the dependent libraries.
Regarding the build system, --whole-archive --no-whole-archive pairs are usually used together and localized. I'd still want to understand why there may be an unclosed --whole-archive. If there is an unclosed --whole-archive, with -static-libgcc, libgcc.a and libgcc_eh.a will be linked in --whole-archive mode. Usually there will be multiple definition issues in libgcc.a. Similarly, with -static-libstdc++, libstdc++/libc++ will be linked in --whole-archive mode.
We are targeting a platform, which is not public. It has lots of its own specifics, and it does not use libgcc and friends.
I am on the fence whether this is a good argument for adding the additional logic. If you do have libraries with duplicate definitions, this will become very clear hard errors in --whole-archive mode. I am not sure "resetting to --no-whole-archive" is still needed. The argument is that this added inconsistency to the saved --push-state states.
Comment Actions
This is not a workaround in the linker for something that is broken elsewhere. It is all about making the linker behavior more stable and predictable. Our case just highlighted the issue. Using --whole-archive with dependent libraries is definitely broken and has to be fixed. Fixing it in the proposed way will add more convenience for the user, for sure, but that is not the main concern.
Regarding the build system, --whole-archive --no-whole-archive pairs are usually used together and localized. I'd still want to understand why there may be an unclosed --whole-archive. If there is an unclosed --whole-archive, with -static-libgcc, libgcc.a and libgcc_eh.a will be linked in --whole-archive mode. Usually there will be multiple definition issues in libgcc.a. Similarly, with -static-libstdc++, libstdc++/libc++ will be linked in --whole-archive mode.
We are targeting a platform, which is not public. It has lots of its own specifics, and it does not use libgcc and friends.
I am on the fence whether this is a good argument for adding the additional logic. If you do have libraries with duplicate definitions, this will become very clear hard errors in --whole-archive mode. I am not sure "resetting to --no-whole-archive" is still needed. The argument is that this added inconsistency to the saved --push-state states.
One of the things I have learned from our case is that it can be difficult to determine the source of the problem. A developer just adds "#pragma comment(lib, ...)" to their code and gets multiple "duplicate symbol" errors. It is really not obvious that might cause them, so they are stuck. You have to know the linker's internals to understand how the error, the pragma, and the switch are connected. For me, that is evidence of the imperfect design of the software.
The patch aims not to add complexity, but rather to make things more straight from the user's perspective. Any temporal states should work only within the direct range in the command line. Their applicants should be obvious. They should not affect hidden dependencies because that influence is not direct and not obvious. The patch does that only for --whole-archive, but maybe it would be better to fully restore the initial state. I just do not have a good example to justify that.
clang-format: please reformat the code