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

Rui Ueyama ruiu at google.com
Mon Feb 23 11:32:35 PST 2015


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


More information about the llvm-commits mailing list