[lib/LTO] Remove now unneded hack for undefined symbols

[davide]

Summary

As already noted by Rafael, once https://reviews.llvm.org/D24594 will land, we won't need this anymore.

[mehdi_amini]

lib/LTO/LTO.cpp

341

I wonder if we should instead assert that we don't have an undefined symbol with a prevailing resolution.

[davide]

I don't have a strong preference. Peter?

[tejohnson]

lib/LTO/LTO.cpp

341

That sounds like a great idea - that's what intrigued me in the first place and what I was trying to figure out in the debugger yesterday for PR30363.

[mehdi_amini]

I'll reformulate: what is the semantic of having a prevailing resolution for an undefined symbol?
If there is none we should error (or at least assert).

[davide]

In D24595#543368, @mehdi_amini wrote:

I'll reformulate: what is the semantic of having a prevailing resolution for an undefined symbol?
If there is none we should error (or at least assert).

I don't think an undefined can have a prevailing resolution, at least in lld and gold.

[davide]

Add an assertion, as per Mehdi suggestion.

[mehdi_amini]

lib/LTO/LTO.cpp

390

We should have the same assert in this loop (maybe in a separate patch)

[davide]

lib/LTO/LTO.cpp

390

Oh, sure :) I'm still not very familiar with Thin so I didn't think about it. Thanks!

[mehdi_amini]

lib/LTO/LTO.cpp

359

Is this ok with Sym.getFlags() & object::BasicSymbolRef::SF_Undefined?
The continue; you're removing makes us no longer short-circuiting this code.

359

I guess we can't have Common and undefined at the same time...

[pcc]

lib/LTO/LTO.cpp

390

Or you could add it to addSymbolToGlobalRes.

[tejohnson]

lib/LTO/LTO.cpp

359

In any case, it will assert now instead of continuing, so we still wouldn't hit this for an undefined (you might be still looking at the older version of the patch).

390

Ah that's a good idea, better to consolidate it there and keep these loops simple.

[davide]

So, are you OK with committing this assertion (after it's moved to addSymbolToGlobalRes())

[Eugene.Zelenko]

Looks like patch was not committed.

[davide]

The hack is still needed for lld, unfortunately. Abandoning.

lib/LTO/LTO.cpp

for (const InputFile::Symbol &Sym :
        make_range(InputFile::symbol_iterator(Obj->symbol_begin()),
                   InputFile::symbol_iterator(Obj->symbol_end()))) {
     assert(ResI != Res.end());
     SymbolResolution Res = *ResI++;
     addSymbolToGlobalRes(Obj.get(), Used, Sym, Res, 0);
 
     GlobalValue *GV = Obj->getSymbolGV(Sym.I->getRawDataRefImpl());
     if (Sym.getFlags() & object::BasicSymbolRef::SF_Undefined)
-      continue;
+      assert(!Res.Prevailing &&
+             "An undefined symbol cannot have prevailing resolution");
     if (Res.Prevailing && GV) {
       Keep.push_back(GV);
       switch (GV->getLinkage()) {
       default:
         break;
       case GlobalValue::LinkOnceAnyLinkage:
         GV->setLinkage(GlobalValue::WeakAnyLinkage);
         break;
       case GlobalValue::LinkOnceODRLinkage:
         GV->setLinkage(GlobalValue::WeakODRLinkage);
         break;
       }
     }
     // Common resolution: collect the maximum size/alignment.
     // FIXME: right now we ignore the prevailing information, it is not clear
     // what is the "right" behavior here.
     if (Sym.getFlags() & object::BasicSymbolRef::SF_Common) {
       auto &CommonRes = RegularLTO.Commons[Sym.getIRName()];
       CommonRes.Size = std::max(CommonRes.Size, Sym.getCommonSize());
       CommonRes.Align = std::max(CommonRes.Align, Sym.getCommonAlignment());
     }
 
     // FIXME: use proposed local attribute for FinalDefinitionInLinkageUnit.
   }
   assert(ResI == Res.end());
ErrorOr<std::unique_ptr<object::ModuleSummaryIndexObjectFile>>
       SummaryObjOrErr =
           object::ModuleSummaryIndexObjectFile::create(MBRef, Conf.DiagHandler);
   if (!SummaryObjOrErr)
     return errorCodeToError(SummaryObjOrErr.getError());
   ThinLTO.CombinedIndex.mergeFrom((*SummaryObjOrErr)->takeIndex(),
                                   ThinLTO.ModuleMap.size());
 
   auto ResI = Res.begin();
   for (const InputFile::Symbol &Sym : Input->symbols()) {
     assert(ResI != Res.end());
     SymbolResolution Res = *ResI++;
     addSymbolToGlobalRes(Input->Obj.get(), Used, Sym, Res,
                          ThinLTO.ModuleMap.size() + 1);
 
     GlobalValue *GV = Input->Obj->getSymbolGV(Sym.I->getRawDataRefImpl());
     if (Res.Prevailing && GV)
       ThinLTO.PrevailingModuleForGUID[GV->getGUID()] =