[PATCH] D23015: [ThinLTO/gold] Don't drop preempted symbols early

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

I discovered while merging this patch with head on Friday that the Resolution-based LTO API I committed for pcc at r278338 already fixes this issue in the same way.

However, the following aspect is still an issue:

-----

In https://reviews.llvm.org/D23015#503394, @tejohnson wrote:

> I have made a change to the gold-plugin to keep the preempted symbols as well. However, this exposed another issue: when we keep the preempted linkonce in the comdat, thinLTOResolveWeakForLinkerModule comes along and changes it to available_externally. We then get a verification failure about a declaration in a comdat.
>
> Interestingly, we have a fix for this in renameModuleForThinLTO, which drops any available_externally from comdats. It is meant to clean up after importing, but if I reorder renameModuleForThinLTO and thinLTOResolveWeakForLinkerModule in gold-plugin, it fixes this particular issue. I managed to reproduce this issue with an llvm-lto test case, since ProcessThinLTOModule() in ThinLTOCodeGenerator.cpp has the same ordering. It is also fixed by flipping the ordering between these two. Interestingly, and probably completely accidentally, ThinLTOCodeGenerator::promote() already has the desirable ordering. However, I don't like the subtle ordering requirement between these two calls. Perhaps it is better to explicitly strip any new available_externally symbols from comdats in thinLTOResolveWeakForLinkerModule. Hmm, I think I like that approach better. WDYT? Let me do that, will do so and clean up and add the new test cases to the patches later today.


----

It turns out that with pcc's changes we get the fortuitous ordering of renameModuleForThinLTO after thinLTOResolveWeakForLinkerModule in the new lto::thinBackend, so the issue is not exposed via the gold-plugin. It is however, still an issue in llvm-lto (and I realized in looking into this that I had forgotten to add my new llvm-lto based test case to this patch).

I am going to change this patch to only include this particular fix, along with the llvm-lto based test, and submit it.


https://reviews.llvm.org/D23015