[lld] r230194 - [ELF] Add .gnu.linkonce support.
Shankar Easwaran
shankare at codeaurora.org
Mon Feb 23 04:51:11 PST 2015
Thanks for reviewing the patch.
On 2/22/2015 9:20 PM, Rui Ueyama wrote:
>
> Scope scope() const override {
> + if (!_symbol)
> + return scopeGlobal;
>
> How can this condition happen? Looks like previously _symbol was guaranteed
> to be non-null. Now, it looks like a null pointer can be assigned to
> _symbol so you need null check everywhere.
This is used by the atom thats used for holding the group signature atom
and the gnu linkonce signature atom.
This is used only by by functions which return properties of the symbol,
which is already taken care in the patch.
>
>> if (_symbol->getVisibility() == llvm::ELF::STV_HIDDEN)
>> return scopeLinkageUnit;
>> else if (_symbol->getBinding() != llvm::ELF::STB_LOCAL)
>> @@ -199,6 +201,9 @@ public:
>> Interposable interposable() const override { return interposeNo; }
>>
>> Merge merge() const override {
>> + if (!_symbol)
>> + return mergeNo;
>> +
>> if (_symbol->getBinding() == llvm::ELF::STB_WEAK)
>> return mergeAsWeak;
>>
>> @@ -216,6 +221,9 @@ public:
>> ContentType ret = typeUnknown;
>> uint64_t flags = _section->sh_flags;
>>
>> + if (!_symbol && _sectionName.startswith(".gnu.linkonce"))
>>
> In the commit message you wrote that sections starts with "gnu.linkonce."
> is GNU linkonce sections. You dropped the last "." (dot) here.
Good catch, I will fix this.
>
>> + return typeGnuLinkOnce;
>> +
>> if (!(flags & llvm::ELF::SHF_ALLOC))
>> return _contentType = typeNoAlloc;
>>
>> @@ -286,6 +294,9 @@ public:
>> }
>>
>> Alignment alignment() const override {
>> + if (!_symbol)
>> + return Alignment(0);
>> +
>> // Obtain proper value of st_value field.
>> const auto symValue = getSymbolValue(_symbol);
>>
>> @@ -323,7 +334,7 @@ public:
>>
>> StringRef customSectionName() const override {
>> if ((contentType() == typeZeroFill) ||
>> - (_symbol->st_shndx == llvm::ELF::SHN_COMMON))
>> + (_symbol && _symbol->st_shndx == llvm::ELF::SHN_COMMON))
>> return ".bss";
>> return _sectionName;
>> }
>>
>> Modified: lld/trunk/lib/ReaderWriter/ELF/ELFFile.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/ELF/ELFFile.h?rev=230194&r1=230193&r2=230194&view=diff
>>
>> ==============================================================================
>> --- lld/trunk/lib/ReaderWriter/ELF/ELFFile.h (original)
>> +++ lld/trunk/lib/ReaderWriter/ELF/ELFFile.h Sun Feb 22 18:04:49 2015
>> @@ -168,8 +168,23 @@ public:
>> return _absoluteAtoms;
>> }
>>
>> - Atom *findAtom(const Elf_Sym *symbol) {
>> - return _symbolToAtomMapping.lookup(symbol);
>> + Atom *findAtom(const Elf_Sym *sourceSymbol, const Elf_Sym
>> *targetSymbol) {
>> + // All references to atoms inside a group are through undefined atoms.
>> + Atom *targetAtom = _symbolToAtomMapping.lookup(targetSymbol);
>> + if (targetAtom->definition() != Atom::definitionRegular)
>> + return targetAtom;
>> + if ((llvm::dyn_cast<DefinedAtom>(targetAtom))->scope() ==
>> + DefinedAtom::scopeTranslationUnit)
>> + return targetAtom;
>> + if (!redirectReferenceUsingUndefAtom(sourceSymbol, targetSymbol))
>> + return targetAtom;
>> + auto undefForGroupchild =
>> _undefAtomsForgroupChild.find(targetAtom->name());
>> + if (undefForGroupchild != _undefAtomsForgroupChild.end())
>> + return undefForGroupchild->getValue();
>> + auto undefGroupChildAtom =
>> + new (_readerStorage) SimpleUndefinedAtom(*this,
>> targetAtom->name());
>> + _undefinedAtoms._atoms.push_back(undefGroupChildAtom);
>> + return (_undefAtomsForgroupChild[targetAtom->name()] =
>> undefGroupChildAtom);
>> }
>>
> This really needs a comment to describe what the intention of the
> conditions is.
Sure, I will point to the COMDAT spec listed outside the ELF spec, as
the ELF spec is so out of date.
>
>
>> protected:
>> @@ -258,6 +273,12 @@ protected:
>> return shdr && (shdr->sh_type == llvm::ELF::SHT_PROGBITS) &&
>> syms.empty();
>> }
>>
>> + /// Handle creation of atoms for .gnu.linkonce sections.
>> + std::error_code handleGnuLinkOnceSection(
>> + StringRef sectionName,
>> + llvm::StringMap<std::vector<ELFDefinedAtom<ELFT> *>>
>> &atomsForSection,
>> + const Elf_Shdr *shdr);
>> +
>> /// Process the Undefined symbol and create an atom for it.
>> ErrorOr<ELFUndefinedAtom<ELFT> *>
>> handleUndefinedSymbol(StringRef symName, const Elf_Sym *sym) {
>> @@ -286,6 +307,11 @@ protected:
>> symbol->st_shndx == llvm::ELF::SHN_COMMON;
>> }
>>
>> + /// Returns true if the section is a gnulinkonce section.
>> + bool isGnuLinkOnceSection(StringRef sectionName) const {
>> + return sectionName.startswith(".gnu.linkonce");
>> + }
>> +
>> /// Returns correct st_value for the symbol depending on the
>> architecture.
>> /// For most architectures it's just a regular st_value with no changes.
>> virtual uint64_t getSymbolValue(const Elf_Sym *symbol) const {
>> @@ -333,6 +359,10 @@ protected:
>> return mergeAtom;
>> }
>>
>> + /// Does the atom need to be redirected using a separate undefined atom
>> ?
>>
> Please remove the space before "?"
>
> Why do we sometimes want to redirect a symbol to an undefined atom? Needs
> an explanation here.
I will add comment from the COMDAT document. References to functions
*from *atoms outside the group need to go through an undefined reference.
>
>
>> + bool redirectReferenceUsingUndefAtom(const Elf_Sym *sourceSymbol,
>> + const Elf_Sym *targetSymbol) const;
>> +
>> llvm::BumpPtrAllocator _readerStorage;
>> std::unique_ptr<llvm::object::ELFFile<ELFT> > _objFile;
>> atom_collection_vector<DefinedAtom> _definedAtoms;
>> @@ -350,6 +380,11 @@ protected:
>> std::unordered_map<StringRef, range<Elf_Rel_Iter>>
>> _relocationReferences;
>> std::vector<ELFReference<ELFT> *> _references;
>> llvm::DenseMap<const Elf_Sym *, Atom *> _symbolToAtomMapping;
>> + // Group child atoms have a pair corresponding to the signature and the
>> + // section header of the section that was used for generating the
>> signature.
>> + llvm::DenseMap<const Elf_Sym *, std::pair<StringRef, const Elf_Shdr *>>
>> + _groupChild;
>> + llvm::StringMap<Atom *> _undefAtomsForgroupChild;
>>
> undefAtomsForgroupChild -> undefAtomsForGroupChild
Sure.
>
>
>> /// \brief Atoms that are created for a section that has the merge
>> property
>> /// set
>> @@ -623,6 +658,11 @@ std::error_code ELFFile<ELFT>::createSym
>> }
>>
>> 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;
>> + // group sections have a mapping of the section header to the signature.
>> + llvm::DenseMap<const Elf_Shdr *, StringRef> groupSections;
>> for (auto &i : _sectionSymbols) {
>> const Elf_Shdr *section = i.first;
>> std::vector<Elf_Sym_Iter> &symbols = i.second;
>> @@ -641,11 +681,21 @@ template <class ELFT> std::error_code EL
>> if (std::error_code ec = sectionContents.getError())
>> return ec;
>>
>> + bool addAtoms = true;
>> +
>> + if (isGnuLinkOnceSection(*sectionName)) {
>> + groupSections.insert(std::make_pair(section, *sectionName));
>> + addAtoms = false;
>> + }
>> +
>> if (handleSectionWithNoSymbols(section, symbols)) {
>> ELFDefinedAtom<ELFT> *newAtom =
>> createSectionAtom(section, *sectionName, *sectionContents);
>> - _definedAtoms._atoms.push_back(newAtom);
>> newAtom->setOrdinal(++_ordinal);
>> + if (addAtoms)
>> + _definedAtoms._atoms.push_back(newAtom);
>> + else
>> + atomsForSection[*sectionName].push_back(newAtom);
>> continue;
>> }
>>
>> @@ -693,8 +743,11 @@ template <class ELFT> std::error_code EL
>> auto definedMergeAtom = handleDefinedSymbol(
>> symbolName, *sectionName, &**si, section, symbolData,
>> _references.size(), _references.size(), _references);
>> - _definedAtoms._atoms.push_back(*definedMergeAtom);
>> (*definedMergeAtom)->setOrdinal(++_ordinal);
>> + if (addAtoms)
>> + _definedAtoms._atoms.push_back(*definedMergeAtom);
>> + else
>> + atomsForSection[*sectionName].push_back(*definedMergeAtom);
>> }
>> continue;
>> }
>> @@ -740,19 +793,60 @@ template <class ELFT> std::error_code EL
>> // is a weak atom.
>> previousAtom = anonAtom ? anonAtom : newAtom;
>>
>> - _definedAtoms._atoms.push_back(newAtom);
>> + if (addAtoms)
>> + _definedAtoms._atoms.push_back(newAtom);
>> + else
>> + atomsForSection[*sectionName].push_back(newAtom);
>> +
>> _symbolToAtomMapping.insert(std::make_pair(&*symbol, newAtom));
>> if (anonAtom) {
>> anonAtom->setOrdinal(++_ordinal);
>> - _definedAtoms._atoms.push_back(anonAtom);
>> + if (addAtoms)
>> + _definedAtoms._atoms.push_back(anonAtom);
>> + else
>> + atomsForSection[*sectionName].push_back(anonAtom);
>> }
>> }
>> }
>>
>> + // Iterate over all the group sections to create parent atoms pointing
>> to
>> + // group-child atoms.
>> + for (auto § : groupSections) {
>> + StringRef signature = sect.second;
>> + if (isGnuLinkOnceSection(signature))
>> + handleGnuLinkOnceSection(signature, atomsForSection, sect.first);
>> + }
>> +
>> updateReferences();
>> return std::error_code();
>> }
>>
>> +template <class ELFT>
>> +std::error_code ELFFile<ELFT>::handleGnuLinkOnceSection(
>>
> This function never fails, so it should return void isntead of error_code.
I need to add error checks to check for an invalid ELF symbol. I will go
ahead and return errors, but its so difficult to create test cases using
ELF object files.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150223/d91283ae/attachment.html>
More information about the llvm-commits
mailing list