[PATCH] Linking of shared libraries for MIPS little-endian 32-bit target
Shankar Kalpathi Easwaran
shankarke at gmail.com
Tue Nov 12 17:17:44 PST 2013
================
Comment at: lib/ReaderWriter/ELF/DefaultTargetHandler.h:62-78
@@ -61,1 +61,19 @@
+
+ /// \brief create dynamic table
+ LLD_UNIQUE_BUMP_PTR(DynamicTable<ELFT>)
+ createDynamicTable(llvm::BumpPtrAllocator &alloc,
+ StringRef name, int32_t order) {
+ return LLD_UNIQUE_BUMP_PTR(DynamicTable<ELFT>)(
+ new (alloc) DynamicTable<ELFT>(TargetHandler<ELFT>::_context,
+ name, order));
+ }
+
+ /// \brief create dynamic symbol table
+ LLD_UNIQUE_BUMP_PTR(DynamicSymbolTable<ELFT>)
+ createDynamicSymbolTable(llvm::BumpPtrAllocator &alloc,
+ StringRef name, int32_t order) {
+ return LLD_UNIQUE_BUMP_PTR(DynamicSymbolTable<ELFT>)(
+ new (alloc) DynamicSymbolTable<ELFT>(TargetHandler<ELFT>::_context,
+ name, order));
+ }
};
----------------
remove all parameters to this function. alloc should be owned by the TargetHandler for Mips. name/order could be completely decided by Mips too.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsELFTypes.h:1-20
@@ +1,21 @@
+//===- lld/ReaderWriter/ELF/Mips/MipsELFTypes.h ---------------------------===//
+//
+// The LLVM Linker
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+#ifndef LLD_READER_WRITER_ELF_MIPS_ELF_TYPES_H
+#define LLD_READER_WRITER_ELF_MIPS_ELF_TYPES_H
+
+#include "llvm/Object/ELFTypes.h"
+
+namespace lld {
+namespace elf {
+
+typedef llvm::object::ELFType<llvm::support::little, 2, false> MipsELFType;
+
+} // elf
+} // lld
+
----------------
why does this need a separate header file ? This would go in MipsLinkingContext
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsLinkingContext.cpp:60
@@ +59,3 @@
+ new MipsTargetHandler(*this))),
+ _got(*this)
+{}
----------------
not the right place for holding to the got.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsTargetHandler.cpp:173
@@ +172,3 @@
+ _gpDispSymAtom->_virtualAddr =
+ gotSection ? gotSection->virtualAddr() + 0x7FF0 : 0;
+ }
----------------
Why are you adding 0x7FF0, this might be difficult to support when we have linker script support.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsTargetHandler.cpp:171
@@ +170,3 @@
+ auto gpDispAtomIter = _targetLayout.findAbsoluteAtom("_gp_disp");
+ _gpDispSymAtom = (*gpDispAtomIter);
+ _gpDispSymAtom->_virtualAddr =
----------------
assert if these atoms are not found.
================
Comment at: lib/ReaderWriter/ELF/SectionChunks.h:1022-1024
@@ +1021,5 @@
+ /// \returns an entry by the index
+ Elf_Dyn &getEntry(std::size_t entry) {
+ return _entries[entry];
+ }
+
----------------
an operator is better.
================
Comment at: lib/ReaderWriter/ELF/TargetHandler.h:130
@@ +129,3 @@
+ virtual LLD_UNIQUE_BUMP_PTR(DynamicSymbolTable<ELFT>)
+ createDynamicSymbolTable(llvm::BumpPtrAllocator &alloc,
+ StringRef name, int32_t order) = 0;
----------------
Michael Spencer wrote:
> No indentation.
can we remove alloc and order here.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsRelocationHandler.cpp:44
@@ +43,3 @@
+ int32_t AHL = A;
+
+ int32_t result = 0;
----------------
nit pick. empty lines
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsTargetHandler.cpp:28-29
@@ +27,4 @@
+ virtual void sortSymbols() {
+ std::stable_sort(_symbolTable.begin(), _symbolTable.end(),
+ [this](const SymbolEntry & A, const SymbolEntry & B) {
+ if (A._symbol.getBinding() != STB_GLOBAL &&
----------------
I thought you want to sort all the atoms in the symbolTable, why stable_sort ?
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsTargetHandler.cpp:27
@@ +26,3 @@
+private:
+ virtual void sortSymbols() {
+ std::stable_sort(_symbolTable.begin(), _symbolTable.end(),
----------------
Didnt find a call to this function.
================
Comment at: lib/ReaderWriter/ELF/SectionChunks.h:626
@@ -624,3 +625,3 @@
public:
- SymbolTable(const ELFLinkingContext &context, const char *str, int32_t order);
+ SymbolTable(const ELFLinkingContext &context, StringRef str, int32_t order);
----------------
unrelated change ?
================
Comment at: lib/ReaderWriter/ELF/SectionChunks.h:610
@@ -609,2 +609,3 @@
class SymbolTable : public Section<ELFT> {
+protected:
typedef typename llvm::object::ELFDataTypeTypedefHelper<ELFT>::Elf_Addr
----------------
I dont know what does the llvm guideline say with classes having two sets of protected fields, to reduce confusion, you might want to move to the same place where other protected members are.
================
Comment at: lib/ReaderWriter/ELF/SectionChunks.h:993
@@ -986,2 +992,3 @@
template <class ELFT> class DynamicTable : public Section<ELFT> {
+protected:
typedef llvm::object::Elf_Dyn_Impl<ELFT> Elf_Dyn;
----------------
same as previous.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsLinkingContext.cpp:71-73
@@ +70,5 @@
+
+bool MipsLinkingContext::compareGOT(const Atom *a, const Atom *b) const {
+ return _got.compare(a, b);
+}
+
----------------
move compareGOT outside the LinkingContext. The LinkingContext should just have just flags that determine what and how to link.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsRelocationHandler.h:42-46
@@ +41,7 @@
+
+private:
+ virtual error_code applyRelocation(ELFWriter &,
+ llvm::FileOutputBuffer &,
+ const lld::AtomLayout &,
+ const Reference &) const;
+};
----------------
merge this private declaration with above. seen similiar declarations in other places too (protected ...)
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsRelocationHandler.cpp:27-29
@@ +26,5 @@
+
+// V - verify
+// T - truncate
+// AHL is (AHI << 16) + (short)ALO
+
----------------
not sure what this comment is addressing.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsGOT.cpp:90
@@ +89,3 @@
+std::size_t MipsGOT::getLocalCount() const {
+ return 2 + _localGotVector.size();
+}
----------------
Why 2 + ?
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsLinkingContext.h:23-31
@@ +22,11 @@
+
+ /// \brief Number of local GOT entries.
+ std::size_t getLocalGOTCount() const;
+
+ /// \brief Number of global GOT entries.
+ std::size_t getGlobalGOTCount() const;
+
+ /// \brief Compare two atoms accordingly theirs positions in the GOT.
+ bool compareGOT(const Atom *a, const Atom *b) const;
+
+ // ELFLinkingContext
----------------
I dont like the interface that LinkingContext is having a lot of helper function. The LinkingContext contains only what and how the linking is done(mainly flags) and very few generic helper functions. This interface sounds like its not dealing with LinkingContext in that way.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsRelocationHandler.cpp:83-84
@@ +82,4 @@
+
+/// \brief R_MIPS_CALL16
+/// external: V-rel16 G
+void relocCall16(uint8_t *location, uint64_t P, uint64_t S, int64_t A,
----------------
the comments are describing parameters that are different in names.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsRelocationHandler.cpp:72-74
@@ +71,5 @@
+
+/// \brief R_MIPS_GOT16
+/// local : V-rel16 G (calculate AHL and put high 16 bit to GOT)
+/// external: V-rel16 G
+void relocGOT16(uint8_t *location, uint64_t P, uint64_t S, int64_t A,
----------------
the comments are describing parameters that are different in names. AHL is not being calculated here too.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsRelocationHandler.cpp:31-32
@@ +30,4 @@
+
+/// \brief R_MIPS_32
+/// local/external: T-word32 S + A
+void reloc32(uint8_t *location, uint64_t P, uint64_t S, int64_t A) {
----------------
the comments are describing parameters that are different in names.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsRelocationHandler.cpp:39-40
@@ +38,4 @@
+/// \brief R_MIPS_HI16
+/// local/external: T-hi16 ((AHL + S) - (short)(AHL + S)) >> 16
+/// _gp_disp : V-hi16 ((AHL + GP - P) - (short)(AHL + GP - P)) >> 16
+void relocHi16(uint8_t *location, uint64_t P, uint64_t S, int64_t A,
----------------
the comments are describing parameters that are different in names.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsLinkingContext.cpp:120-121
@@ +119,4 @@
+void MipsLinkingContext::addPasses(PassManager &pm) {
+ if (isDynamic())
+ pm.add(std::unique_ptr<Pass>(new MipsGOTPass(_got)));
+
----------------
add else case and llvm_unreachable.
http://llvm-reviews.chandlerc.com/D2156
More information about the llvm-commits
mailing list