Looks good to me.

Addressed review comments.

Addressed review comments.

Applied code review suggestions.

Addressed review comments.

Addressed review comments.

I agree that it would be better to create a single LIT test for a file name beginning with dash, but it is not clear how to write it. I assume such test would require use of %T (directory of a %t), but according to the documentation it is deprecated now. So, I decided to follow the same strategy for testing this patch as was used in a similar llvm-rc change (D56743) - updating few arbitrary tests to use ‘--’ separator for the file names. If you have ideas how to write a test for a file name beginning with ‘-‘, please share.

With this change OMPIRBuilder may produce invalid IR if existing function attributes conflict with the new attributes it is trying to add. The problem can be reproduced on llvm/test/Transforms/OpenMP/gpu_state_machine_function_ptr_replacement.ll test where __kmpc_global_thread_num function is declared with the following attributes

Addressed review comments.

Given the langref text I'm unsure what it means to link an append linkage variable to a non-append linkage one. I can see how extern is different but what would happen before & now if you remove the extern.

ping

In D94643#2498870, @tra wrote:

LGTM.

Do you have any measurements showing actual impact on performance or memory consumption?

Addressed review comment.

In D92535#2471222, @saiislam wrote:

Looks good to me. Please wait for @tra to accept it.

Well, it looks like nobody is going to respond.

ping

Updated diff after committing NFC changes (D92918).

I wonder if we want to go even further and bring archive linking closer to how static linking works with normal object files?
Instead of linking all files in the archive together (that may have its own set of issues) and returning a single module, perhaps we should just load individual files, return them as a vector and link each of them in order with --only-needed.

Well, when you use a class you have to know the interface it defines, so this is not about the implementation details, but about the interface provided by the class. llvm::Linker::linkModules(), as it is defined today, is a static member of the class, so it does not require an instance of llvm::Linker class to be called. Adding an additional parameter to loadArFile just in case does not buy as anything at all except unneeded redundancy. And, by the way, this function is called as a static member (i.e. Linker::linkModules()) everywhere in LLVM sources except llvm-link.cpp:

I have prepared a separate patch for NFC changes D92918. Will update this patch once D92918 is approved.

In D92535#2440317, @tra wrote:

The patch description describes what the patch does, but does not tell us much about the problem is is supposed to fix.
Could you give us more details on why the patch is needed?

Do you have any comments for this patch?

I have realized that this patch is not NFC, and it corrects llvm-link behavior when archive is linked with other inputs as a library (with --only-needed option). I have updated description and added LIT test to the patch.

Added BinaryFormat to the list of components that need to be linked.

Added unit test for the CallGraphNode::replaceCallEdge() change. Without this change test finishes with an assertion on windows debug build.

Yes, exactly. If the number of callbacks is the same we can avoid invalidating iterators. I assume that should really cover most of the cases.

LGTM

Rebase after committing NFC patch (D82686).

Sure, I have separated NFC changes to D82686.