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

Rui Ueyama ruiu at google.com
Mon Feb 23 12:03:31 PST 2015


I'm not asking because it'd affect the core. I'm asking because I'm working
on ELF and need to keep an eye on ELF.

On Mon, Feb 23, 2015 at 11:38 AM, Shankar Easwaran <shankare at codeaurora.org>
wrote:

> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150223/edf9cf08/attachment.html>


More information about the llvm-commits mailing list