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

Shankar Easwaran shankare at codeaurora.org
Mon Feb 23 11:31:08 PST 2015


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




More information about the llvm-commits mailing list