[PATCH] D70006: [ThinLTO] Fix bug when importing writeonly variables

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

We hit a linker undef also because of D69561 <https://reviews.llvm.org/D69561>. I just confirmed this patch fixes the issue. It would be good to get this patch in asap or revert the original change. I have some questions on this patch though.

I'm a little confused about what causes the original failure mode, and whether this patch is an enhancement that essentially hides the underlying bug or is really fixing the underlying issue. Since D69561 <https://reviews.llvm.org/D69561> only affected read only variables, how did it affect a write only variable?



================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:322
         // Promote referenced functions and variables
-        for (const auto &VI : RefSummary->refs())
-          for (const auto &RefFn : VI.getSummaryList())
-            MarkExported(VI, RefFn.get());
+        if (!Index.isWriteOnly(cast<GlobalVarSummary>(RefSummary.get())))
+          for (const auto &VI : RefSummary->refs())
----------------
Needs comment


================
Comment at: llvm/lib/Transforms/Utils/FunctionImportUtils.cpp:259
         V->addAttribute("thinlto-internalize");
+        if (ImportIndex.isWriteOnly(GVS)) {
+          auto *VTy = V->getValueType();
----------------
Needs comment


================
Comment at: llvm/lib/Transforms/Utils/FunctionImportUtils.cpp:261
+          auto *VTy = V->getValueType();
+          if (VTy->isStructTy() || VTy->isArrayTy())
+            V->setInitializer(ConstantAggregateZero::get(VTy));
----------------
What if it isn't a struct or array? Is that the only situation where we can hit this problem? 


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

https://reviews.llvm.org/D70006