[PATCH] D34448: bug33527 - Linker::LinkOnlyNeeded should import AppendingLinkage globals

[Benoit Belley via Phabricator via llvm-commits]

belleyb added inline comments.


================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:35
 #include "llvm/LTO/LTO.h"
-#include "llvm/Linker/Linker.h"
 #include "llvm/MC/SubtargetFeature.h"
----------------
Linker isn't used by ThinLTO. Include can be removed.


================
Comment at: lib/Linker/LinkModules.cpp:334
+    // Always import variables with appending linkage.
+    if (!GV.hasAppendingLinkage()) {
+      // Don't import globals unless they are referenced by the destination
----------------
This extra check is the fix (Line 334). Without it, the unit tests below are failing.

The rest of the changes around is only to make the code more readable, allowing me to better document the intent,


================
Comment at: unittests/Linker/LinkModulesTest.cpp:364
 
+TEST_F(LinkModuleTest, ImportIntrinsicGlobalVariables_ctors1) {
+  SMDiagnostic Err;
----------------
Starting from here are added unit tests demonstrating the actual issue and proving the suggested fix is correct.

I was wondering if it would have been more appropriate to write those using llvm-link and FileCheck instead. Let me know if you would prefer that alternate approach instead.


https://reviews.llvm.org/D34448