[PATCH] D88877: [ELF] Drop --whole-archive before processing dependent libraries.

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added a comment.

In D88877#2350507 <https://reviews.llvm.org/D88877#2350507>, @MaskRay wrote:

> 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.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88877/new/

https://reviews.llvm.org/D88877