[llvm-commits] [lld] add support for followon references
Michael Spencer
bigcheesegs at gmail.com
Thu Nov 1 13:43:07 PDT 2012
On Thu, Nov 1, 2012 at 7:24 AM, Shankar Easwaran
<shankare at codeaurora.org> wrote:
> Hi,
>
> Can you please review the patch to add support for followon references in
> lld. The patch includes a test case for the changes done in ReaderELF.
>
> Thanks
>
> Shankar Easwaran
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
> the Linux Foundation
>
>
>
>
>
> Index: test/elf/Inputs/followon-reference-test.elf-i386
> ===================================================================
> Cannot display: file marked as a binary type.
> svn:mime-type = application/octet-stream
>
> Property changes on: test/elf/Inputs/followon-reference-test.elf-i386
> ___________________________________________________________________
> Added: svn:mime-type
> + application/octet-stream
>
> Index: test/elf/followon-reference.objtxt
> ===================================================================
> --- test/elf/followon-reference.objtxt (revision 0)
> +++ test/elf/followon-reference.objtxt (revision 0)
> @@ -0,0 +1,54 @@
> +# Test generated by using the following
> +# .file "1.c"
> +# .text
> +# .globl fn
> +# .type fn, @function
> +#fn:
> +#.LFB0:
> +# .cfi_startproc
> +# pushq %rbp
> +# .cfi_def_cfa_offset 16
> +# .cfi_offset 6, -16
> +# movq %rsp, %rbp
> +# .cfi_def_cfa_register 6
> +# movl $0, %eax
> +# popq %rbp
> +# .cfi_def_cfa 7, 8
> +# ret
> +# .cfi_endproc
> +#.LFE0:
> +# .size fn, .-fn
> +# .align 32
> +# .globl fn1
> +# .type fn1, @function
> +#fn1:
> +#.LFB1:
> +# .cfi_startproc
> +# pushq %rbp
> +# .cfi_def_cfa_offset 16
> +# .cfi_offset 6, -16
> +# movq %rsp, %rbp
> +# .cfi_def_cfa_register 6
> +# movl $0, %eax
> +# popq %rbp
> +# .cfi_def_cfa 7, 8
> +# ret
> +# .cfi_endproc
> +#.LFE1:
> +# .size fn1, .-fn1
> +# .section .note.GNU-stack,"", at progbits
> +
> +RUN: lld-core -reader ELF %p/Inputs/followon-reference-test.elf-i386 | FileCheck -check-prefix=YAML %s
> +
> +YAML: - name: fn
> +YAML: scope: global
> +YAML: type: code
> +YAML: section-choice: custom-required
> +YAML: section-name: .text
> +YAML: content: [ 55, 48, 89, E5, B8, 00, 00, 00, 00, 5D, C3, 66,
> +YAML: 0F, 1F, 44, 00, 00, 66, 66, 66, 66, 66, 66, 2E,
> +YAML: 0F, 1F, 84, 00, 00, 00, 00, 00 ]
> +YAML: fixups:
> +YAML: - offset: 0
> +YAML: kind: follow-on
> +YAML: target: fn1
> Index: tools/lld-core/TestingHelpers.hpp
> ===================================================================
> --- tools/lld-core/TestingHelpers.hpp (revision 167130)
> +++ tools/lld-core/TestingHelpers.hpp (working copy)
> @@ -271,6 +271,7 @@
> // Table of fixup kinds in YAML documents used for testing
> //
> const TestingKindMapping sKinds[] = {
> + { "follow-on", -1, true, false, false},
> { "call32", 2, true, false, false},
> { "pcrel32", 3, false, false, false },
> { "gotLoad32", 7, false, true, true },
> Index: include/lld/Core/Reference.h
> ===================================================================
> --- include/lld/Core/Reference.h (revision 167130)
> +++ include/lld/Core/Reference.h (working copy)
> @@ -36,6 +36,10 @@
> // A value to be added to the value of a target
> typedef int64_t Addend;
>
> + enum {
> + kindFollowOnReference = -1,
> + };
> +
This should be (along with changing the typedef above):
enum class Kind : int32_t {
FollowOn = -1
};
> /// What sort of reference this is.
> virtual Kind kind() const = 0;
>
> @@ -72,7 +76,44 @@
> virtual ~Reference() {}
> };
>
> +/// FollowOnReference supports forcing layout of atoms, that one atom must
> +/// follow another atom.
Doxygen comments should follow:
http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
> +class FollowOnReference : public Reference {
> +public:
> + /// Return the reference is of type FollowOnReference
> + virtual Kind kind() const { return _kind; }
This can never be any other Kind. Get rid of _kind, setKind, and
return a constant.
>
> + virtual void setKind(Kind) { _kind = kindFollowOnReference; }
> +
> + virtual uint64_t offsetInAtom() const { return 0; }
> +
> + /// If the reference is an edge to another Atom, then this returns the
> + /// other Atom. Otherwise, it returns nullptr.
> + virtual const class Atom * target() const { return _targetAtom; }
Drop class and there should be no space after the *.
> +
> + /// Set the followon Atom
> + virtual void setTarget(const class Atom *atom) { _targetAtom = atom; }
Drop class.
> +
> + /// Some relocations require a symbol and a value (e.g. foo + 4).
> + virtual Addend addend() const { return 0; }
> +
> + /// During linking, some optimzations may change addend value.
> + virtual void setAddend(Addend) { }
> +
> +public:
Already public.
> + FollowOnReference():Reference(),
> + _targetAtom(NULL),
> + _kind(kindFollowOnReference)
> + {}
This should be:
FollowOnReference()
: Reference()
, _targetAtom(nullptr)
, _kind(kindFollowOnReference)
{}
> +
> + virtual ~FollowOnReference() {}
http://llvm.org/docs/CodingStandards.html#provide-a-virtual-method-anchor-for-classes-in-headers
> +
> +private:
> + const Atom *_targetAtom;
> + Kind _kind;
> +};
> +
> +
Only one new line between things.
> } // namespace lld
>
> #endif // LLD_CORE_REFERENCES_H_
> Index: lib/ReaderWriter/ELF/ReaderELF.cpp
> ===================================================================
> --- lib/ReaderWriter/ELF/ReaderELF.cpp (revision 167130)
> +++ lib/ReaderWriter/ELF/ReaderELF.cpp (working copy)
> @@ -84,6 +84,10 @@
> return _target;
> }
>
> + static bool classof(const Reference *S) {
> + return true;
> + }
> +
> /// \brief targetSymbolIndex: This is the symbol table index that contains
> /// the target reference.
> uint64_t targetSymbolIndex() const {
> @@ -220,8 +224,7 @@
> llvm::ArrayRef<uint8_t> contentData,
> unsigned int referenceStart,
> unsigned int referenceEnd,
> - std::vector<ELFReference
> - <target_endianness, is64Bits> *> &referenceList)
> + std::vector<Reference *> &referenceList)
>
> : _owningFile(file)
> , _symbolName(symbolName)
> @@ -403,6 +406,12 @@
> return _contentData;
> }
>
> + void updateReferences(std::vector<Reference *> &referenceList)
> + {
No newline before {.
> + _referenceList = referenceList;
> + _referenceEndIndex = referenceList.size();
> + }
> +
> DefinedAtom::reference_iterator begin() const {
> uintptr_t index = _referenceStartIndex;
> const void *it = reinterpret_cast<const void*>(index);
> @@ -441,7 +450,7 @@
> uint64_t _ordinal;
> unsigned int _referenceStartIndex;
> unsigned int _referenceEndIndex;
> - std::vector<ELFReference<target_endianness, is64Bits> *> &_referenceList;
> + std::vector<Reference *> &_referenceList;
> };
>
>
> @@ -606,6 +615,9 @@
> return A->st_value < B->st_value;
> }));
>
> + ELFDefinedAtom<target_endianness, is64Bits> *previous_atom = NULL;
> + const Elf_Sym *previous_sym = NULL;
Use nullptr.
> +
> // i.first is the section the symbol lives in
> for (auto si = symbols.begin(), se = symbols.end(); si != se; ++si) {
>
> @@ -640,7 +652,24 @@
> symbolData = llvm::ArrayRef<uint8_t>((uint8_t *)symbolContents.data()
> + (*si)->st_value, contentSize);
>
> + Reference *followOn = NULL;
nullptr.
>
> + // Check to see if we need to add the FollowOn Reference
> + // We add a follow on reference only if current_symbol_value -
> + // previous_symbol_value + previous_symbol_size > 0
> + if (!isCommon && previous_sym) {
> + if (((*si)->st_value - (previous_sym->st_value +
> + previous_sym->st_size)) > 0) {
> +
No newline here.
> + followOn = new (_readerStorage.Allocate<FollowOnReference>())
> + FollowOnReference();
> +
> + _references.push_back(followOn);
> +
> + previous_atom->updateReferences(_references);
> + }
> + }
> +
> unsigned int referenceStart = _references.size();
>
> // Only relocations that are inside the domain of the atom are
> @@ -682,6 +711,17 @@
> *si, i.first, symbolData,
> referenceStart, _references.size(), _references);
>
> + // Check to see if we need to add the FollowOn Reference
> + // We add a follow on reference only if current_symbol_value -
> + // previous_symbol_value + previous_symbol_size > 0
This comment does not describe the code.
> + if (followOn) {
> + fprintf(stderr, "adding a followon reference\n");
We don't use this. Use http://llvm.org/docs/ProgrammersManual.html#DEBUG
> + followOn->setTarget(newAtom);
> + }
> +
> + previous_atom = newAtom;
> + previous_sym = *si;
> +
> _definedAtoms._atoms.push_back(newAtom);
> _symbolToAtomMapping.insert(std::make_pair((*si), newAtom));
>
> @@ -691,8 +731,11 @@
> // All the Atoms and References are created. Now update each Reference's
> // target with the Atom pointer it refers to.
> for (auto &ri : _references) {
> - const Elf_Sym *Symbol = _objFile->getElfSymbol(ri->targetSymbolIndex());
> - ri->setTarget(findAtom (Symbol));
> + if (ri->kind() != Reference::kindFollowOnReference) {
> + ELFReference<target_endianness, is64Bits> *eri = (llvm::dyn_cast<ELFReference<target_endianness,is64Bits>>(ri));
> + const Elf_Sym *Symbol = _objFile->getElfSymbol(eri->targetSymbolIndex());
Both of these lines are beyond 80 col.
> + ri->setTarget(findAtom (Symbol));
No space before (.
> + }
> }
> }
>
> @@ -740,7 +783,7 @@
> std::map<llvm::StringRef, std::vector<const Elf_Rel *> >
> _relocationReferences;
>
> - std::vector<ELFReference<target_endianness, is64Bits> *> _references;
> + std::vector<Reference *> _references;
> llvm::DenseMap<const Elf_Sym *, Atom *> _symbolToAtomMapping;
>
> llvm::BumpPtrAllocator _readerStorage;
>
I can't confirm the test because I don't have followon-reference-test.elf-i386
Can you send that too?
- Michael Spencer
More information about the llvm-commits
mailing list