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