[llvm-commits] [PATCH] [lld] patch for ELF Writer to get it to 'Hello world'
Michael Spencer
bigcheesegs at gmail.com
Mon Dec 17 15:53:01 PST 2012
The following tests don't pass for me with this change:
* branch.objtext
* phdr.objtxt
* reloc.objtxt
* rodata.objtxt
* sections.objtxt
These are my initial comments. There are a few things I need to look deeper into.
================
Comment at: include/lld/Core/DefinedAtom.h:151-153
@@ -149,4 +150,5 @@
permR__ = 8, // mapped read-only
- permR_X = 8 + 2, // mapped readable and executable
- permRW_ = 8 + 4, // mapped readable and writable
+ permRW_ = 8 + 2, // mapped readable and writable
+ permR_X = 8 + 4, // mapped readable and executable
+ permRWX = 8 + 4 + 2, // mapped readable and writable and executable
permRW_L = 8 + 4 + 1, // initially mapped r/w, then made read-only
----------------
I assume you reordered these so the code later can sort based on the value. Please add a comment here to that effect.
You also need to change premRW_L to match.
================
Comment at: lib/Core/SymbolTable.cpp:76
@@ -75,3 +75,3 @@
// first is absolute
- NCR_Error, NCR_Error, NCR_First, NCR_First
+ NCR_Error, NCR_First, NCR_First, NCR_First
},
----------------
I don't understand why this change is here.
================
Comment at: lib/ReaderWriter/ELF/ELFAtoms.h:1
@@ +1,2 @@
+#ifndef LLD_ELFATOMS_H_
+#define LLD_ELFATOMS_H_
----------------
Needs a proper file header.
Don't add a trailing _.
================
Comment at: lib/ReaderWriter/ELF/ELFAtoms.h:14
@@ +13,3 @@
+class ELFReference final : public Reference {
+
+ typedef llvm::object::Elf_Rel_Impl
----------------
No empty line.
================
Comment at: lib/ReaderWriter/ELF/ELFAtoms.h:83
@@ +82,3 @@
+class ELFAbsoluteAtom final : public AbsoluteAtom {
+
+ typedef llvm::object::Elf_Sym_Impl<target_endianness, is64Bits> Elf_Sym;
----------------
No empty line.
================
Comment at: lib/ReaderWriter/ELF/ELFAtoms.h:133
@@ +132,3 @@
+class ELFUndefinedAtom final: public UndefinedAtom {
+
+ typedef llvm::object::Elf_Sym_Impl<target_endianness, is64Bits> Elf_Sym;
----------------
No empty line.
================
Comment at: lib/ReaderWriter/ELF/ELFAtoms.h:132
@@ +131,3 @@
+template<llvm::support::endianness target_endianness, bool is64Bits>
+class ELFUndefinedAtom final: public UndefinedAtom {
+
----------------
Space between final and :.
================
Comment at: lib/ReaderWriter/ELF/ELFAtoms.h:159
@@ +158,3 @@
+ virtual CanBeNull canBeNull() const {
+
+ if (_symbol->getBinding() == llvm::ELF::STB_WEAK)
----------------
No empty line.
================
Comment at: lib/ReaderWriter/ELF/ELFAtoms.h:176
@@ +175,3 @@
+template<llvm::support::endianness target_endianness, bool is64Bits>
+class ELFDefinedAtom final: public DefinedAtom {
+
----------------
Space between final and :.
================
Comment at: lib/ReaderWriter/ELF/ELFAtoms.h:177
@@ +176,3 @@
+class ELFDefinedAtom final: public DefinedAtom {
+
+ typedef llvm::object::Elf_Sym_Impl<target_endianness, is64Bits> Elf_Sym;
----------------
No empty line after {.
================
Comment at: lib/ReaderWriter/ELF/ELFAtoms.h:188-189
@@ +187,4 @@
+ llvm::ArrayRef<uint8_t> contentData,
+ unsigned int referenceStart,
+ unsigned int referenceEnd,
+ std::vector<ELFReference
----------------
The meaning of these two needs to be documented. I assume they are indices into referenceList.
================
Comment at: lib/ReaderWriter/ELF/ELFAtoms.h:202-203
@@ +201,4 @@
+ , _referenceList(referenceList) {
+ static uint64_t orderNumber = 0;
+ _ordinal = ++orderNumber;
+ }
----------------
This is not thread safe.
================
Comment at: lib/ReaderWriter/ELF/ELFAtoms.h:210
@@ +209,3 @@
+
+ virtual llvm::StringRef name() const {
+ return _symbolName;
----------------
No need to qualify StringRef.
================
Comment at: lib/ReaderWriter/ELF/ELFAtoms.h:219
@@ +218,3 @@
+ virtual uint64_t size() const {
+
+ // Common symbols are not allocated in object files,
----------------
No empty line after {.
================
Comment at: lib/ReaderWriter/ELF/ELFAtoms.h:422-424
@@ +421,5 @@
+
+//
+// Simple File
+//
+class SimpleFile : public File {
----------------
This should be a doxygen comment.
================
Comment at: lib/ReaderWriter/ELF/ELFAtoms.h:434-435
@@ +433,4 @@
+ _definedAtoms._atoms.push_back(defAtom);
+ }
+ else if (const UndefinedAtom* undefAtom = dyn_cast<UndefinedAtom>(&atom)) {
+ _undefinedAtoms._atoms.push_back(undefAtom);
----------------
No newline before else. Same bellow.
================
Comment at: lib/ReaderWriter/ELF/ELFAtoms.h:432
@@ +431,3 @@
+ virtual void addAtom(const Atom &atom) {
+ if (const DefinedAtom* defAtom = dyn_cast<DefinedAtom>(&atom)) {
+ _definedAtoms._atoms.push_back(defAtom);
----------------
%%%
* goes to the right.
%%%
Same for the rest.
================
Comment at: lib/ReaderWriter/ELF/ELFAtoms.h:450
@@ +449,3 @@
+
+ virtual const atom_collection<DefinedAtom>& defined() const {
+ return _definedAtoms;
----------------
& goes to the right. Same bellow.
================
Comment at: lib/ReaderWriter/ELF/ReaderELF.cpp:25
@@ -24,2 +24,3 @@
#include "llvm/Support/Casting.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/ELF.h"
----------------
Includes should be sorted in alphabetical order.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:25
@@ -24,2 +24,3 @@
#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringExtras.h"
----------------
Sort alphabetically.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:41
@@ -38,2 +40,3 @@
#include <map>
+#include <unordered_map>
#include <tuple>
----------------
Sort alphabetically.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:60-71
@@ -90,5 +59,14 @@
+ public:
+ /// \brief builds the chunks that needs to be written to the output
+ /// ELF file
+ virtual void buildChunks(const lld::File &file) = 0;
-template<support::endianness target_endianness, bool is64Bits>
-class StockSectionChunk;
+ /// \brief Writes the chunks into the output file specified by path
+ virtual error_code writeFile(const lld::File &File, StringRef path) = 0;
-/// \brief A Chunk is a contiguous range of space.
+ /// \brief Writes the chunks into the output file specified by path
+ virtual int64_t addressOfAtom(const Atom *atom) = 0;
+
+ /// \brief Return the processing function to apply Relocations
+ virtual KindHandler *kindHandler() = 0;
+};
----------------
I don't see a reason for any of these except writeFile to be virtual. They are never called virtually.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:80-86
@@ +79,9 @@
+ /// \brief Describes the type of Chunk
+ enum Kind {
+ ELFHeader, // ELF Header
+ ProgramHeader, // Program Header
+ Segment, // Segment
+ Section, // Section
+ SectionHeader // Section header
+ };
+ Chunk(StringRef name, Kind kind) : _name(name),
----------------
This either needs to be an enum class, or the members need to follow the naming convention. Currently this breaks MSVC because it doesn't do two phase name lookup and therefore sees Section and Segment in the derived class (Chunk) instead of the templates Section and Segment.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:87-95
@@ -99,1 +86,11 @@
+ };
+ Chunk(StringRef name, Kind kind) : _name(name),
+ _kind(kind),
+ _fsize(0),
+ _msize(0),
+ _align2(0),
+ _order(0),
+ _ordinal(1),
+ _start(0),
+ _fileoffset(0) {}
virtual ~Chunk() {}
----------------
Please match the style used elsewhere.
lang=c++
Chunk(StringRef name, Kind kind)
: _name(name)
, _kind(kind)
, _fsize(0)
, etc... {}
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:109-112
@@ +108,6 @@
+
+ // Align the chunk, this doesnot take modulus into account
+ static int64_t alignTo(int64_t value, int64_t align2) {
+ return ((value + align2 - 1) & ~(align2 - 1));
+ }
+ // The ordinal value of the chunk
----------------
This can be replaced by MathExtras.h RoundUpToAlignment.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:121-123
@@ +120,5 @@
+ int64_t fileOffset() const { return _fileoffset; }
+ void setFileOffset(int64_t offset) { _fileoffset = offset; }
+ // Output start address of the chunk
+ void setStart(int64_t start) { _start = start; }
+ int64_t startAddr() const { return _start; }
----------------
What is the difference between these two function? Is one the virtual address?
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:98
@@ +97,3 @@
+ // Does the chunk occupy disk space
+ virtual bool occupiesNoDiskSpace() {
+ return false;
----------------
This function must be marked const. The derived classes declare this as non-const, and therefore add a new overload instead of an implementation of this virtual function.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:129
@@ +128,3 @@
+ // Writer the chunk
+ virtual void write(ELFWriter *writer,
+ OwningPtr<FileOutputBuffer> &buffer) = 0;
----------------
No chuck seems to use the writer.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:133-135
@@ +132,5 @@
+ virtual void finalize() = 0;
+ static inline bool classof(Chunk<target_endianness, is64Bits> *s) {
+ return true;
+ }
+
----------------
This is no longer needed for llvm RTTI.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:149-153
@@ -135,10 +148,7 @@
-template<support::endianness target_endianness, bool is64Bits>
-static void swapChunkPositions(Chunk<target_endianness, is64Bits>&a,
- Chunk<target_endianness, is64Bits>&b) {
- uint64_t tempOrdinal;
- if (a.ordinal() == b.ordinal()) return;
- tempOrdinal = a.ordinal();
- a.setOrdinal(b.ordinal());
- b.setOrdinal(tempOrdinal);
-}
+///
+/// \brief The ELFLayoutOptions encapsulates the options used by all Layouts
+/// Examples of the ELFLayoutOptions would be a script that would be used
+/// to drive the layout
+///
+class ELFLayoutOptions
----------------
Don't add extra empty /// lines around comments.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:154-155
@@ +153,4 @@
+///
+class ELFLayoutOptions
+{
+public:
----------------
No newline before {.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:162
@@ -153,1 +161,3 @@
+ // parse the linker script
+ error_code parseLinkerScript();
----------------
Doxygen.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:165
@@ +164,3 @@
+
+ // Is the current section present in the linker script
+ bool isSectionPresent();
----------------
Doxygen.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:180-181
@@ +179,4 @@
+///
+class ELFLayout
+{
+public:
----------------
No newline before {.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:189-191
@@ +188,5 @@
+ // Return the order the section would appear in the output file
+ virtual SectionOrder getSectionOrder(const StringRef name,
+ int32_t contentType,
+ int32_t contentPerm) = 0;
+ // append the Atom to the layout and create
----------------
Line up args.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:188
@@ +187,3 @@
+public:
+ // Return the order the section would appear in the output file
+ virtual SectionOrder getSectionOrder(const StringRef name,
----------------
Doxygen.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:192-193
@@ +191,4 @@
+ int32_t contentPerm) = 0;
+ // append the Atom to the layout and create
+ // appropriate sections
+ virtual error_code addAtom(const Atom *atom) = 0;
----------------
Doxygen.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:195
@@ +194,3 @@
+ virtual error_code addAtom(const Atom *atom) = 0;
+ // find the Atom Address in the current layout
+ virtual bool findAtomAddrByName(const StringRef name, int64_t &addr) = 0;
----------------
Doxygen.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:197
@@ +196,3 @@
+ virtual bool findAtomAddrByName(const StringRef name, int64_t &addr) = 0;
+ // associates a section to a segment
+ virtual void assignSectionsToSegments() = 0;
----------------
Doxygen.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:199
@@ +198,3 @@
+ virtual void assignSectionsToSegments() = 0;
+ // associates a virtual address to the segment, section, and the atom
+ virtual void assignVirtualAddress() = 0;
----------------
Doxygen.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:201
@@ +200,3 @@
+ virtual void assignVirtualAddress() = 0;
+ // associates a file offset to the segment, section and the atom
+ virtual void assignFileOffset() = 0;
----------------
Doxygen.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:206-208
@@ +205,5 @@
+ ELFLayout() {}
+ ELFLayout(WriterOptionsELF &writerOptions,
+ ELFLayoutOptions &layoutOptions) : _writerOptions(writerOptions),
+ _layoutOptions(layoutOptions) { }
+ virtual ~ELFLayout() { }
----------------
Style.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:230-236
@@ +229,9 @@
+ // caller doesnot set it
+ Section(const StringRef sectionName,
+ const int32_t contentType,
+ const int32_t contentPermissions,
+ const int32_t order,
+ const Kind kind = Default) :
+ Chunk<target_endianness, is64Bits>(sectionName,
+ Chunk<target_endianness, is64Bits>::Section) {
+ _contentType = contentType;
----------------
Style.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:237-242
@@ +236,8 @@
+ Chunk<target_endianness, is64Bits>::Section) {
+ _contentType = contentType;
+ _contentPermissions = contentPermissions;
+ _sectionKind = kind;
+ _link = 0;
+ _entsize = 0;
+ _shinfo = 0;
+ this->setOrder(order);
----------------
These should be in the init list.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:257
@@ +256,3 @@
+ Atom::Definition atomType = atom->definition();
+ const DefinedAtom* definedAtom = dyn_cast<DefinedAtom>(atom);
+ assert(atom != nullptr && "Expecting the atom to be a DefinedAtom");
----------------
The * goes on the right.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:261-262
@@ -172,1 +260,4 @@
+ int64_t align2 = 1 << atomAlign.powerOf2;
+ uint64_t foffset = this->filesize();
+ uint64_t moffset = this->memsize();
----------------
fOffset and mOffset.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:105
@@ +104,3 @@
+ Kind kind() const { return _kind; }
+ int64_t filesize() const { return _fsize; }
+ int64_t align2() const { return _align2; }
----------------
fileSize.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:264-267
@@ -172,22 +263,6 @@
-/// \brief A SectionChunk represents ELF sections
-template<support::endianness target_endianness, bool is64Bits>
-class SectionChunk : public Chunk<target_endianness, is64Bits> {
-public:
- virtual StringRef segmentName() const { return _segmentName; }
- virtual bool occupiesNoDiskSpace();
- virtual const char *info();
- StringRef sectionName() { return _sectionName; }
- uint64_t shStrtableOffset(){ return _offsetInStringTable; }
- void setShStrtableOffset (uint64_t val) {
- _offsetInStringTable = val; }
- uint32_t flags() { return _flags; }
- uint32_t type() const { return _type; }
- uint64_t link() { return _link; }
- void link(uint64_t val) { _link = val; }
- uint16_t shinfo() { return _shinfo; }
- uint64_t entsize() { return _entsize; }
- SectionChunk(StringRef secName, StringRef segName, bool loadable,
- uint64_t flags , uint64_t link, uint64_t info ,
- uint64_t type, uint64_t entsz, const WriterOptionsELF &op,
- ELFWriter<target_endianness, is64Bits> &writer);
+ // align the atom to the required modulus
+ // align the file offset and the memory offset seperately
+ // this is required so that BSS symbols are handled properly as the
+ // BSS symbols only occupy memory size and not file size
+ uint64_t requiredModulus = atomAlign.modulus;
----------------
This text should be 80 col wrapped.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:252-254
@@ +251,5 @@
+ // append an atom to a Section
+ // The atom gets pushed into a vector that contains
+ // the atom, the atom file offset, the atom virtual address
+ // the atom file offset is aligned appropriately as set by the Reader
+ void appendAtom(const Atom *atom) {
----------------
This text should be 80 col wrapped.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:272-284
@@ +271,15 @@
+ // align the file offset
+ if (fcurrentModulus != requiredModulus) {
+ if (requiredModulus > fcurrentModulus)
+ foffset += requiredModulus - fcurrentModulus;
+ else
+ foffset += align2 + requiredModulus - fcurrentModulus;
+ }
+ // align memory offset
+ if (mcurrentModulus != requiredModulus) {
+ if (requiredModulus > mcurrentModulus)
+ moffset += requiredModulus - mcurrentModulus;
+ else
+ moffset += align2 + requiredModulus - mcurrentModulus;
+ }
+ switch (atomType) {
----------------
This operation can be pulled out into a function.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:286-306
@@ +285,23 @@
+ switch (atomType) {
+ case Atom::definitionRegular:
+ switch(definedAtom->contentType()) {
+ case DefinedAtom::typeCode:
+ case DefinedAtom::typeData:
+ _atoms.push_back(std::make_pair(atom, std::make_pair(foffset, 0)));
+ this->_fsize = foffset + definedAtom->size();
+ this->_msize = moffset + definedAtom->size();
+ break;
+ case DefinedAtom::typeZeroFill:
+ _atoms.push_back(std::make_pair(atom, std::make_pair(moffset, 0)));
+ this->_msize = moffset + definedAtom->size();
+ break;
+ default:
+ this->_fsize = foffset + definedAtom->size();
+ this->_msize = moffset + definedAtom->size();
+ break;
+ }
+ break;
+ default:
+ assert(0);
+ break;
+ }
----------------
This block is indented one too far.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:288-301
@@ +287,16 @@
+ switch(definedAtom->contentType()) {
+ case DefinedAtom::typeCode:
+ case DefinedAtom::typeData:
+ _atoms.push_back(std::make_pair(atom, std::make_pair(foffset, 0)));
+ this->_fsize = foffset + definedAtom->size();
+ this->_msize = moffset + definedAtom->size();
+ break;
+ case DefinedAtom::typeZeroFill:
+ _atoms.push_back(std::make_pair(atom, std::make_pair(moffset, 0)));
+ this->_msize = moffset + definedAtom->size();
+ break;
+ default:
+ this->_fsize = foffset + definedAtom->size();
+ this->_msize = moffset + definedAtom->size();
+ break;
+ }
----------------
This block is indented one too far.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:309-310
@@ +308,4 @@
+ // Set the section alignment to the largest alignment
+ if (this->_align2 < align2)
+ this->_align2 = align2;
+ }
----------------
this->_align2 = std::max(this->_align2, align2);
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:549
@@ -235,1 +548,3 @@
+ void setOrdinal(int64_t ordinal) {
+ ordinal = ordinal;
}
----------------
_ordinal = ordinal;
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:313-315
@@ +312,5 @@
+
+ /// \brief Set the virtual address of each Atom in the Section
+ /// This routine gets called after the linker fixes up
+ /// the virtual address of the section
+ void assignVirtualAddress(int64_t &addr) {
----------------
Word wrap at 80 col.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:305
@@ +304,3 @@
+ default:
+ assert(0);
+ break;
----------------
`llvm_unreachable` with message.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:318
@@ +317,3 @@
+ for (auto ai = _atoms.begin(); ai != _atoms.end(); ++ai) {
+ ai->second.second = addr + ai->second.first;
+ }
----------------
Using pair for this makes this code unreadable. Make a struct to represent this data.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:323-325
@@ +322,5 @@
+
+ /// \brief Set the file offset of each Atom in the section
+ /// This routine gets called after the linker fixes
+ /// up the section offset
+ void assignOffset(int64_t offset) {
----------------
Word wrap at 80 col.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:317
@@ +316,3 @@
+ void assignVirtualAddress(int64_t &addr) {
+ for (auto ai = _atoms.begin(); ai != _atoms.end(); ++ai) {
+ ai->second.second = addr + ai->second.first;
----------------
range base for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:327
@@ +326,3 @@
+ void assignOffset(int64_t offset) {
+ for (auto ai = _atoms.begin(); ai != _atoms.end(); ++ai) {
+ ai->second.first = offset + ai->second.first;
----------------
range base for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:332-333
@@ +331,4 @@
+
+ /// \brief Find the Atom address given a name, this is needed to
+ /// to properly apply relocation
+ bool findAtomAddrByName(const StringRef name, int64_t &addr) {
----------------
Word wrap at 80 col.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:335-341
@@ +334,9 @@
+ bool findAtomAddrByName(const StringRef name, int64_t &addr) {
+ for (auto ai = _atoms.begin(); ai != _atoms.end(); ++ai) {
+ if (ai->first->name() == name) {
+ addr = ai->second.second;
+ return true;
+ }
+ }
+ return false;
+ }
----------------
Linear lookups for things like this are generally bad, however, this is only used to lookup the entry point. This should be documented.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:346-348
@@ +345,5 @@
+ bool occupiesNoDiskSpace() const {
+ if (_contentType == DefinedAtom::typeZeroFill)
+ return true;
+ return false;
+ }
----------------
return _contentType == DefinedAtom::typeZeroFill;
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:351-353
@@ +350,5 @@
+
+ /// \brief The permission of the section is the most
+ /// restrictive permission of all atoms that the
+ /// section contains
+ void setContentPermissions(int32_t perm) {
----------------
Word wrap at 80 col.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:359-360
@@ +358,4 @@
+
+ /// \brief Get the section flags, defined by the permissions
+ /// of the section
+ int64_t flags() {
----------------
Word wrap at 80 col.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:363-380
@@ -194,1 +362,20 @@
+ switch (_contentPermissions) {
+ case DefinedAtom::perm___:
+ return 0;
+
+ case DefinedAtom::permR__:
+ return llvm::ELF::SHF_ALLOC;
+ case DefinedAtom::permR_X:
+ return (llvm::ELF::SHF_ALLOC | llvm::ELF::SHF_EXECINSTR);
+
+ case DefinedAtom::permRW_:
+ case DefinedAtom::permRW_L:
+ return (llvm::ELF::SHF_ALLOC | llvm::ELF::SHF_WRITE);
+
+ case DefinedAtom::permRWX:
+ return (llvm::ELF::SHF_ALLOC | llvm::ELF::SHF_WRITE | llvm::ELF::SHF_EXECINSTR);
+
+ default:
+ break;
+ }
----------------
This block is indented one too far.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:370-377
@@ +369,10 @@
+ case DefinedAtom::permR_X:
+ return (llvm::ELF::SHF_ALLOC | llvm::ELF::SHF_EXECINSTR);
+
+ case DefinedAtom::permRW_:
+ case DefinedAtom::permRW_L:
+ return (llvm::ELF::SHF_ALLOC | llvm::ELF::SHF_WRITE);
+
+ case DefinedAtom::permRWX:
+ return (llvm::ELF::SHF_ALLOC | llvm::ELF::SHF_WRITE | llvm::ELF::SHF_EXECINSTR);
+
----------------
No need for () around these three returns.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:390-391
@@ +389,4 @@
+
+ /// \brief Return the section type, the returned value is recorded
+ /// in the sh_type field of the Section Header
+ int type() {
----------------
Word wrap at 80 col.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:394-405
@@ +393,14 @@
+ switch (_contentType) {
+ case DefinedAtom::typeCode:
+ case DefinedAtom::typeData:
+ case DefinedAtom::typeConstant:
+ return llvm::ELF::SHT_PROGBITS;
+
+ case DefinedAtom::typeZeroFill:
+ return llvm::ELF::SHT_NOBITS;
+
+ // Case to handle section types
+ // Symtab, String Table ...
+ default:
+ return _contentType;
+ }
----------------
Indented one too far.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:440-455
@@ +439,18 @@
+ switch(_segmentType) {
+ case llvm::ELF::PT_INTERP:
+ return "INTERP";
+ case llvm::ELF::PT_LOAD:
+ return "LOAD";
+ case llvm::ELF::PT_GNU_EH_FRAME:
+ return "EH_FRAME";
+ case llvm::ELF::PT_NOTE:
+ return "NOTE";
+ case llvm::ELF::PT_DYNAMIC:
+ return "DYNAMIC";
+ case llvm::ELF::PT_GNU_RELRO:
+ return "RELRO";
+ case llvm::ELF::PT_NULL:
+ return "NULL";
+ default:
+ return "UNKNOWN";
+ }
----------------
Indented one too far.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:460-462
@@ +459,5 @@
+ /// \brief for LLVM style RTTI information
+ static inline bool classof(Section<target_endianness, is64Bits> *s) {
+ return true;
+ }
+
----------------
Not needed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:475
@@ +474,3 @@
+ uint8_t *chunkBuffer = buffer->getBufferStart();
+ for (auto ai = _atoms.begin(); ai != _atoms.end(); ++ai) {
+ const DefinedAtom* definedAtom = llvm::dyn_cast<DefinedAtom>(ai->first);
----------------
range based for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:476
@@ +475,3 @@
+ for (auto ai = _atoms.begin(); ai != _atoms.end(); ++ai) {
+ const DefinedAtom* definedAtom = llvm::dyn_cast<DefinedAtom>(ai->first);
+ // Copy raw content of atom to file buffer.
----------------
The * goes on the right.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:484
@@ +483,3 @@
+ std::copy_n(content.data(), contentSize, atomContent);
+ for (auto ref = definedAtom->begin(); ref != definedAtom->end(); ++ref) {
+ uint32_t offset = ref->offsetInAtom();
----------------
Range based for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:487
@@ +486,3 @@
+ uint64_t targetAddress = 0;
+ if ( ref->target() != nullptr )
+ targetAddress = writer->addressOfAtom(ref->target());
----------------
No spaces after `(` or before `)`.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:487
@@ +486,3 @@
+ uint64_t targetAddress = 0;
+ if ( ref->target() != nullptr )
+ targetAddress = writer->addressOfAtom(ref->target());
----------------
Michael Spencer wrote:
> No spaces after `(` or before `)`.
If we end up here with a null target, don't we need to error or emit a relocation?
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:515
@@ +514,3 @@
+ // field3 : virtual address
+ std::vector<std::pair<const Atom *, std::pair<int64_t, int64_t>>> _atoms;
+ ELFLayout::SegmentType _segmentType;
----------------
This needs to be a vector of some struct. It's difficult to remember what the nested first and second are.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:517
@@ +516,3 @@
+ ELFLayout::SegmentType _segmentType;
+ int64_t _entsize;
+ int64_t _shinfo;
----------------
_entSize;
Or better:
_entitySize;
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:518
@@ +517,3 @@
+ int64_t _entsize;
+ int64_t _shinfo;
+ int64_t _link;
----------------
Same.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:529-541
@@ +528,15 @@
+ MergedSection(StringRef name) : _name(name) {
+ _hasSegment = false;
+ _ordinal = 0;
+ _flags = 0;
+ _size = 0;
+ _memsize = 0;
+ _fileOffset = 0;
+ _startAddr = 0;
+ _shinfo = 0;
+ _entsize = 0;
+ _link = 0;
+ _align2 = 0;
+ _kind = 0;
+ _type = 0;
+ }
----------------
These should be initialized in this init list.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:526
@@ -212,8 +525,3 @@
template<support::endianness target_endianness, bool is64Bits>
-bool IsBss(const Chunk<target_endianness, is64Bits> *A) {
- if (auto X = llvm::dyn_cast<SectionChunk<target_endianness, is64Bits>>(A))
- return X->type() != ELF::SHT_NOBITS;
- llvm_unreachable("Call to a non section type chunk");
- // adding a non-reachable return bool for making compiler happy
- return false;
-}
+class MergedSection {
+public:
----------------
`MergedSections`?
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:552
@@ -238,14 +551,3 @@
-/// \brief A StockSectionChunk is a section created by linker with all
-/// attributes concluded from the defined atom contained within.
-template<support::endianness target_endianness, bool is64Bits>
-class StockSectionChunk : public SectionChunk<target_endianness, is64Bits> {
-public:
- virtual StringRef segmentName() const { return this->_segmentName; }
- void appendAtom(const DefinedAtom*);
- virtual void write(uint8_t *filebuffer);
- const ArrayRef<AtomInfo> atoms() const;
- StockSectionChunk(StringRef sectionName, bool loadable,
- DefinedAtom::ContentType type,
- const WriterOptionsELF &options,
- ELFWriter<target_endianness, is64Bits> &writer);
+ // Sets the Memory size
+ void setMemSize(int64_t memsz) {
----------------
Doxygen.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:557
@@ +556,3 @@
+
+ // Sets the size fo the merged Section
+ void setSize(int64_t fsiz) {
----------------
Doxygen.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:562-563
@@ +561,4 @@
+
+ // The offset of the first section contained in the merged section
+ // is contained here
+ void setFileOffset(int64_t foffset) {
----------------
Doxygen.
Word wrap at 80 col.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:576
@@ +575,3 @@
+ void appendSection(Chunk<target_endianness, is64Bits> *c) {
+ Section<target_endianness, is64Bits> *section;
+ if (c->align2() > _align2)
----------------
This decl should be in the if scope.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:657-659
@@ -266,16 +656,5 @@
- void e_ident(int I, unsigned char C) { _eh.e_ident[I] = C; }
- void e_type(uint16_t type) { _eh.e_type = type; }
- void e_machine(uint16_t machine) { _eh.e_machine = machine; }
- void e_version(uint32_t version) { _eh.e_version = version; }
- void e_entry(uint64_t entry) { _eh.e_entry = entry; }
- void e_phoff(uint64_t phoff) { _eh.e_phoff = phoff; }
- void e_shoff(uint64_t shoff) { _eh.e_shoff = shoff; }
- void e_flags(uint32_t flags) { _eh.e_flags = flags; }
- void e_ehsize(uint16_t ehsize) { _eh.e_ehsize = ehsize; }
- void e_phentsize(uint16_t phentsize) { _eh.e_phentsize = phentsize; }
- void e_phnum(uint16_t phnum) { _eh.e_phnum = phnum; }
- void e_shentsize(uint16_t shentsize) { _eh.e_shentsize = shentsize; }
- void e_shnum(uint16_t shnum) { _eh.e_shnum = shnum; }
- void e_shstrndx(uint16_t shstrndx) { _eh.e_shstrndx = shstrndx; }
- uint64_t size() { return sizeof (Elf_Ehdr); }
+ // Set the segment slice so that it begins at the offset specified
+ // by fileoffset and set the start of the slice to be s and the end
+ // of the slice to be e
+ void set(int64_t fileoffset, int32_t s, int e) {
----------------
Doxygen.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:658-659
@@ +657,4 @@
+ // Set the segment slice so that it begins at the offset specified
+ // by fileoffset and set the start of the slice to be s and the end
+ // of the slice to be e
+ void set(int64_t fileoffset, int32_t s, int e) {
----------------
Please use a half open range. `e` should be one past the end.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:666-668
@@ -282,7 +665,5 @@
- virtual StringRef segmentName() const;
- virtual void write(uint8_t *fileBuffer);
- virtual const char *info();
- static inline bool classof(const Chunk<target_endianness, is64Bits> *c) {
- return c->getChunkKind() == Chunk<target_endianness, is64Bits>::Kind
- ::Header;
+ // Set the segment slice start and end iterators
+ // This is used to walk through the sections that are part of the
+ // Segment slice
+ void setChunks(chunk_iter start, chunk_iter end) {
----------------
Doxygen and 80 col.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:671
@@ -289,1 +670,3 @@
+ startChunkIter = start;
+ endChunkIter = end;
}
----------------
Close or half open?
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:733-738
@@ +732,8 @@
+
+ Segment(const StringRef name,
+ const ELFLayout::SegmentType type,
+ const WriterOptionsELF &options):
+ Chunk<target_endianness, is64Bits>(name,
+ Chunk<target_endianness, is64Bits>::Segment),
+ _options(options) {
+ _segmentType = type;
----------------
Style.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:739-741
@@ +738,5 @@
+ _options(options) {
+ _segmentType = type;
+ _flags = 0;
+ _atomflags = 0;
+ this->_align2 = 0;
----------------
Initialize in init list.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:753-755
@@ +752,5 @@
+ _atomflags = section->atomflags();
+ if (this->_align2 < section->align2()) {
+ this->_align2 = section->align2();
+ }
+ }
----------------
No block needed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:766-777
@@ +765,14 @@
+ /// All Write execute segments follow
+ static bool compare_segments(Chunk<target_endianness, is64Bits> *a,
+ Chunk<target_endianness, is64Bits> *b)
+ {
+
+ Segment<target_endianness, is64Bits> *sega =
+ llvm::dyn_cast<Segment<target_endianness, is64Bits>>(a);
+ Segment<target_endianness, is64Bits> *segb =
+ llvm::dyn_cast<Segment<target_endianness, is64Bits>>(b);
+ if (sega->atomflags() < segb->atomflags())
+ return false;
+ return true;
+ }
+
----------------
Why does this function take two Chunks if it's comparing Segments?
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:779-784
@@ +778,8 @@
+
+ // \brief Start assigning file offset to the segment chunks
+ // The fileoffset needs to be page at the start of the segment
+ // and in addition the fileoffset needs to be aligned to the
+ // max section alignment within the segment. This is required
+ // so that the ELF property p_poffset % p_align = p_vaddr mod p_align
+ // holds true
+ void assignOffset(int64_t startOffset) {
----------------
Doxygen.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:766
@@ +765,3 @@
+ /// All Write execute segments follow
+ static bool compare_segments(Chunk<target_endianness, is64Bits> *a,
+ Chunk<target_endianness, is64Bits> *b)
----------------
Michael Spencer wrote:
> Why does this function take two Chunks if it's comparing Segments?
compareSegments
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:802
@@ +801,3 @@
+ bool isFirstSection = true;
+ for (auto ci = _chunks.begin(); ci != _chunks.end(); ++ci) {
+ section = llvm::dyn_cast<Section<target_endianness, is64Bits>>(*ci);
----------------
Range based for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:803
@@ +802,3 @@
+ for (auto ci = _chunks.begin(); ci != _chunks.end(); ++ci) {
+ section = llvm::dyn_cast<Section<target_endianness, is64Bits>>(*ci);
+ if (isFirstSection) {
----------------
Why is ci Chunk* if all of them are Section*?
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:806-807
@@ +805,4 @@
+ // align the startOffset to the section alignment
+ int64_t newOffset = Chunk<target_endianness, is64Bits>::alignTo
+ (startOffset, section->align2());
+ curSliceFileOffset = newOffset;
----------------
MathExtras.h `RoundUpToAlignment`.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:806
@@ +805,3 @@
+ // align the startOffset to the section alignment
+ int64_t newOffset = Chunk<target_endianness, is64Bits>::alignTo
+ (startOffset, section->align2());
----------------
Michael Spencer wrote:
> MathExtras.h `RoundUpToAlignment`.
Why create a new temp? Just use curSliceFileOffset.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:785
@@ +784,3 @@
+ // holds true
+ void assignOffset(int64_t startOffset) {
+ int startChunk = 0;
----------------
assignOffsets
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:822
@@ +821,3 @@
+ if ((newOffset - curOffset) > _options.pageSize()) {
+ for (auto sei = slices_begin(); sei != slices_end(); ++sei) {
+ if ((*sei)->startChunk() == startChunk) {
----------------
Range based for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:846-847
@@ +845,4 @@
+ sliceAlign = section->align2();
+ }
+ else {
+ if (sliceAlign < section->align2())
----------------
No newline before else.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:800
@@ +799,3 @@
+ endChunkIter = _chunks.end();
+ startChunk = currChunk;
+ bool isFirstSection = true;
----------------
This should be `= 0`.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:816-817
@@ +815,4 @@
+ uint64_t curOffset = curSliceFileOffset + curSliceSize;
+ uint64_t newOffset = Chunk<target_endianness, is64Bits>::alignTo
+ (curOffset, section->align2());
+ SegmentSlice<target_endianness, is64Bits> *slice = nullptr;
----------------
`RoundUpToAlignment`
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:829
@@ +828,3 @@
+ if (!slice) {
+ slice = new SegmentSlice<target_endianness, is64Bits>();
+ _segmentSlices.push_back(slice);
----------------
This should use the writer's `BumpPtrAllocator`.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:822-827
@@ +821,8 @@
+ if ((newOffset - curOffset) > _options.pageSize()) {
+ for (auto sei = slices_begin(); sei != slices_end(); ++sei) {
+ if ((*sei)->startChunk() == startChunk) {
+ slice = *sei;
+ break;
+ }
+ }
+ if (!slice) {
----------------
Michael Spencer wrote:
> Range based for loop.
std::find.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:834
@@ +833,3 @@
+ slice->setChunks(startChunkIter, endChunkIter);
+ slice->setSize(curSliceSize);
+ slice->setAlign(sliceAlign);
----------------
This doesn't seem to include the size of the Section we just added.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:836-839
@@ +835,6 @@
+ slice->setAlign(sliceAlign);
+ int64_t newPageOffset = Chunk<target_endianness, is64Bits>::alignTo
+ (curOffset, _options.pageSize());
+ newOffset = Chunk<target_endianness, is64Bits>::alignTo
+ (newPageOffset, section->align2());
+ curSliceFileOffset = newOffset;
----------------
`RoundUpToAlignment`
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:858-863
@@ +857,8 @@
+ SegmentSlice<target_endianness, is64Bits> *slice = nullptr;
+ for (auto sei = slices_begin(); sei != slices_end(); ++sei) {
+ if ((*sei)->startChunk() == startChunk) {
+ slice = *sei;
+ break;
+ }
+ }
+ if (!slice) {
----------------
std::find.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:865
@@ +864,3 @@
+ if (!slice) {
+ slice = new SegmentSlice<target_endianness, is64Bits>();
+ _segmentSlices.push_back(slice);
----------------
BumpPtrAllocator.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:780-784
@@ +779,7 @@
+ // \brief Start assigning file offset to the segment chunks
+ // The fileoffset needs to be page at the start of the segment
+ // and in addition the fileoffset needs to be aligned to the
+ // max section alignment within the segment. This is required
+ // so that the ELF property p_poffset % p_align = p_vaddr mod p_align
+ // holds true
+ void assignOffset(int64_t startOffset) {
----------------
This should describe the algorithm.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:880
@@ +879,3 @@
+ Section<target_endianness, is64Bits> *section;
+ for (auto sei = slices_begin(); sei != slices_end(); ++sei) {
+ bool firstSlice = (sei == slices_begin());
----------------
Since this can't be a range based for loop:
for (auto sei = slices_begin(), see = slices_end(); sei != see; ++sei)
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:886-888
@@ +885,5 @@
+ // which is the segment address
+ if (isFirstSegment && firstSlice) {
+ (*sei)->setStart(this->startAddr());
+ }
+ else {
----------------
No braces needed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:891-895
@@ +890,7 @@
+ // Align to a page
+ addr = Chunk<target_endianness, is64Bits>::alignTo(addr,
+ _options.pageSize());
+ // Align to the slice alignment
+ addr = Chunk<target_endianness, is64Bits>::alignTo(addr,
+ (*sei)->align2());
+ }
----------------
`RoundUpToAlignment`
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:898
@@ +897,3 @@
+ bool startAddressSet = false;
+ for (auto ci = (*sei)->chunks_begin(); ci != (*sei)->chunks_end(); ++ci) {
+ section = llvm::dyn_cast<Section<target_endianness, is64Bits>>(*ci);
----------------
Don't reevaluate chunks_end() each time through the loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:901-902
@@ +900,4 @@
+ // Align the section address
+ addr = Chunk<target_endianness, is64Bits>::alignTo(addr,
+ section->align2());
+ if (!isFirstSegment && !startAddressSet) {
----------------
`RoundUpToAlignment`
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:925-926
@@ +924,4 @@
+ void write(ELFWriter *writer, OwningPtr<FileOutputBuffer> &buffer) {
+ for (auto sei = slices_begin(); sei != slices_end(); ++sei) {
+ for (auto ci = (*sei)->chunks_begin(); ci != (*sei)->chunks_end(); ++ci) {
+ (*ci)->write(writer, buffer);
----------------
Don't evaluate {slices/chunks}_end() each time through the loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:936-938
@@ +935,5 @@
+ // For LLVM RTTI
+ static inline bool classof(Segment<target_endianness, is64Bits> *s) {
+ return true;
+ }
+
----------------
Not needed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:951
@@ +950,3 @@
+
+ int pgsz() const { return _options.pageSize(); }
+
----------------
`pageSize()`
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:972
@@ +971,3 @@
+ std::vector<Chunk<target_endianness, is64Bits> *> _chunks;
+ std::vector<SegmentSlice<target_endianness, is64Bits> *> _segmentSlices;
+ ELFLayout::SegmentType _segmentType;
----------------
who is responsible for deleting these?
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:984-990
@@ -334,8 +983,9 @@
public:
- ELFStringSectionChunk(const WriterOptionsELF &Options,
- ELFWriter<target_endianness, is64Bits> &writer,
- StringRef secName);
- virtual StringRef segmentName() const { return this->_segmentName; }
- uint64_t addString(StringRef symName);
- const char *info();
- virtual void write(uint8_t *filebuffer);
+ ELFStringTable(const char *str,
+ int32_t order):
+ Section<target_endianness, is64Bits>(str,
+ llvm::ELF::SHT_STRTAB,
+ DefinedAtom::perm___,
+ order,
+ Section<target_endianness, is64Bits>::StringTable) {
+ // the string table has a NULL entry for which
----------------
Style.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:999-1005
@@ -342,1 +998,9 @@
+ static inline bool classof(ELFStringTable<target_endianness, is64Bits> *s) {
+ return true;
+ }
+
+ static inline bool classof(const Section<target_endianness, is64Bits> *c) {
+ return c->kind() == Section<target_endianness, is64Bits>::StringTable;
+ }
+
----------------
Not needed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1008
@@ +1007,3 @@
+ static inline bool classof(const Chunk<target_endianness, is64Bits> *c) {
+ return c->kind() == Section<target_endianness, is64Bits>::StringTable;
+ }
----------------
This is broken. Chunk already has a Kind with the same value.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1022
@@ +1021,3 @@
+ uint8_t *dest = chunkBuffer + this->fileOffset();
+ for (auto si = _strings.begin(); si != _strings.end(); ++si) {
+ memcpy(dest, si->data(), si->size());
----------------
Range based for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1043-1049
@@ -365,1 +1042,9 @@
+ ELFSymbolTable(const char *str,
+ int32_t order):
+ Section<target_endianness, is64Bits>(str,
+ llvm::ELF::SHT_SYMTAB,
+ 0,
+ order,
+ Section<target_endianness, is64Bits>::SymbolTable)
+
----------------
Style.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1061-1067
@@ +1060,9 @@
+
+ static inline bool classof(ELFSymbolTable<target_endianness, is64Bits> *s) {
+ return true;
+ }
+
+ static inline bool classof(const Section<target_endianness, is64Bits> *c) {
+ return c->kind() == Section<target_endianness, is64Bits>::SymbolTable;
+ }
+
----------------
Not needed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1070
@@ +1069,3 @@
+ static inline bool classof(const Chunk<target_endianness, is64Bits> *c) {
+ return c->kind() == Section<target_endianness, is64Bits>::SymbolTable;
+ }
----------------
Broken. Same issue as StringTable.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1073
@@ +1072,3 @@
+
+ void addSymbol(const Atom *a, int32_t shndx, int64_t addr=0) {
+ Elf_Sym *symbol = new(_symbolAllocate.Allocate<Elf_Sym>()) Elf_Sym;
----------------
Spaces around `=`.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1075
@@ +1074,3 @@
+ Elf_Sym *symbol = new(_symbolAllocate.Allocate<Elf_Sym>()) Elf_Sym;
+ unsigned char b = 0, t = 0;
+ symbol->st_name = _stringSection->addString(a->name());
----------------
Better names please.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1073
@@ +1072,3 @@
+
+ void addSymbol(const Atom *a, int32_t shndx, int64_t addr=0) {
+ Elf_Sym *symbol = new(_symbolAllocate.Allocate<Elf_Sym>()) Elf_Sym;
----------------
Michael Spencer wrote:
> Spaces around `=`.
Better names:
atom
sectionIndex
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1085-1100
@@ +1084,18 @@
+ switch (ct = da->contentType()){
+ case DefinedAtom::typeCode:
+ // TODO: add offset
+ symbol->st_value = addr;
+ t = ELF::STT_FUNC;
+ break;
+ case DefinedAtom::typeData:
+ // TODO: add offset
+ symbol->st_value = addr;
+ t = ELF::STT_OBJECT;
+ break;
+ case DefinedAtom::typeZeroFill:
+ t = ELF::STT_COMMON;
+ symbol->st_value = addr;
+ break;
+ default:
+ t = ELF::STT_NOTYPE;
+ }
----------------
Indented one too far.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1086
@@ +1085,3 @@
+ case DefinedAtom::typeCode:
+ // TODO: add offset
+ symbol->st_value = addr;
----------------
Which offset?
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1140-1142
@@ +1139,5 @@
+ // we sort the symbol table to keep all local symbols at the beginning
+ std::stable_sort(_symbolTable.begin(), _symbolTable.end(), ([]
+ (const Elf_Sym *A, const Elf_Sym *B) -> bool {
+ return (A->getBinding() < B->getBinding());}));
+ uint16_t shInfo = 0;
----------------
std::stable_sort(_symbolTable.begin(), _symbolTable.end(),
[](const Elf_Sym *A, const Elf_Sym *B) {
return A->getBinding() < B->getBinding();
});
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1158
@@ +1157,3 @@
+ uint8_t *dest = chunkBuffer + this->fileOffset();
+ for (auto sti = _symbolTable.begin(); sti != _symbolTable.end(); ++sti) {
+ memcpy(dest, (*sti), sizeof(Elf_Sym));
----------------
Range based for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1178-1180
@@ -383,6 +1177,5 @@
- virtual StringRef segmentName() const;
- virtual void write(uint8_t *filebuffer);
- virtual const char *info();
- void createPHeaders();
- uint64_t computeNumber();
+ ELFHeader():Chunk<target_endianness, is64Bits>("elfhdr",
+ Chunk<target_endianness, is64Bits>::ELFHeader)
+ {
+ for (int i=0; i < llvm::ELF::EI_NIDENT; ++i)
----------------
Style
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1181-1182
@@ +1180,4 @@
+ {
+ for (int i=0; i < llvm::ELF::EI_NIDENT; ++i)
+ e_ident(i, 0);
+ e_ident(ELF::EI_MAG0, 0x7f);
----------------
std::fill_n
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1221
@@ +1220,3 @@
+private:
+ Elf_Ehdr _eh;
+};
----------------
Extra whitespace.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1231-1234
@@ -428,6 +1230,6 @@
-template<support::endianness target_endianness, bool is64Bits>
-bool Chunk<target_endianness, is64Bits>::occupiesNoDiskSpace() {
- return false;
-}
+ ELFProgramHeader():Chunk<target_endianness, is64Bits>("elfphdr",
+ Chunk<target_endianness, is64Bits>::ProgramHeader)
+ {
+ }
----------------
Style.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1240-1241
@@ -438,5 +1239,4 @@
-template<support::endianness target_endianness, bool is64Bits>
-uint64_t Chunk<target_endianness, is64Bits>::align2() const {
- return _align2;
-}
+ for (auto sei = segment->slices_begin(); sei != segment->slices_end();
+ ++sei) {
+ if (_phi == _ph.end()) {
----------------
Don't evaluate slices_end() each time through the loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1242
@@ +1241,3 @@
+ ++sei) {
+ if (_phi == _ph.end()) {
+ phdr = new(_allocator.Allocate<Elf_Phdr>()) Elf_Phdr;
----------------
Better names.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1247-1248
@@ +1246,4 @@
+ ret = true;
+ }
+ else {
+ phdr = (*_phi);
----------------
No newline.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1270
@@ +1269,3 @@
+ void setStart(int64_t addr) {
+ this->_start = Chunk<target_endianness, is64Bits>::alignTo(addr, 8);
+ this->_fsize = this->_start - addr;
----------------
`RoundUpToAlignment`
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1285
@@ +1284,3 @@
+ uint8_t *dest = chunkBuffer + this->fileOffset();
+ for (auto phi = _ph.begin(); phi != _ph.end(); ++phi) {
+ memcpy(dest, (*phi), sizeof(Elf_Phdr));
----------------
Range based for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1373
@@ +1372,3 @@
+ uint8_t *dest = chunkBuffer + this->fileOffset();
+ for (auto shi = _sectionInfo.begin(); shi != _sectionInfo.end(); ++shi) {
+ memcpy(dest, (*shi), sizeof(Elf_Shdr));
----------------
Range based for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1442-1443
@@ +1441,4 @@
+ // SectionName, [contentType, contentPermissions]
+ typedef std::pair<StringRef,
+ std::pair<int32_t, int32_t>> Key;
+ typedef typename std::vector<Chunk<target_endianness, is64Bits> *>::iterator
----------------
Please make a proper data structure.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1449
@@ +1448,3 @@
+ // SegmentName, Segment flags
+ typedef std::pair<StringRef, int64_t> SegmentKey;
+ // Merged Sections contain the map of Sectionnames to a vector of sections,
----------------
Same here.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1445
@@ +1444,3 @@
+ typedef typename std::vector<Chunk<target_endianness, is64Bits> *>::iterator
+ chunk_iter;
+ // The key used for Segments
----------------
ChunkIter
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1453
@@ +1452,3 @@
+ typedef std::map<StringRef, MergedSection<target_endianness, is64Bits> *>
+ mergedSectionMap_t;
+ typedef typename std::vector
----------------
`MergedSectionMapT`
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1455
@@ -726,1 +1454,3 @@
+ typedef typename std::vector
+ <MergedSection<target_endianness, is64Bits> *>::iterator merged_section_iter;
----------------
`MergedSectionIter`
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1457-1475
@@ -726,73 +1456,21 @@
-/// \brief Add symbols to symbol table
-/// We examine each property of atom to infer the various st_* fields in Elf*_Sym
-template< support::endianness target_endianness, bool is64Bits>
-void ELFSymbolTableChunk<target_endianness, is64Bits>
- ::addSymbol(const Atom *a, uint16_t shndx) {
- Elf_Sym *symbol = new(_symbolAllocate.Allocate<Elf_Sym>()) Elf_Sym;
- unsigned char b = 0, t = 0;
- symbol->st_name = _stringSection->addString(a->name());
- _symbolToAtom[symbol] = a;
-// In relocatable files, st_value holds a section offset for a defined symbol.
-// st_value is an offset from the beginning of the section that st_shndx
-// identifies. After we assign file offsets we can set this value correctly.
- symbol->st_size = 0;
- symbol->st_shndx = shndx;
- symbol->st_value = 0;
-// FIXME: Need to change and account all STV* when visibilities are supported
- symbol->st_other = ELF::STV_DEFAULT;
- if (const DefinedAtom *da = llvm::dyn_cast<const DefinedAtom>(a)){
- symbol->st_size = da->size();
- lld::DefinedAtom::ContentType ct;
- switch (ct = da->contentType()){
- case DefinedAtom::typeCode:
- t = ELF::STT_FUNC;
- break;
- case DefinedAtom::typeData:
- t = ELF::STT_OBJECT;
- break;
- case DefinedAtom::typeZeroFill:
- // In relocatable files, st_value holds alignment constraints for a symbol whose
- // section index is SHN_COMMON
- if (this->_options.type() == ELF::ET_REL){
- t = ELF::STT_COMMON;
- symbol->st_value = 1 << (da->alignment()).powerOf2;
- symbol->st_shndx = ELF::SHN_COMMON;
- }
- break;
- // TODO:: How to find STT_FILE symbols?
- default:
- t = ELF::STT_NOTYPE;
+ // HashKey for the Section
+ class HashKey {
+ public:
+ int64_t operator() (const Key &k) const {
+ // k.first = section Name
+ // k.second = [contentType, Permissions]
+ return llvm::HashString(k.first) + k.second.first + k.second.second;
}
-
- if (da->scope() == DefinedAtom::scopeTranslationUnit)
- b = ELF::STB_LOCAL;
- else if (da->merge() == DefinedAtom::mergeAsWeak)
- b = ELF::STB_WEAK;
- else
- b = ELF::STB_GLOBAL;
- } else if (const AbsoluteAtom *aa = llvm::dyn_cast<const AbsoluteAtom>(a)){
-//FIXME: Absolute atoms need more properties to differentiate each other
-// based on binding and type of symbol
- t = ELF::STT_OBJECT;
+ };
- switch (aa->scope()) {
- case AbsoluteAtom::scopeLinkageUnit:
- symbol->st_other = ELF::STV_HIDDEN;
- b = ELF::STB_LOCAL;
- break;
- case AbsoluteAtom::scopeTranslationUnit:
- b = ELF::STB_LOCAL;
- break;
- case AbsoluteAtom::scopeGlobal:
- b = ELF::STB_GLOBAL;
- break;
+ // HashKey for the Segment
+ class SegmentHashKey {
+ public:
+ int64_t operator() (const SegmentKey &k) const {
+ // k.first = SegmentName
+ // k.second = SegmentFlags
+ return llvm::HashString(k.first) + k.second;
}
- symbol->st_value = aa->value();
- } else {
- symbol->st_value = 0;
- t = ELF::STT_NOTYPE;
- b = ELF::STB_LOCAL;
- }
- symbol->setBindingAndType(b, t);
+ };
----------------
Use the hashing stuff in llvm/Support/Hashing.h
Most importantly use hash_combine instead of some custom mechanism.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1478
@@ +1477,3 @@
+ typedef std::unordered_map<Key,
+ Section<target_endianness, is64Bits>*, HashKey> sectionMap_t;
+ typedef std::unordered_map<SegmentKey,
----------------
`SectionMapT`
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1481
@@ -802,1 +1480,3 @@
+ Segment<target_endianness, is64Bits>*,
+ SegmentHashKey> segmentMap_t;
----------------
`SegmentMapT`
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1486-1488
@@ +1485,5 @@
+ /// \brief Return the section order for a input section
+ virtual SectionOrder getSectionOrder(const StringRef name,
+ int32_t contentType,
+ int32_t contentPermissions) {
+ switch (contentType) {
----------------
Line up arguments.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1490-1518
@@ -826,8 +1489,31 @@
+ switch (contentType) {
+ case DefinedAtom::typeCode:
+ if (name.startswith(".eh_frame_hdr"))
+ return ORDER_EH_FRAMEHDR;
+ if (name.startswith(".eh_frame"))
+ return ORDER_EH_FRAME;
+ else if (name.startswith(".init"))
+ return ORDER_INIT;
+ else if (name.startswith(".fini"))
+ return ORDER_FINI;
+ else if (name.startswith(".hash"))
+ return ORDER_HASH;
+ else
+ return ORDER_TEXT;
-template<support::endianness target_endianness, bool is64Bits>
-void ELFSymbolTableChunk<target_endianness, is64Bits>::
- write(uint8_t *chunkBuffer) {
- uint64_t chunkOffset = 0;
- for (auto it : _symbolTable) {
- ::memcpy(chunkBuffer + chunkOffset, it, this->_entsize);
- chunkOffset += this->_entsize;
+ case DefinedAtom::typeConstant:
+ return ORDER_RODATA;
+
+ case DefinedAtom::typeData:
+ return ORDER_DATA;
+
+ case DefinedAtom::typeZeroFill:
+ return ORDER_BSS;
+
+ default:
+ // If we get passed in a section push it to OTHER
+ if (contentPermissions == DefinedAtom::perm___)
+ return ORDER_OTHER;
+
+ return ORDER_NOT_DEFINED;
+ }
----------------
Indented one too far.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1491-1502
@@ -826,1 +1490,14 @@
+ case DefinedAtom::typeCode:
+ if (name.startswith(".eh_frame_hdr"))
+ return ORDER_EH_FRAMEHDR;
+ if (name.startswith(".eh_frame"))
+ return ORDER_EH_FRAME;
+ else if (name.startswith(".init"))
+ return ORDER_INIT;
+ else if (name.startswith(".fini"))
+ return ORDER_FINI;
+ else if (name.startswith(".hash"))
+ return ORDER_HASH;
+ else
+ return ORDER_TEXT;
----------------
llvm::StringSwitch in llvm/ADT/StringSwitch.h
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1522-1529
@@ -836,23 +1521,10 @@
-//===----------------------------------------------------------------------===//
-// ELFHeaderChunk
-//===----------------------------------------------------------------------===//
-template<support::endianness target_endianness, bool is64Bits>
-ELFHeaderChunk<target_endianness, is64Bits>
- ::ELFHeaderChunk(const WriterOptionsELF &options,
- const File &File)
- : Chunk<target_endianness, is64Bits>(Chunk<target_endianness, is64Bits>
- ::Kind::Header){
- this->_size = size();
- e_ident(ELF::EI_MAG0, 0x7f);
- e_ident(ELF::EI_MAG1, 'E');
- e_ident(ELF::EI_MAG2, 'L');
- e_ident(ELF::EI_MAG3, 'F');
- e_ident(ELF::EI_CLASS, (options.is64Bit() ? ELF::ELFCLASS64
- : ELF::ELFCLASS32));
- e_ident(ELF::EI_DATA, (options.endianness() == llvm::support::big)
- ? ELF::ELFDATA2MSB
- : ELF::ELFDATA2LSB);
- e_ident(ELF::EI_VERSION, 1);
- e_ident(ELF::EI_OSABI, ELF::ELFOSABI_NONE);
+ /// \brief This maps the input sections to the output section names
+ StringRef getSectionName(const StringRef name) {
+ if (name.startswith(".text"))
+ return ".text";
+ if (name.startswith(".rodata"))
+ return ".rodata";
+ return name;
+ }
----------------
Same thing needs to happen with .bss I believe. I'm not sure what criteria ld uses to do this.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1535-1567
@@ -862,56 +1534,35 @@
+ switch(section->order()) {
+ case ORDER_INTERP:
+ return llvm::ELF::PT_INTERP;
- e_entry(0ULL);
- e_phoff(0);
- e_shoff(0ULL);
-
- e_flags(2);
- e_ehsize(this->_size);
- e_phentsize(0);
- e_phnum(0);
- e_shentsize(0);
- e_shnum(0);
- e_shstrndx(0);
- this->setGroup(CG_HEADER);
-}
+ case ORDER_TEXT:
+ case ORDER_HASH:
+ case ORDER_DYNAMIC_SYMBOLS:
+ case ORDER_DYNAMIC_STRINGS:
+ case ORDER_INIT:
+ case ORDER_PLT:
+ case ORDER_FINI:
+ case ORDER_RODATA:
+ case ORDER_EH_FRAME:
+ case ORDER_EH_FRAMEHDR:
+ return llvm::ELF::PT_LOAD;
-template<support::endianness target_endianness, bool is64Bits>
-StringRef ELFHeaderChunk<target_endianness, is64Bits>
- ::segmentName() const {
- return "ELF";
-}
+ case ORDER_NOTE:
+ return llvm::ELF::PT_NOTE;
-template<support::endianness target_endianness, bool is64Bits>
-void ELFHeaderChunk<target_endianness, is64Bits>
- ::write(uint8_t *chunkBuffer) {
- ::memcpy(chunkBuffer, &_eh, size());
-}
+ case ORDER_DYNAMIC:
+ return llvm::ELF::PT_DYNAMIC;
-template<support::endianness target_endianness, bool is64Bits>
-const char *ELFHeaderChunk<target_endianness, is64Bits>::info() {
- return "elf_header";
-}
+ case ORDER_CTORS:
+ case ORDER_DTORS:
+ case ORDER_GOT:
+ return llvm::ELF::PT_GNU_RELRO;
-//===----------------------------------------------------------------------===//
-// ELFSectionHeaderChunk
-// List of Section Headers:
-//[Nr] Name Type Addr Off Size ES Flg Lk Inf Al
-//[ 0] NULL 00000000 000000 000000 00 0 0 0
-//[ 1] .text PROGBITS 00000000 000034 000040 00 AX 0 0 4
-//===----------------------------------------------------------------------===//
-template<support::endianness target_endianness, bool is64Bits>
-ELFSectionHeaderChunk<target_endianness, is64Bits>
- ::ELFSectionHeaderChunk(const WriterOptionsELF& options,
- ELFWriter<target_endianness,
- is64Bits> &writer)
- : Chunk<target_endianness, is64Bits>(Chunk<target_endianness, is64Bits>
- ::Kind::Header)
- , _options(options)
- , _writer(writer) {
- this->_size = 0;
- this->_align2 = 0;
- // The first element in the list is always NULL
- Elf_Shdr *nullshdr = new (_sectionAllocate.Allocate<Elf_Shdr>()) Elf_Shdr;
- ::memset(nullshdr, 0, sizeof (Elf_Shdr));
- _sectionInfo.push_back(nullshdr);
- this->_size += sizeof (Elf_Shdr);
- this->setGroup(CG_FILE_END);
+ case ORDER_GOT_PLT:
+ case ORDER_DATA:
+ case ORDER_BSS:
+ return llvm::ELF::PT_LOAD;
+
+ default:
+ return llvm::ELF::PT_NULL;
+ }
----------------
Indented one too far.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1576-1598
@@ -920,5 +1575,25 @@
+ switch(section->order()) {
+ case ORDER_INTERP:
+ case ORDER_HASH:
+ case ORDER_DYNAMIC_SYMBOLS:
+ case ORDER_DYNAMIC_STRINGS:
+ case ORDER_INIT:
+ case ORDER_PLT:
+ case ORDER_TEXT:
+ case ORDER_FINI:
+ case ORDER_RODATA:
+ case ORDER_EH_FRAME:
+ case ORDER_EH_FRAMEHDR:
+ case ORDER_NOTE:
+ case ORDER_DYNAMIC:
+ case ORDER_CTORS:
+ case ORDER_DTORS:
+ case ORDER_GOT:
+ case ORDER_GOT_PLT:
+ case ORDER_DATA:
+ case ORDER_BSS:
+ return true;
-template<support::endianness target_endianness, bool is64Bits>
-void ELFSectionHeaderChunk<target_endianness, is64Bits>::computeSize(){
- this->_size = (this->_writer.sectionChunks().size() + 1) * sizeof(Elf_Shdr);
-}
+ default:
+ return false;
+ }
----------------
Indented one too far.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1605-1606
@@ +1604,4 @@
+ const DefinedAtom *definedAtom = dyn_cast<DefinedAtom> (atom);
+ const StringRef sectionName = getSectionName
+ (definedAtom->customSectionName());
+ const lld::DefinedAtom::ContentPermissions permissions =
----------------
const StringRef sectionName =
getSectionName(definedAtom->customSectionName());
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1604
@@ +1603,3 @@
+ virtual error_code addAtom(const Atom *atom) {
+ const DefinedAtom *definedAtom = dyn_cast<DefinedAtom> (atom);
+ const StringRef sectionName = getSectionName
----------------
No newline between > and (.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1640-1641
@@ +1639,4 @@
+ // Merge sections with the same name into a MergedSection
+ void mergeSimiliarSections()
+ {
+ MergedSection<target_endianness, is64Bits> *mergedSection;
----------------
No newline before {.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1644
@@ +1643,3 @@
+
+ for (auto si = _sections.begin(); si != _sections.end(); ++si) {
+ const std::pair<StringRef, MergedSection<target_endianness, is64Bits> *>
----------------
Range based for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1665-1666
@@ -944,6 +1664,4 @@
- for (const auto &chunk : _writer.sectionChunks()) {
- Elf_Shdr *shdr = new (_sectionAllocate.Allocate<Elf_Shdr>()) Elf_Shdr;
- StringRef Name = chunk->sectionName();
- if (chunk->shStrtableOffset() == 0){
- chunk->setShStrtableOffset(str->addString(Name));
+ void assignSectionsToSegments()
+ {
+ // sort the sections by their order as defined by the layout
----------------
No newline before {.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1668-1671
@@ +1667,6 @@
+ // sort the sections by their order as defined by the layout
+ std::stable_sort(_sections.begin(), _sections.end(), ([]
+ (Chunk<target_endianness, is64Bits> *A,
+ Chunk<target_endianness, is64Bits> *B) ->
+ bool { return (A->order() < B->order());}));
+ // Merge all sections
----------------
std::stable_sort(_sections.begin(), _sections.end(),
[](Chunk<target_endianness, is64Bits> *A, Chunk<target_endianness, is64Bits> *B) {
return A->order() < B->order();
});
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1676
@@ +1675,3 @@
+ int ordinal = 1;
+ for (auto msi = _mergedSections.begin(); msi != _mergedSections.end();
+ ++msi) {
----------------
Range based for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1679-1680
@@ +1678,4 @@
+ (*msi)->setOrdinal(ordinal);
+ for (auto ai = (*msi)->begin_sections(); ai != (*msi)->end_sections();
+ ++ai) {
+ (*ai)->setOrdinal(ordinal);
----------------
Don't evaluate end_sections() each time through the loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1687-1690
@@ +1686,6 @@
+ Segment<target_endianness, is64Bits> *segment;
+ for (auto msi = merged_sections_begin(); msi != merged_sections_end();
+ ++msi) {
+ for (auto ai = (*msi)->begin_sections(); ai != (*msi)->end_sections();
+ ++ai) {
+ if ((*ai)->kind() == Chunk<target_endianness, is64Bits>::Section) {
----------------
Don't evaluate end condition each time through the loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1707-1708
@@ +1706,4 @@
+ segment = segmentInsert.first->second;
+ }
+ else {
+ segment = new (_allocator.Allocate
----------------
No newline before else.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1727-1735
@@ -971,14 +1726,11 @@
-template<support::endianness target_endianness, bool is64Bits>
-StringRef ELFSectionHeaderChunk<target_endianness, is64Bits>
- ::segmentName() const {
- return "PT_NULL";
-}
-
-template<support::endianness target_endianness, bool is64Bits>
-void ELFSectionHeaderChunk<target_endianness, is64Bits>
- ::write(uint8_t *chunkBuffer) {
- for (const auto si : _sectionInfo) {
- ::memcpy(chunkBuffer, si, sizeof(*si));
- chunkBuffer += sizeof (*si);
+ void findSectionByOrder(chunk_iter *chunk, DefaultSectionOrder s) {
+ for (auto ai = sections_begin(); ai != sections_end(); ++ai) {
+ if ((*ai)->order() == s) {
+ chunk = *ai;
+ return;
+ }
+ }
+ chunk = sections_end();
}
+ void findSectionByName(chunk_iter *chunk, StringRef s) {
----------------
This is never used.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1736-1744
@@ -984,3 +1735,11 @@
}
-}
+ void findSectionByName(chunk_iter *chunk, StringRef s) {
+ for (auto ai = sections_begin(); ai != sections_end(); ++ai) {
+ if ((*ai)->name() == s) {
+ chunk = *ai;
+ return;
+ }
+ }
+ chunk = sections_end();
+ }
----------------
This is never used.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1746
@@ -986,9 +1745,3 @@
-template<support::endianness target_endianness, bool is64Bits>
-uint16_t ELFSectionHeaderChunk<target_endianness, is64Bits>::count() {
- return _sectionInfo.size();
-}
-template<support::endianness target_endianness, bool is64Bits>
-uint16_t ELFSectionHeaderChunk<target_endianness, is64Bits>::size() {
- return sizeof (Elf_Shdr);
-}
+ void assignFileOffset() {
+ Segment<target_endianness, is64Bits> *segment;
----------------
`assignFileOffsets`
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1749-1750
@@ -995,5 +1748,4 @@
-template<support::endianness target_endianness, bool is64Bits>
-const char *ELFSectionHeaderChunk<target_endianness, is64Bits>::info() {
- return "elf_section_header";
-}
+ std::sort(_segments.begin(), _segments.end(),
+ Segment<target_endianness, is64Bits>::compare_segments);
+ int ordinal = 0;
----------------
Align arguments.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1752
@@ +1751,3 @@
+ int ordinal = 0;
+ for (auto si = _segments.begin(); si != _segments.end(); ++si) {
+ segment = llvm::dyn_cast<Segment<target_endianness, is64Bits>>(*si);
----------------
Range based for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1759
@@ +1758,3 @@
+ int64_t offset = 0;
+ for (auto si = _segments.begin(); si != _segments.end(); ++si) {
+ segment = llvm::dyn_cast<Segment<target_endianness, is64Bits>>(*si);
----------------
Range based for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1752-1763
@@ +1751,14 @@
+ int ordinal = 0;
+ for (auto si = _segments.begin(); si != _segments.end(); ++si) {
+ segment = llvm::dyn_cast<Segment<target_endianness, is64Bits>>(*si);
+ segment->setOrdinal(++ordinal);
+ }
+ // Compute the number of segments that might be needed, so that the
+ // size of the program header can be computed
+ int64_t offset = 0;
+ for (auto si = _segments.begin(); si != _segments.end(); ++si) {
+ segment = llvm::dyn_cast<Segment<target_endianness, is64Bits>>(*si);
+ segment->assignOffset(offset);
+ offset += segment->filesize();
+ }
+ }
----------------
Michael Spencer wrote:
> Range based for loop.
Why are these two loops split?
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1753
@@ +1752,3 @@
+ for (auto si = _segments.begin(); si != _segments.end(); ++si) {
+ segment = llvm::dyn_cast<Segment<target_endianness, is64Bits>>(*si);
+ segment->setOrdinal(++ordinal);
----------------
Why is _segments not an array of Segment*?
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1791
@@ +1790,3 @@
+ while (true) {
+ for (auto si = _segments.begin(); si != _segments.end(); ++si) {
+ segment = llvm::dyn_cast<Segment<target_endianness, is64Bits>>(*si);
----------------
Range based for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1801
@@ +1800,3 @@
+ // Fix the offsets after adding the program header
+ for (auto si = _segments.begin(); si != _segments.end(); ++si) {
+ segment = llvm::dyn_cast<Segment<target_endianness, is64Bits>>(*si);
----------------
Range based for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1804-1805
@@ +1803,4 @@
+ // Align the segment to a page boundary
+ fileoffset = Chunk<target_endianness, is64Bits>::alignTo(fileoffset,
+ _options.pageSize());
+ segment->assignOffset(fileoffset);
----------------
`RoundUpToAlignment`
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1810
@@ +1809,3 @@
+ // start assigning virtual addresses
+ for (auto si = _segments.begin(); si != _segments.end(); ++si) {
+ segment = llvm::dyn_cast<Segment<target_endianness, is64Bits>>(*si);
----------------
Range based for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1813-1817
@@ +1812,7 @@
+ segment->setStart(startAddress);
+ // The first segment has the startAddress set to the base address
+ // as we have added the file header and the program header
+ // dont align the first segment to the pagesize
+ // TODO: check in addition to this, that the first segment is a load
+ // segment, only load segments have the addresses aligned
+ segment->assignVirtualAddress(address, (si == _segments.begin()));
----------------
Word wrap at 80 col.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1820-1821
@@ +1819,4 @@
+ segment->setMemSize(address - startAddress);
+ startAddress = Chunk<target_endianness, is64Bits>::alignTo(address,
+ _options.pageSize());
+ }
----------------
`RoundUpToAlignment`
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1827
@@ +1826,3 @@
+ // Fix the offsets of all the atoms within a section
+ for (auto si = _sections.begin(); si != _sections.end(); ++si) {
+ section = llvm::dyn_cast<Section<target_endianness, is64Bits>>(*si);
----------------
Range based for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1833
@@ +1832,3 @@
+ // Set the size of the merged Sections
+ for (auto msi = merged_sections_begin(); msi != merged_sections_end(); ++msi) {
+ int64_t sectionfileoffset = 0;
----------------
Don't reevaluate merged_sections_end() each time through the loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1873
@@ +1872,3 @@
+ int64_t size = 0;
+ for (auto si = _segments.begin(); si != _segments.end(); ++si) {
+ fileoffset = (*si)->fileOffset();
----------------
Range based for loop.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:2014-2015
@@ -1188,27 +2013,4 @@
template<support::endianness target_endianness, bool is64Bits>
-void ELFWriter<target_endianness, is64Bits>::build(const lld::File &file){
-// Create objects for each chunk.
- createChunks(file);
- assignFileOffsets();
- _phdr->createPHeaders();
- _sectionHeaderChunk->createHeaders();
- // Creating program headers changed its size. so we need to re-assign offsets
- assignFileOffsets();
- _sectionHeaderChunk->fixOffsets();
- _phdr->createPHeaders();
- buildAtomToAddressMap();
- _symtable->fixSymbolValue();
- ehc->e_shentsize(_sectionHeaderChunk->size());
- ehc->e_shnum(_sectionHeaderChunk->count());
- // We need to put the index of section string table in ELF header
- // first two chunks are not sections so we subtract 2 to start sections
- // and add 1 since we have a NULL header
- ehc->e_shstrndx(_shstrtable->ordinal() - 1);
- ehc->e_phnum(_phdr->computeNumber());
- ehc->e_phoff(_phdr->fileOffset());
- ehc->e_phentsize(sizeof(Elf_Phdr));
- for (auto i : _stockSectionChunks) {
- if(const DefinedAtom* da = findDefinedAtomByName(_options.entryPoint(), i)) {
- ehc->e_entry(this->addressOfAtom(da));
- return;
- }
+void ELFExecutableWriter<target_endianness, is64Bits>::buildChunks
+ (const lld::File &file){
+ for (const DefinedAtom *definedAtom : file.defined() ) {
----------------
void ELFExecutableWriter<target_endianness, is64Bits>
::buildChunks(const lld::File &file){
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:2163-2165
@@ -1423,2 +2162,5 @@
+
return buffer->commit();
+
+ return error_code::success();
}
----------------
Which return?
http://llvm-reviews.chandlerc.com/D222
More information about the llvm-commits
mailing list