<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, May 14, 2015 at 12:23 PM, Adhemerval Zanella <span dir="ltr"><<a href="mailto:adhemerval.zanella@linaro.org" target="_blank">adhemerval.zanella@linaro.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Rui and Shankar,<br>
<br>
While debugging some test-suite SingleSource C++ EH cases when linked with<br>
lld [1] I noted that lld is applying wrong relocation on the text segments<br>
if there is multiple .text segments in the same object (the COMDAT ones).<br>
<br>
It is due in the mapping from Segments and Relocation, only the segment<br>
name is considered and for this cases the relocation for segment '.text'<br>
will be overwritten by last COMDAT being handled.<br>
<br>
The patch changes this by considering the segment index references in the<br>
group section while creating the mapping.  It also adds a small optimization<br>
on handleSectionGroup by avoiding using the same, but rather the segment<br>
header in the sections mapping (it should be no code generation changes for<br>
this one).<br>
<br>
Although I could not make the C++ EH testcases work, the relocation seems<br>
correct and the right function are now being correctly called. I think<br>
lld is messing up with eh_frame and CFE generation, since the programs<br>
now crashed in the stack unwind code from libstdc++, rather than an<br>
illegal function.<br>
<br>
And I putting this change on RFC because I am not sure if this is the<br>
best strategy to handle group segments (I got the idea from my understanding<br>
of binutils), neither how to effectively test it without restricting to<br>
one platform.  Also, if someone could point me on how lld may be not<br>
generating correct unwind information, I would appreciate.<br>
<br>
[1] <a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2015-May/085311.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvmdev/2015-May/085311.html</a><br>
<br>
--<br>
<br>
diff --git a/lib/ReaderWriter/ELF/ELFFile.cpp b/lib/ReaderWriter/ELF/ELFFile.cpp<br>
index fd2ef71..9d9f588 100644<br>
--- a/lib/ReaderWriter/ELF/ELFFile.cpp<br>
+++ b/lib/ReaderWriter/ELF/ELFFile.cpp<br>
@@ -10,6 +10,7 @@<br>
 #include "ELFFile.h"<br>
 #include "FileCommon.h"<br>
 #include "llvm/ADT/STLExtras.h"<br>
