[lld] r230191 - [ELF] Add symbol to ELFReference.

Shankar Easwaran shankare at codeaurora.org
Mon Feb 23 11:38:29 PST 2015


Hi Rui,

Yes, and the detail of the implementation is not affecting the Core 
atom. Its a data structure thats used only and lives only within the 
ELFReader.

Shankar Easwaran

On 2/23/2015 1:32 PM, Rui Ueyama wrote:
> I'm sorry but the specification doesn't say anything about whether we need
> to add a field for a symbol to the reference. That's the detail of the
> implementation.
>
> On Mon, Feb 23, 2015 at 11:31 AM, Shankar Easwaran <shankare at codeaurora.org>
> wrote:
>
>> Hi Rui,
>>
>> Its in the specification and requirements of section groups/COMDAT.
>>
>> https://mentorembedded.github.io/cxx-abi/abi/prop-72-comdat.html
>>
>> This code is still needed as its used by the functionality for gnu
>> linkonce and section groups, and models them properly.
>>
>> Its still an ELFReference and not changing anything in the Core Atom model.
>>
>> Shankar Easwaran
>>
>>
>> On 2/23/2015 1:19 PM, Rui Ueyama wrote:
>>
>>> You submitted a patch without code review which introduced new notions
>>> such
>>> as "redirecting using a separate undefined atom" which was never discussed
>>> before. I'm still trying to get what that means and whether they are
>>> actually well designed or not.
>>>
>>> So, is this change needed? If you need to maintain a relationship between
>>> references and symbols only in the reader, you can make a map from
>>> references to symbols and use that, instead of adding a new field to store
>>> symbols to references.
>>>
>>> On Mon, Feb 23, 2015 at 10:42 AM, Shankar Easwaran <
>>> shankare at codeaurora.org>
>>> wrote:
>>>
>>>   Hi Rui,
>>>> Its not the name of the reference, We need the properties of the node
>>>> from
>>>> the reference.
>>>>
>>>> If the reference is from a node, which is in the same group as the
>>>> reference is, we dont need to redirect using a undefined reference,
>>>> otherwise it has to be routed through a undefined reference.
>>>>
>>>> Example :-
>>>>
>>>> If foo(node) calls bar(reference) which is in the same group, foo can
>>>> directly call bar.
>>>>
>>>> If baz(node) which is in a different group tries to call bar(reference),
>>>> it has to be redirected using a undefined reference.
>>>>
>>>> Shankar Easwaran
>>>>
>>>>
>>>> On 2/23/2015 12:31 PM, Rui Ueyama wrote:
>>>>
>>>>   My point is that usually when you want to know the name of a symbol to
>>>>> which a reference is pointing, you'd do ref->getTarget()->name() instead
>>>>> of
>>>>> adding a name to a reference. In LLD's model, symbol name is a property
>>>>> of
>>>>> the node (atom) and not a property of edge (reference).
>>>>>
>>>>> My question is that why you had to add a name to the reference, instead
>>>>> of
>>>>> using the usual way? What's special about the feature that you were
>>>>> trying
>>>>> to implement?
>>>>>
>>>>> On Mon, Feb 23, 2015 at 4:59 AM, Shankar Easwaran <
>>>>> shankare at codeaurora.org>
>>>>> wrote:
>>>>>
>>>>>    Hi Rui,
>>>>>
>>>>>> Thanks for the review.
>>>>>>
>>>>>> This is used to lookup the symbol/section, for a reference.
>>>>>>
>>>>>> This is used to check if the symbol/section that points to the
>>>>>> reference
>>>>>> is from outside the group (or) not in the same section group.
>>>>>>
>>>>>> More details are here FYI. https://mentorembedded.github.
>>>>>> io/cxx-abi/abi/prop-72-comdat.html
>>>>>>
>>>>>> Shankar Easwaran
>>>>>>
>>>>>>
>>>>>> On 2/22/2015 9:34 PM, Rui Ueyama wrote:
>>>>>>
>>>>>>    This patch looks really weird. Basically in the atom model a
>>>>>> reference
>>>>>>
>>>>>>> is
>>>>>>> a
>>>>>>> relocation, and an atom is a symbol + data. However, looks like this
>>>>>>> patch
>>>>>>> made a reference to have an item that it is pointing to. If so, that
>>>>>>> doesn't sound correct.
>>>>>>>
>>>>>>> Send this kind of nontrivial changes to pre-commit review first,
>>>>>>> please.
>>>>>>>
>>>>>>> On Sun, Feb 22, 2015 at 3:46 PM, Shankar Easwaran <
>>>>>>> shankare at codeaurora.org>
>>>>>>> wrote:
>>>>>>>
>>>>>>>     Author: shankare
>>>>>>>
>>>>>>>   Date: Sun Feb 22 17:46:21 2015
>>>>>>>> New Revision: 230191
>>>>>>>>
>>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=230191&view=rev
>>>>>>>> Log:
>>>>>>>> [ELF] Add symbol to ELFReference.
>>>>>>>>
>>>>>>>> Relocation handling need more information about the Symbol that we
>>>>>>>> are
>>>>>>>> creating
>>>>>>>> references for.
>>>>>>>>
>>>>>>>> No change in functionality.
>>>>>>>>
>>>>>>>> Modified:
>>>>>>>>         lld/trunk/lib/ReaderWriter/ELF/Atoms.h
>>>>>>>>         lld/trunk/lib/ReaderWriter/ELF/ELFFile.h
>>>>>>>>         lld/trunk/lib/ReaderWriter/ELF/Mips/MipsELFFile.h
>>>>>>>>
>>>>>>>> Modified: lld/trunk/lib/ReaderWriter/ELF/Atoms.h
>>>>>>>> URL:
>>>>>>>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/
>>>>>>>> ReaderWriter/ELF/Atoms.h?rev=230191&r1=230190&r2=230191&view=diff
>>>>>>>>
>>>>>>>> ============================================================
>>>>>>>> ==================
>>>>>>>> --- lld/trunk/lib/ReaderWriter/ELF/Atoms.h (original)
>>>>>>>> +++ lld/trunk/lib/ReaderWriter/ELF/Atoms.h Sun Feb 22 17:46:21 2015
>>>>>>>> @@ -35,23 +35,27 @@ template <typename ELFT> class ELFFile;
>>>>>>>>      template <class ELFT> class ELFReference : public Reference {
>>>>>>>>        typedef llvm::object::Elf_Rel_Impl<ELFT, false> Elf_Rel;
>>>>>>>>        typedef llvm::object::Elf_Rel_Impl<ELFT, true> Elf_Rela;
>>>>>>>> +  typedef llvm::object::Elf_Sym_Impl<ELFT> Elf_Sym;
>>>>>>>> +
>>>>>>>>      public:
>>>>>>>> -  ELFReference(const Elf_Rela *rela, uint64_t off,
>>>>>>>> Reference::KindArch
>>>>>>>> arch,
>>>>>>>> -               Reference::KindValue relocType, uint32_t idx)
>>>>>>>> -      : Reference(Reference::KindNamespace::ELF, arch, relocType),
>>>>>>>> +  ELFReference(const Elf_Sym *sym, const Elf_Rela *rela, uint64_t
>>>>>>>> off,
>>>>>>>> +               Reference::KindArch arch, Reference::KindValue
>>>>>>>> relocType,
>>>>>>>> +               uint32_t idx)
>>>>>>>> +      : Reference(Reference::KindNamespace::ELF, arch, relocType),
>>>>>>>> _sym(sym),
>>>>>>>>              _target(nullptr), _targetSymbolIndex(idx),
>>>>>>>> _offsetInAtom(off),
>>>>>>>>              _addend(rela->r_addend) {}
>>>>>>>>
>>>>>>>> -  ELFReference(uint64_t off, Reference::KindArch arch,
>>>>>>>> +  ELFReference(const Elf_Sym *sym, uint64_t off, Reference::KindArch
>>>>>>>> arch,
>>>>>>>>                     Reference::KindValue relocType, uint32_t idx)
>>>>>>>> -      : Reference(Reference::KindNamespace::ELF, arch, relocType),
>>>>>>>> +      : Reference(Reference::KindNamespace::ELF, arch, relocType),
>>>>>>>> _sym(sym),
>>>>>>>>              _target(nullptr), _targetSymbolIndex(idx),
>>>>>>>> _offsetInAtom(off),
>>>>>>>>              _addend(0) {}
>>>>>>>>
>>>>>>>>        ELFReference(uint32_t edgeKind)
>>>>>>>>            : Reference(Reference::KindNamespace::all,
>>>>>>>> Reference::KindArch::all,
>>>>>>>>                        edgeKind),
>>>>>>>> -        _target(nullptr), _targetSymbolIndex(0), _offsetInAtom(0),
>>>>>>>> _addend(0) {}
>>>>>>>> +        _sym(nullptr), _target(nullptr), _targetSymbolIndex(0),
>>>>>>>> +        _offsetInAtom(0), _addend(0) {}
>>>>>>>>
>>>>>>>>        uint64_t offsetInAtom() const override { return _offsetInAtom;
>>>>>>>> }
>>>>>>>>
>>>>>>>> @@ -70,7 +74,10 @@ public:
>>>>>>>>
>>>>>>>>        void setTarget(const Atom *newAtom) override { _target =
>>>>>>>> newAtom;
>>>>>>>> }
>>>>>>>>
>>>>>>>> +  const Elf_Sym *symbol() const { return _sym; }
>>>>>>>> +
>>>>>>>>      private:
>>>>>>>> +  const Elf_Sym *_sym;
>>>>>>>>        const Atom *_target;
>>>>>>>>        uint64_t _targetSymbolIndex;
>>>>>>>>        uint64_t _offsetInAtom;
>>>>>>>>
>>>>>>>> Modified: lld/trunk/lib/ReaderWriter/ELF/ELFFile.h
>>>>>>>> URL:
>>>>>>>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/
>>>>>>>> ReaderWriter/ELF/ELFFile.h?rev=230191&r1=230190&r2=230191&view=diff
>>>>>>>>
>>>>>>>> ============================================================
>>>>>>>> ==================
>>>>>>>> --- lld/trunk/lib/ReaderWriter/ELF/ELFFile.h (original)
>>>>>>>> +++ lld/trunk/lib/ReaderWriter/ELF/ELFFile.h Sun Feb 22 17:46:21
>>>>>>>> 2015
>>>>>>>> @@ -181,12 +181,12 @@ protected:
>>>>>>>>        std::error_code doParse() override;
>>>>>>>>
>>>>>>>>        /// \brief Iterate over Elf_Rela relocations list and create
>>>>>>>> references.
>>>>>>>> -  virtual void createRelocationReferences(const Elf_Sym &symbol,
>>>>>>>> +  virtual void createRelocationReferences(const Elf_Sym *symbol,
>>>>>>>>                                                ArrayRef<uint8_t>
>>>>>>>> content,
>>>>>>>>                                                range<Elf_Rela_Iter>
>>>>>>>> rels);
>>>>>>>>
>>>>>>>>        /// \brief Iterate over Elf_Rel relocations list and create
>>>>>>>> references.
>>>>>>>> -  virtual void createRelocationReferences(const Elf_Sym &symbol,
>>>>>>>> +  virtual void createRelocationReferences(const Elf_Sym *symbol,
>>>>>>>>                                                ArrayRef<uint8_t>
>>>>>>>> symContent,
>>>>>>>>                                                ArrayRef<uint8_t>
>>>>>>>> secContent,
>>>>>>>>                                                range<Elf_Rel_Iter>
>>>>>>>> rels);
>>>>>>>> @@ -796,12 +796,12 @@ ELFDefinedAtom<ELFT> *ELFFile<ELFT>::cre
>>>>>>>>        // Add Rela (those with r_addend) references:
>>>>>>>>        auto rari = _relocationAddendReferences.find(sectionName);
>>>>>>>>        if (rari != _relocationAddendReferences.end())
>>>>>>>> -    createRelocationReferences(*symbol, symContent, rari->second);
>>>>>>>> +    createRelocationReferences(symbol, symContent, rari->second);
>>>>>>>>
>>>>>>>>        // Add Rel references.
>>>>>>>>        auto rri = _relocationReferences.find(sectionName);
>>>>>>>>        if (rri != _relocationReferences.end())
>>>>>>>> -    createRelocationReferences(*symbol, symContent, secContent,
>>>>>>>> rri->second);
>>>>>>>> +    createRelocationReferences(symbol, symContent, secContent,
>>>>>>>> rri->second);
>>>>>>>>
>>>>>>>>        // Create the DefinedAtom and add it to the list of
>>>>>>>> DefinedAtoms.
>>>>>>>>        return *handleDefinedSymbol(symbolName, sectionName, symbol,
>>>>>>>> section,
>>>>>>>> @@ -810,35 +810,35 @@ ELFDefinedAtom<ELFT> *ELFFile<ELFT>::cre
>>>>>>>>      }
>>>>>>>>
>>>>>>>>      template <class ELFT>
>>>>>>>> -void ELFFile<ELFT>::createRelocationReferences(const Elf_Sym
>>>>>>>> &symbol,
>>>>>>>> +void ELFFile<ELFT>::createRelocationReferences(const Elf_Sym
>>>>>>>> *symbol,
>>>>>>>>                                                     ArrayRef<uint8_t>
>>>>>>>> content,
>>>>>>>>
>>>>>>>>   range<Elf_Rela_Iter>
>>>>>>>> rels)
>>>>>>>> {
>>>>>>>>        bool isMips64EL = _objFile->isMips64EL();
>>>>>>>> -  const auto symValue = getSymbolValue(&symbol);
>>>>>>>> +  const auto symValue = getSymbolValue(symbol);
>>>>>>>>        for (const auto &rel : rels) {
>>>>>>>>          if (rel.r_offset < symValue ||
>>>>>>>>              symValue + content.size() <= rel.r_offset)
>>>>>>>>            continue;
>>>>>>>>          _references.push_back(new (_readerStorage)
>>>>>>>> ELFReference<ELFT>(
>>>>>>>> -        &rel, rel.r_offset - symValue, kindArch(),
>>>>>>>> +        symbol, &rel, rel.r_offset - symValue, kindArch(),
>>>>>>>>              rel.getType(isMips64EL), rel.getSymbol(isMips64EL)));
>>>>>>>>        }
>>>>>>>>      }
>>>>>>>>
>>>>>>>>      template <class ELFT>
>>>>>>>> -void ELFFile<ELFT>::createRelocationReferences(const Elf_Sym
>>>>>>>> &symbol,
>>>>>>>> +void ELFFile<ELFT>::createRelocationReferences(const Elf_Sym
>>>>>>>> *symbol,
>>>>>>>>                                                     ArrayRef<uint8_t>
>>>>>>>> symContent,
>>>>>>>>                                                     ArrayRef<uint8_t>
>>>>>>>> secContent,
>>>>>>>>
>>>>>>>>   range<Elf_Rel_Iter>
>>>>>>>> rels) {
>>>>>>>>        bool isMips64EL = _objFile->isMips64EL();
>>>>>>>> -  const auto symValue = getSymbolValue(&symbol);
>>>>>>>> +  const auto symValue = getSymbolValue(symbol);
>>>>>>>>        for (const auto &rel : rels) {
>>>>>>>>          if (rel.r_offset < symValue ||
>>>>>>>>              symValue + symContent.size() <= rel.r_offset)
>>>>>>>>            continue;
>>>>>>>>          _references.push_back(new (_readerStorage)
>>>>>>>> ELFReference<ELFT>(
>>>>>>>> -        rel.r_offset - symValue, kindArch(),
>>>>>>>> -        rel.getType(isMips64EL), rel.getSymbol(isMips64EL)));
>>>>>>>> +        symbol, rel.r_offset - symValue, kindArch(),
>>>>>>>> rel.getType(isMips64EL),
>>>>>>>> +        rel.getSymbol(isMips64EL)));
>>>>>>>>          int32_t addend = *(symContent.data() + rel.r_offset -
>>>>>>>> symValue);
>>>>>>>>          _references.back()->setAddend(addend);
>>>>>>>>        }
>>>>>>>>
>>>>>>>> Modified: lld/trunk/lib/ReaderWriter/ELF/Mips/MipsELFFile.h
>>>>>>>> URL:
>>>>>>>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/
>>>>>>>> ReaderWriter/ELF/Mips/
>>>>>>>> MipsELFFile.h?rev=230191&r1=230190&r2=230191&view=diff
>>>>>>>>
>>>>>>>> ============================================================
>>>>>>>> ==================
>>>>>>>> --- lld/trunk/lib/ReaderWriter/ELF/Mips/MipsELFFile.h (original)
>>>>>>>> +++ lld/trunk/lib/ReaderWriter/ELF/Mips/MipsELFFile.h Sun Feb 22
>>>>>>>> 17:46:21
>>>>>>>> 2015
>>>>>>>> @@ -199,17 +199,17 @@ private:
>>>>>>>>          return std::error_code();
>>>>>>>>        }
>>>>>>>>
>>>>>>>> -  void createRelocationReferences(const Elf_Sym &symbol,
>>>>>>>> +  void createRelocationReferences(const Elf_Sym *symbol,
>>>>>>>>                                        ArrayRef<uint8_t> symContent,
>>>>>>>>                                        ArrayRef<uint8_t> secContent,
>>>>>>>>                                        range<Elf_Rel_Iter> rels)
>>>>>>>> override {
>>>>>>>>          for (Elf_Rel_Iter rit = rels.begin(), eit = rels.end(); rit
>>>>>>>> !=
>>>>>>>> eit;
>>>>>>>> ++rit) {
>>>>>>>> -      if (rit->r_offset < symbol.st_value ||
>>>>>>>> -          symbol.st_value + symContent.size() <= rit->r_offset)
>>>>>>>> +      if (rit->r_offset < symbol->st_value ||
>>>>>>>> +          symbol->st_value + symContent.size() <= rit->r_offset)
>>>>>>>>              continue;
>>>>>>>>
>>>>>>>>            this->_references.push_back(new (this->_readerStorage)
>>>>>>>> ELFReference<ELFT>(
>>>>>>>> -          rit->r_offset - symbol.st_value, this->kindArch(),
>>>>>>>> +          symbol, rit->r_offset - symbol->st_value,
>>>>>>>> this->kindArch(),
>>>>>>>>                rit->getType(isMips64EL()),
>>>>>>>> rit->getSymbol(isMips64EL())))
>>>>>>>> ;
>>>>>>>>
>>>>>>>>            auto addend = getAddend(*rit, secContent);
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> llvm-commits mailing list
>>>>>>>> llvm-commits at cs.uiuc.edu
>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>>>>
>>>>>>>>
>>>>>>>>    --
>>>>>>>>
>>>>>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>>>>>> hosted
>>>>>> by the Linux Foundation
>>>>>>
>>>>>>
>>>>>>
>>>>>>   --
>>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted
>>>> by the Linux Foundation
>>>>
>>>>
>>>>
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted
>> by the Linux Foundation
>>
>>


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation




More information about the llvm-commits mailing list