[PATCH] Linking of shared libraries for MIPS little-endian 32-bit target
Simon Atanasyan
simon at atanasyan.com
Sun Nov 17 13:43:26 PST 2013
Thanks for review. I put my answers and questions inline.
================
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));
+ }
};
----------------
Shankar Kalpathi Easwaran wrote:
> remove all parameters to this function. alloc should be owned by the TargetHandler for Mips. name/order could be completely decided by Mips too.
Just want to be sure that I understand you properly. Do you suggest to delete default implementations of `createDynamicTable()` and `createDynamicSymbolTable()` from the `DefaultTargetHandler` and define these functions as well as allocator in each target handlers for each supported targets (x86, x86_64, hexagon, mips etc)?
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsELFTypes.h:17
@@ +16,3 @@
+
+typedef llvm::object::ELFType<llvm::support::little, 2, false> MipsELFType;
+
----------------
Rui Ueyama wrote:
> Is this the only architecture you're going to support? Should we name Mips32ELFType?
Agree. I will rename it to the `Mips32ElELFType`.
================
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
+
----------------
Shankar Kalpathi Easwaran wrote:
> why does this need a separate header file ? This would go in MipsLinkingContext
OK
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsGOT.cpp:69
@@ +68,3 @@
+ const DefinedAtom *da = dyn_cast<DefinedAtom>(a);
+ auto isLocal = da && da->scope() == Atom::scopeTranslationUnit;
+
----------------
Rui Ueyama wrote:
> s/auto/bool/
OK
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsGOT.cpp:79
@@ +78,3 @@
+ }
+#ifndef NDEBUG
+ ga->_name = "__got_";
----------------
Rui Ueyama wrote:
> I'd write like this instead of #ifndef
>
> DEBUG_WITH_TYPE("MipsGOT", {
> ga->name = ...
> ...
> llvm::dbgs() << ...
> });
>
> Note that it's probably better to use MipsGOT or something instead of GOT.
OK
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsGOT.cpp:90
@@ +89,3 @@
+std::size_t MipsGOT::getLocalCount() const {
+ return 2 + _localGotVector.size();
+}
----------------
Shankar Kalpathi Easwaran wrote:
> Why 2 + ?
The GOT contains two reserved entries: lazy resolver and module pointer. But I agree that hardcoded constant is not a good solution. I will handle all GOT entries in a uniform way and remove this constant.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsGOT.cpp:100
@@ +99,3 @@
+
+#ifndef NDEBUG
+ DEBUG_WITH_TYPE("GOT", llvm::dbgs() << "[ GOT ] Adding GOT header\n");
----------------
Rui Ueyama wrote:
> No need to use #ifndef. DEBUG_WITH_TYPE will be expanded to do{}while(0) if NDEBUG is not defined.
OK
================
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)));
+
----------------
Shankar Kalpathi Easwaran wrote:
> add else case and llvm_unreachable.
OK
================
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;
+};
----------------
Shankar Kalpathi Easwaran wrote:
> merge this private declaration with above. seen similiar declarations in other places too (protected ...)
OK
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsRelocationHandler.cpp:25
@@ +24,3 @@
+ result | \
+ (uint32_t) * reinterpret_cast<llvm::support::ulittle32_t *>(location);
+
----------------
Rui Ueyama wrote:
> Why macro? Can it be an inline function?
I will replace this macros by inline function.
================
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) {
----------------
Shankar Kalpathi Easwaran wrote:
> the comments are describing parameters that are different in names.
The comment mentions `A` and `S`, parameter's names are `A` and `S`. Where is the difference?
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsRelocationHandler.cpp:27-29
@@ +26,5 @@
+
+// V - verify
+// T - truncate
+// AHL is (AHI << 16) + (short)ALO
+
----------------
Shankar Kalpathi Easwaran wrote:
> not sure what this comment is addressing.
OK. I will reformat comments for relocation handling function.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsRelocationHandler.cpp:33
@@ +32,3 @@
+/// local/external: T-word32 S + A
+void reloc32(uint8_t *location, uint64_t P, uint64_t S, int64_t A) {
+ uint32_t result = (uint32_t)(S + A);
----------------
Rui Ueyama wrote:
> In LLD local variables usually start with a lowercase letter.
It looks like this rule is violated in the `X86_64RelocationHandler.cpp` and `HexagonRelocationHandler.cpp`. I know the reason - `P`, `S`, `A` names are used in the corresponding ABI specification. It simplify reading and understanding the code if we keep these names unchanged.
================
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,
----------------
Shankar Kalpathi Easwaran wrote:
> the comments are describing parameters that are different in names.
OK. I will fix that,
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsRelocationHandler.cpp:44
@@ +43,3 @@
+ int32_t AHL = A;
+
+ int32_t result = 0;
----------------
Shankar Kalpathi Easwaran wrote:
> nit pick. empty lines
OK
================
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,
----------------
Shankar Kalpathi Easwaran wrote:
> the comments are describing parameters that are different in names. AHL is not being calculated here too.
OK. I will fix that.
================
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,
----------------
Shankar Kalpathi Easwaran wrote:
> the comments are describing parameters that are different in names.
OK. I will fix that.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsRelocationHandler.cpp:188
@@ +187,3 @@
+ s.flush();
+ llvm_unreachable(str.c_str());
+ }
----------------
Rui Ueyama wrote:
> nit: s.str() would flush the stream and then return the result string, so you can remove the line above.
OK. I will fix that.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsSectionChunks.h:27
@@ +26,3 @@
+ this->_flags |= SHF_MIPS_GPREL;
+ this->_align2 = 16;
+ }
----------------
Rui Ueyama wrote:
> 2^16 is too large. Typo of 4?
OK. I will fix that.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsTarget.h:10
@@ +9,2 @@
+
+#include "MipsLinkingContext.h"
----------------
Rui Ueyama wrote:
> Do you really need this file?
The `lib/ReaderWriter/ELF/Targets.h` includes `xxxTarget.h` files for each supported target. All these files look similar and include only `xxxLinkingContext.h`. I keep to the established order and do the same thing for the MIPS target.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsTargetHandler.h:27
@@ +26,3 @@
+public:
+ MipsTargetLayout(const MipsLinkingContext &hti)
+ : TargetLayout<MipsELFType>(hti),
----------------
Rui Ueyama wrote:
> "ti" standed for "target info". Because LinkingContext used to be named TargetInfo, some local variables still contain "ti". It no longer makes sense. I'd think "ctx" is preferred.
OK. I will fix that.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsTargetHandler.cpp:27
@@ +26,3 @@
+private:
+ virtual void sortSymbols() {
+ std::stable_sort(_symbolTable.begin(), _symbolTable.end(),
----------------
Shankar Kalpathi Easwaran wrote:
> Didnt find a call to this function.
`OutputELFWriter<ELFT>::buildDynamicSymbolTable()`
and
`SymbolTable<ELFT>::finalize()`
================
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 &&
----------------
Shankar Kalpathi Easwaran wrote:
> I thought you want to sort all the atoms in the symbolTable, why stable_sort ?
I want to sort all atoms in the `symbolTable` which have corresponding entries in the global part of GOT. These atoms are subset of the `symbolTable`. So I do not want to change an order of other atoms.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsTargetHandler.cpp:171
@@ +170,3 @@
+ auto gpDispAtomIter = _targetLayout.findAbsoluteAtom("_gp_disp");
+ _gpDispSymAtom = (*gpDispAtomIter);
+ _gpDispSymAtom->_virtualAddr =
----------------
Shankar Kalpathi Easwaran wrote:
> assert if these atoms are not found.
OK
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsTargetHandler.cpp:173
@@ +172,3 @@
+ _gpDispSymAtom->_virtualAddr =
+ gotSection ? gotSection->virtualAddr() + 0x7FF0 : 0;
+ }
----------------
Shankar Kalpathi Easwaran wrote:
> Why are you adding 0x7FF0, this might be difficult to support when we have linker script support.
OK. I will move this constant to the separate function.
================
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
----------------
Shankar Kalpathi Easwaran wrote:
> 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.
OK. I will fix that.
================
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);
----------------
Shankar Kalpathi Easwaran wrote:
> unrelated change ?
Yes. I will fix that.
================
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;
----------------
Shankar Kalpathi Easwaran wrote:
> same as previous.
OK. I will fix that.
================
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];
+ }
+
----------------
Shankar Kalpathi Easwaran wrote:
> an operator is better.
This time only `MipsDynamicTable` which is successor of `DynamicTable` needs access to the `_entries`. I will remove the `DynamicTable::getEntry()` method and move the `DynamicTable::_entries` field to the protected section.
================
Comment at: lib/ReaderWriter/ELF/TargetHandler.h:125
@@ +124,3 @@
+ virtual LLD_UNIQUE_BUMP_PTR(DynamicTable<ELFT>)
+ createDynamicTable(llvm::BumpPtrAllocator &alloc,
+ StringRef name, int32_t order) = 0;
----------------
Michael Spencer wrote:
> No indentation.
OK. I will fix that.
================
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;
----------------
Shankar Kalpathi Easwaran wrote:
> Michael Spencer wrote:
> > No indentation.
> can we remove alloc and order here.
OK. I will fix that.
================
Comment at: lib/ReaderWriter/ELF/X86/X86TargetHandler.h:18
@@ -17,3 +17,3 @@
namespace elf {
-typedef llvm::object::ELFType<llvm::support::little, 2, false> X86ELFType;
+typedef llvm::object::ELFType<llvm::support::little, 4, false> X86ELFType;
class X86LinkingContext;
----------------
Michael Spencer wrote:
> This is an unrelated (and incorrect) change.
OK. I will fix that.
================
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
----------------
Shankar Kalpathi Easwaran wrote:
> 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.
OK. I will fix that.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsLinkingContext.h:39
@@ +38,3 @@
+private:
+ MipsGOT _got;
+};
----------------
Michael Spencer wrote:
> This isn't a great place for this as it preserves atom pointers from a specific pass, which are not guaranteed to be stable. It seems that it's only used for the local and global offsets of the got. I think a better option here would be to calculate the offsets in the layout by iterating over the final .got MergedSection.
OK. I will fix that.
================
Comment at: lib/ReaderWriter/ELF/Mips/MipsLinkingContext.cpp:60
@@ +59,3 @@
+ new MipsTargetHandler(*this))),
+ _got(*this)
+{}
----------------
Shankar Kalpathi Easwaran wrote:
> not the right place for holding to the got.
OK. I will fix that.
================
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);
+}
+
----------------
Shankar Kalpathi Easwaran wrote:
> move compareGOT outside the LinkingContext. The LinkingContext should just have just flags that determine what and how to link.
OK. I will fix that.
http://llvm-reviews.chandlerc.com/D2156
More information about the llvm-commits
mailing list