+#include "llvm/ADT/StringExtras.h"<br>
<br>
 namespace lld {<br>
 namespace elf {<br>
@@ -143,7 +144,12 @@ std::error_code ELFFile<ELFT>::createAtomizableSections() {<br>
       auto rai(_objFile->begin_rela(&section));<br>
       auto rae(_objFile->end_rela(&section));<br>
<br>
-      _relocationAddendReferences[*sectionName] = make_range(rai, rae);<br>
+      std::string sectionKey = *sectionName;<br>
+      if (isSectionMemberOfGroup(&section))<br>
+        sectionKey += llvm::utohexstr(section.sh_info);<br>
+<br>
+      _relocationAddendReferences[sectionKey] = make_range(rai, rae);<br></blockquote><div><br></div><div>This piece of code appears many times in this patch. Can you make it a function?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
       totalRelocs += std::distance(rai, rae);<br>
     } else if (section.sh_type == llvm::ELF::SHT_REL) {<br>
       auto sHdr = _objFile->getSection(section.sh_info);<br>
@@ -155,7 +161,12 @@ std::error_code ELFFile<ELFT>::createAtomizableSections() {<br>
       auto ri(_objFile->begin_rel(&section));<br>
       auto re(_objFile->end_rel(&section));<br>
<br>
-      _relocationReferences[*sectionName] = make_range(ri, re);<br>
+      std::string sectionKey = *sectionName;<br>
+      if (isSectionMemberOfGroup(&section))<br>
+        sectionKey += llvm::utohexstr(section.sh_info);<br>
+<br>
+      _relocationReferences[sectionKey] = make_range(ri, re);<br>
+<br>
       totalRelocs += std::distance(ri, re);<br>
     } else {<br>
       _sectionSymbols[&section];<br>
@@ -259,12 +270,16 @@ std::error_code ELFFile<ELFT>::createSymbolsFromAtomizableSections() {<br>
 template <class ELFT> std::error_code ELFFile<ELFT>::createAtoms() {<br>
   // Holds all the atoms that are part of the section. They are the targets of<br>
   // the kindGroupChild reference.<br>
-  llvm::StringMap<std::vector<ELFDefinedAtom<ELFT> *>> atomsForSection;<br>
+  AtomsForSectionMap atomsForSection;<br>
<br>
-  // Contains a list of comdat sections for a group.<br>
-  for (auto &i : _sectionSymbols) {<br>
-    const Elf_Shdr *section = i.first;<br>
-    std::vector<Elf_Sym_Iter> &symbols = i.second;<br>
+  // Contains a list of comdat sections for a group.  The sectionIdx is used<br>
+  // to keep track of section ordering to match group section in the References<br>
+  // match when creating ElfDefineAtom.<br>
+  int sectionIdx = 1;<br>
+  for (auto i = _sectionSymbols.begin();<br>
+       i != _sectionSymbols.end(); ++i, ++sectionIdx) {<br></blockquote><div><br></div><div>Cache the end iterator.</div><div><br></div><div>for (auto i = _sectionSymbols.begin(), e = _sectionSymbols.end(); i != e; ...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+    const Elf_Shdr *section = (*i).first;<br>
+    std::vector<Elf_Sym_Iter> &symbols = (*i).second;<br>
<br>
     // Sort symbols by position.<br>
     std::stable_sort(symbols.begin(), symbols.end(),<br>
@@ -294,7 +309,7 @@ template <class ELFT> std::error_code ELFFile<ELFT>::createAtoms() {<br>
       if (addAtoms)<br>
         addAtom(*newAtom);<br>
       else<br>
-        atomsForSection[*sectionName].push_back(newAtom);<br>
+        atomsForSection[section].push_back(newAtom);<br>
       continue;<br>
     }<br>
<br>
@@ -347,7 +362,7 @@ template <class ELFT> std::error_code ELFFile<ELFT>::createAtoms() {<br>
         if (addAtoms)<br>
           addAtom(*atom);<br>
         else<br>
-          atomsForSection[*sectionName].push_back(atom);<br>
+          atomsForSection[section].push_back(atom);<br>
         continue;<br>
       }<br>
<br>
@@ -361,7 +376,7 @@ template <class ELFT> std::error_code ELFFile<ELFT>::createAtoms() {<br>
         auto sym = new (_readerStorage) Elf_Sym(*symbol);<br>
         sym->setBinding(llvm::ELF::STB_GLOBAL);<br>
         anonAtom = createDefinedAtomAndAssignRelocations(<br>
-            "", *sectionName, sym, section, symbolData, *sectionContents);<br>
+            "", *sectionName, sym, section, sectionIdx, symbolData, *sectionContents);<br>
         symbolData = ArrayRef<uint8_t>();<br>
<br>
         // If this is the last atom, let's not create a followon reference.<br>
@@ -373,7 +388,7 @@ template <class ELFT> std::error_code ELFFile<ELFT>::createAtoms() {<br>
       }<br>
<br>
       ELFDefinedAtom<ELFT> *newAtom = createDefinedAtomAndAssignRelocations(<br>
-          symbolName, *sectionName, &*symbol, section, symbolData,<br>
+          symbolName, *sectionName, &*symbol, section, sectionIdx, symbolData,<br>
           *sectionContents);<br>
       newAtom->setOrdinal(++_ordinal);<br>
<br>
@@ -395,7 +410,7 @@ template <class ELFT> std::error_code ELFFile<ELFT>::createAtoms() {<br>
       if (addAtoms)<br>
         addAtom(*newAtom);<br>
       else<br>
-        atomsForSection[*sectionName].push_back(newAtom);<br>
+        atomsForSection[section].push_back(newAtom);<br>
<br>
       _symbolToAtomMapping.insert(std::make_pair(&*symbol, newAtom));<br>
       if (anonAtom) {<br>
@@ -403,7 +418,7 @@ template <class ELFT> std::error_code ELFFile<ELFT>::createAtoms() {<br>
         if (addAtoms)<br>
           addAtom(*anonAtom);<br>
         else<br>
-          atomsForSection[*sectionName].push_back(anonAtom);<br>
+          atomsForSection[section].push_back(anonAtom);<br>
       }<br>
     }<br>
   }<br>
@@ -422,7 +437,7 @@ template <class ELFT> std::error_code ELFFile<ELFT>::createAtoms() {<br>
 template <class ELFT><br>
 std::error_code ELFFile<ELFT>::handleGnuLinkOnceSection(<br>
     const Elf_Shdr *section,<br>
-    llvm::StringMap<std::vector<ELFDefinedAtom<ELFT> *>> &atomsForSection) {<br>
+    AtomsForSectionMap &atomsForSection) {<br>
   ErrorOr<StringRef> sectionName = this->getSectionName(section);<br>
   if (std::error_code ec = sectionName.getError())<br>
     return ec;<br>
@@ -431,14 +446,14 @@ std::error_code ELFFile<ELFT>::handleGnuLinkOnceSection(<br>
<br>
   unsigned int referenceStart = _references.size();<br>
   std::vector<ELFReference<ELFT> *> refs;<br>
-  for (auto ha : atomsForSection[*sectionName]) {<br>
+  for (auto ha : atomsForSection[section]) {<br>
     _groupChild[ha->symbol()] = std::make_pair(*sectionName, section);<br>
     ELFReference<ELFT> *ref =<br>
         new (_readerStorage) ELFReference<ELFT>(Reference::kindGroupChild);<br>
     ref->setTarget(ha);<br>
     refs.push_back(ref);<br>
   }<br>
-  atomsForSection[*sectionName].clear();<br>
+  atomsForSection[section].clear();<br>
   // Create a gnu linkonce atom.<br>
   ELFDefinedAtom<ELFT> *atom = createDefinedAtom(<br>
       *sectionName, *sectionName, nullptr, section, ArrayRef<uint8_t>(),<br>
@@ -453,7 +468,7 @@ std::error_code ELFFile<ELFT>::handleGnuLinkOnceSection(<br>
 template <class ELFT><br>
 std::error_code ELFFile<ELFT>::handleSectionGroup(<br>
     const Elf_Shdr *section,<br>
-    llvm::StringMap<std::vector<ELFDefinedAtom<ELFT> *>> &atomsForSection) {<br>
+    AtomsForSectionMap &atomsForSection) {<br>
   ErrorOr<StringRef> sectionName = this->getSectionName(section);<br>
   if (std::error_code ec = sectionName.getError())<br>
     return ec;<br>
@@ -473,16 +488,13 @@ std::error_code ELFFile<ELFT>::handleSectionGroup(<br>
   // member contains the symbol table index of the identifying entry.<br>
   // The sh_flags member of the section header contains 0. The name of<br>
   // the section (sh_name) is not specified.<br>
-  std::vector<StringRef> sectionNames;<br>
+  std::vector<const Elf_Shdr *> sections;<br>
   const Elf_Word *groupMembers =<br>
       reinterpret_cast<const Elf_Word *>(sectionContents->data());<br>
   const size_t count = section->sh_size / sizeof(Elf_Word);<br>
   for (size_t i = 1; i < count; i++) {<br>
     const Elf_Shdr *shdr = _objFile->getSection(groupMembers[i]);<br>
-    ErrorOr<StringRef> sectionName = _objFile->getSectionName(shdr);<br>
-    if (std::error_code ec = sectionName.getError())<br>
-      return ec;<br>
-    sectionNames.push_back(*sectionName);<br>
+    sections.push_back(shdr);<br>
   }<br>
   const Elf_Sym *symbol = _objFile->getSymbol(section->sh_info);<br>
   const Elf_Shdr *symtab = _objFile->getSection(section->sh_link);<br>
@@ -492,15 +504,15 @@ std::error_code ELFFile<ELFT>::handleSectionGroup(<br>
<br>
   unsigned int referenceStart = _references.size();<br>
   std::vector<ELFReference<ELFT> *> refs;<br>
-  for (auto name : sectionNames) {<br>
-    for (auto ha : atomsForSection[name]) {<br>
+  for (auto sec : sections) {<br>
+    for (auto ha : atomsForSection[sec]) {<br>
       _groupChild[ha->symbol()] = std::make_pair(*symbolName, section);<br>
       ELFReference<ELFT> *ref =<br>
           new (_readerStorage) ELFReference<ELFT>(Reference::kindGroupChild);<br>
       ref->setTarget(ha);<br>
       refs.push_back(ref);<br>
     }<br>
-    atomsForSection[name].clear();<br>
+    atomsForSection[sec].clear();<br>
   }<br>
<br>
   // Create an atom for comdat signature.<br>
@@ -549,17 +561,21 @@ template <class ELFT> std::error_code ELFFile<ELFT>::createAtomsFromContext() {<br>
 template <class ELFT><br>
 ELFDefinedAtom<ELFT> *ELFFile<ELFT>::createDefinedAtomAndAssignRelocations(<br>
     StringRef symbolName, StringRef sectionName, const Elf_Sym *symbol,<br>
-    const Elf_Shdr *section, ArrayRef<uint8_t> symContent,<br>
+    const Elf_Shdr *section, int sectionIdx, ArrayRef<uint8_t> symContent,<br>
     ArrayRef<uint8_t> secContent) {<br>
   unsigned int referenceStart = _references.size();<br>
<br>
+  std::string sectionKey = sectionName;<br>
+  if (isSectionMemberOfGroup(section))<br>
+    sectionKey += llvm::utohexstr(sectionIdx);<br>
+<br>
   // Add Rela (those with r_addend) references:<br>
-  auto rari = _relocationAddendReferences.find(sectionName);<br>
+  auto rari = _relocationAddendReferences.find(sectionKey);<br>
   if (rari != _relocationAddendReferences.end())<br>
     createRelocationReferences(symbol, symContent, rari->second);<br>
<br>
   // Add Rel references.<br>
-  auto rri = _relocationReferences.find(sectionName);<br>
+  auto rri = _relocationReferences.find(sectionKey);<br>
   if (rri != _relocationReferences.end())<br>
     createRelocationReferences(symbol, symContent, secContent, rri->second);<br>
<br>
@@ -691,7 +707,7 @@ ELFFile<ELFT>::createSectionAtom(const Elf_Shdr *section, StringRef sectionName,<br>
   sym->st_value = 0;<br>
   sym->st_size = 0;<br>
   auto *newAtom = createDefinedAtomAndAssignRelocations(<br>
-      "", sectionName, sym, section, content, content);<br>
+      "", sectionName, sym, section, -1, content, content);<br>
   newAtom->setOrdinal(++_ordinal);<br>
   return newAtom;<br>
 }<br>
diff --git a/lib/ReaderWriter/ELF/ELFFile.h b/lib/ReaderWriter/ELF/ELFFile.h<br>
index 93ade6b..3b1b94a 100644<br>
--- a/lib/ReaderWriter/ELF/ELFFile.h<br>
+++ b/lib/ReaderWriter/ELF/ELFFile.h<br>
@@ -128,9 +128,12 @@ public:<br>
   Atom *findAtom(const Elf_Sym *sourceSym, const Elf_Sym *targetSym);<br>
<br>
 protected:<br>
+  typedef llvm::DenseMap<const Elf_Shdr *, std::vector<ELFDefinedAtom<ELFT> *>><br>
+    AtomsForSectionMap;<br>
+<br>
   ELFDefinedAtom<ELFT> *createDefinedAtomAndAssignRelocations(<br>
       StringRef symbolName, StringRef sectionName, const Elf_Sym *symbol,<br>
-      const Elf_Shdr *section, ArrayRef<uint8_t> symContent,<br>
+      const Elf_Shdr *sectionHdr, int sectionIdx, ArrayRef<uint8_t> symContent,<br>
       ArrayRef<uint8_t> secContent);<br>
<br>
   std::error_code doParse() override;<br>
@@ -212,12 +215,12 @@ protected:<br>
   /// Handle creation of atoms for .gnu.linkonce sections.<br>
   std::error_code handleGnuLinkOnceSection(<br>
       const Elf_Shdr *section,<br>
-      llvm::StringMap<std::vector<ELFDefinedAtom<ELFT> *>> &atomsForSection);<br>
+      AtomsForSectionMap &atomsForSection);<br>
<br>
   // Handle COMDAT scetions.<br>
   std::error_code handleSectionGroup(<br>
       const Elf_Shdr *section,<br>
-      llvm::StringMap<std::vector<ELFDefinedAtom<ELFT> *>> &atomsForSection);<br>
+      AtomsForSectionMap &atomsForSection);<br>
<br>
   /// Process the Undefined symbol and create an atom for it.<br>
   ELFUndefinedAtom<ELFT> *createUndefinedAtom(StringRef symName,<br>
@@ -345,10 +348,11 @@ protected:<br>
   /// list of relocations references.  In ELF, if a section named, ".text" has<br>
   /// relocations will also have a section named ".rel.text" or ".rela.text"<br>
   /// which will hold the entries.<br>
-  std::unordered_map<StringRef, range<Elf_Rela_Iter>><br>
-  _relocationAddendReferences;<br>
+  std::unordered_map<std::string, range<Elf_Rela_Iter>><br>
+    _relocationAddendReferences;<br>
+  std::unordered_map<std::string, range<Elf_Rel_Iter>><br>
+    _relocationReferences;<br>
   MergedSectionMapT _mergedSectionMap;<br>
-  std::unordered_map<StringRef, range<Elf_Rel_Iter>> _relocationReferences;<br>
   std::vector<ELFReference<ELFT> *> _references;<br>
   llvm::DenseMap<const Elf_Sym *, Atom *> _symbolToAtomMapping;<br>
   llvm::DenseMap<const ELFReference<ELFT> *, const Elf_Sym *><br>
</blockquote></div><br></div></div>