[llvm-commits] [PATCH] [lld] patch for ELF Writer to get it to 'Hello world'
Shankar Kalpathi Easwaran
shankarke at gmail.com
Fri Dec 21 08:49:26 PST 2012
I have made changes to all of the comments except for changes for using std::pair. I will attend to that in the next patch.
================
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
----------------
Michael Spencer wrote:
> 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.
Yes. I sort the sections/segments by the permissions that are set.
================
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
},
----------------
Michael Spencer wrote:
> I don't understand why this change is here.
To support two absolute symbols that have the same name.
================
Comment at: lib/ReaderWriter/ELF/ELFAtoms.h:1
@@ +1,2 @@
+#ifndef LLD_ELFATOMS_H_
+#define LLD_ELFATOMS_H_
----------------
Michael Spencer wrote:
> Needs a proper file header.
>
> Don't add a trailing _.
ok
================
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;
+};
----------------
Michael Spencer wrote:
> I don't see a reason for any of these except writeFile to be virtual. They are never called virtually.
Once we have multiple ELF writers (to emit sharedlibrary/relocatables) we would need all of them to be virtual.
================
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),
----------------
Michael Spencer wrote:
> 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.
Changed the names to add ELF in the front
================
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() {}
----------------
Michael Spencer wrote:
> Please match the style used elsewhere.
>
> lang=c++
> Chunk(StringRef name, Kind kind)
> : _name(name)
> , _kind(kind)
> , _fsize(0)
> , etc... {}
Done.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:98
@@ +97,3 @@
+ // Does the chunk occupy disk space
+ virtual bool occupiesNoDiskSpace() {
+ return false;
----------------
Michael Spencer wrote:
> 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.
Ok.
================
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; }
----------------
Michael Spencer wrote:
> fileSize.
Done.
================
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
----------------
Michael Spencer wrote:
> This can be replaced by MathExtras.h RoundUpToAlignment.
Done.
================
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; }
----------------
Michael Spencer wrote:
> What is the difference between these two function? Is one the virtual address?
yes.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:129
@@ +128,3 @@
+ // Writer the chunk
+ virtual void write(ELFWriter *writer,
+ OwningPtr<FileOutputBuffer> &buffer) = 0;
----------------
Michael Spencer wrote:
> No chuck seems to use the writer.
Section uses the writer to get the address of the atom.
================
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;
+ }
+
----------------
Michael Spencer wrote:
> This is no longer needed for llvm RTTI.
Removed this definition.
================
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
----------------
Michael Spencer wrote:
> Don't add extra empty /// lines around comments.
Removed the extra empty /// lines
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:154-155
@@ +153,4 @@
+///
+class ELFLayoutOptions
+{
+public:
----------------
Michael Spencer wrote:
> No newline before {.
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:162
@@ -153,1 +161,3 @@
+ // parse the linker script
+ error_code parseLinkerScript();
----------------
Michael Spencer wrote:
> Doxygen.
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:165
@@ +164,3 @@
+
+ // Is the current section present in the linker script
+ bool isSectionPresent();
----------------
Michael Spencer wrote:
> Doxygen.
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:180-181
@@ +179,4 @@
+///
+class ELFLayout
+{
+public:
----------------
Michael Spencer wrote:
> No newline before {.
Changed.
================
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,
----------------
Michael Spencer wrote:
> Doxygen.
Changed.
================
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
----------------
Michael Spencer wrote:
> Line up args.
Changed.
================
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;
----------------
Michael Spencer wrote:
> Doxygen.
Changed.
================
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;
----------------
Michael Spencer wrote:
> Doxygen.
Changed.
================
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;
----------------
Michael Spencer wrote:
> Doxygen.
Changed.
================
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;
----------------
Michael Spencer wrote:
> Doxygen.
Changed.
================
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;
----------------
Michael Spencer wrote:
> Doxygen.
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:206-208
@@ +205,5 @@
+ ELFLayout() {}
+ ELFLayout(WriterOptionsELF &writerOptions,
+ ELFLayoutOptions &layoutOptions) : _writerOptions(writerOptions),
+ _layoutOptions(layoutOptions) { }
+ virtual ~ELFLayout() { }
----------------
Michael Spencer wrote:
> Style.
Changed.
================
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;
----------------
Michael Spencer wrote:
> Style.
Changed.
================
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);
----------------
Michael Spencer wrote:
> These should be in the init list.
Changed.
================
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) {
----------------
Michael Spencer wrote:
> This text should be 80 col wrapped.
Changed.
================
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");
----------------
Michael Spencer wrote:
> The * goes on the right.
Changed.
================
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();
----------------
Michael Spencer wrote:
> fOffset and mOffset.
Changed.
================
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;
----------------
Michael Spencer wrote:
> This text should be 80 col wrapped.
Changed.
================
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) {
----------------
Michael Spencer wrote:
> This operation can be pulled out into a function.
Pulled into a seperate function
================
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;
+ }
----------------
Michael Spencer wrote:
> This block is indented one too far.
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:305
@@ +304,3 @@
+ default:
+ assert(0);
+ break;
----------------
Michael Spencer wrote:
> `llvm_unreachable` with message.
Changed.
================
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;
+ }
----------------
Michael Spencer wrote:
> This block is indented one too far.
Changed.
================
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;
+ }
----------------
Michael Spencer wrote:
> this->_align2 = std::max(this->_align2, align2);
Changed.
================
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) {
----------------
Michael Spencer wrote:
> Word wrap at 80 col.
Changed.
================
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;
----------------
Michael Spencer wrote:
> range base for loop.
Changed.
================
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;
+ }
----------------
Michael Spencer wrote:
> Using pair for this makes this code unreadable. Make a struct to represent this data.
TODO: Will make the change in the next patch. Leaving it for now as is.
================
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) {
----------------
Michael Spencer wrote:
> Word wrap at 80 col.
Changed.
================
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;
----------------
Michael Spencer wrote:
> range base for loop.
Changed.
================
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) {
----------------
Michael Spencer wrote:
> Word wrap at 80 col.
Changed.
================
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;
+ }
----------------
Michael Spencer wrote:
> Linear lookups for things like this are generally bad, however, this is only used to lookup the entry point. This should be documented.
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:346-348
@@ +345,5 @@
+ bool occupiesNoDiskSpace() const {
+ if (_contentType == DefinedAtom::typeZeroFill)
+ return true;
+ return false;
+ }
----------------
Michael Spencer wrote:
> return _contentType == DefinedAtom::typeZeroFill;
Changed.
================
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) {
----------------
Michael Spencer wrote:
> Word wrap at 80 col.
Changed.
================
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() {
----------------
Michael Spencer wrote:
> Word wrap at 80 col.
Changed.
================
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);
+
----------------
Michael Spencer wrote:
> No need for () around these three returns.
Changed.
================
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;
+ }
----------------
Michael Spencer wrote:
> This block is indented one too far.
Changed
================
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() {
----------------
Michael Spencer wrote:
> Word wrap at 80 col.
Changed.
================
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;
+ }
----------------
Michael Spencer wrote:
> Indented one too far.
Changed
================
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";
+ }
----------------
Michael Spencer wrote:
> Indented one too far.
Changed
================
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;
+ }
+
----------------
Michael Spencer wrote:
> Not needed.
Removed.
================
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);
----------------
Michael Spencer wrote:
> range based for loop.
Changed.
================
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.
----------------
Michael Spencer wrote:
> The * goes on the right.
Done.
================
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();
----------------
Michael Spencer wrote:
> Range based for loop.
Changed
================
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:
> 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?
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:487
@@ +486,3 @@
+ uint64_t targetAddress = 0;
+ if ( ref->target() != nullptr )
+ targetAddress = writer->addressOfAtom(ref->target());
----------------
Shankar Kalpathi Easwaran wrote:
> Michael Spencer wrote:
> > 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?
> Changed.
Changed and removed ()
================
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;
----------------
Michael Spencer wrote:
> This needs to be a vector of some struct. It's difficult to remember what the nested first and second are.
TODO: Will make the change in the next patch. Leaving it for now as is.
================
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:
----------------
Michael Spencer wrote:
> `MergedSections`?
Changed.
================
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;
+ }
----------------
Michael Spencer wrote:
> These should be initialized in this init list.
Done.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:549
@@ -235,1 +548,3 @@
+ void setOrdinal(int64_t ordinal) {
+ ordinal = ordinal;
}
----------------
Michael Spencer wrote:
> _ordinal = ordinal;
Nice catch.
================
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) {
----------------
Michael Spencer wrote:
> Doxygen.
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:557
@@ +556,3 @@
+
+ // Sets the size fo the merged Section
+ void setSize(int64_t fsiz) {
----------------
Michael Spencer wrote:
> Doxygen.
Changed.
================
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) {
----------------
Michael Spencer wrote:
> Doxygen.
>
> Word wrap at 80 col.
Done.
================
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)
----------------
Michael Spencer wrote:
> This decl should be in the if scope.
Changed.
================
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) {
----------------
Michael Spencer wrote:
> Please use a half open range. `e` should be one past the end.
Changed.
================
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) {
----------------
Michael Spencer wrote:
> Doxygen and 80 col.
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:671
@@ -289,1 +670,3 @@
+ startChunkIter = start;
+ endChunkIter = end;
}
----------------
Michael Spencer wrote:
> Close or half open?
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;
----------------
Michael Spencer wrote:
> Style.
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:739-741
@@ +738,5 @@
+ _options(options) {
+ _segmentType = type;
+ _flags = 0;
+ _atomflags = 0;
+ this->_align2 = 0;
----------------
Michael Spencer wrote:
> Initialize in init list.
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:753-755
@@ +752,5 @@
+ _atomflags = section->atomflags();
+ if (this->_align2 < section->align2()) {
+ this->_align2 = section->align2();
+ }
+ }
----------------
Michael Spencer wrote:
> No block needed.
Changed.
================
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:
> Michael Spencer wrote:
> > Why does this function take two Chunks if it's comparing Segments?
> compareSegments
Changed.
================
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;
+ }
+
----------------
Shankar Kalpathi Easwaran wrote:
> Michael Spencer wrote:
> > Michael Spencer wrote:
> > > Why does this function take two Chunks if it's comparing Segments?
> > compareSegments
> Changed.
Changed to take 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) {
----------------
Michael Spencer wrote:
> Doxygen.
Done.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:785
@@ +784,3 @@
+ // holds true
+ void assignOffset(int64_t startOffset) {
+ int startChunk = 0;
----------------
Michael Spencer wrote:
> assignOffsets
Done.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:800
@@ +799,3 @@
+ endChunkIter = _chunks.end();
+ startChunk = currChunk;
+ bool isFirstSection = true;
----------------
Michael Spencer wrote:
> This should be `= 0`.
Changed.
================
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);
----------------
Michael Spencer wrote:
> Range based for loop.
Changed.
================
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) {
----------------
Michael Spencer wrote:
> Why is ci Chunk* if all of them are Section*?
Changed to section.
================
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:
> Michael Spencer wrote:
> > MathExtras.h `RoundUpToAlignment`.
> Why create a new temp? Just use curSliceFileOffset.
No curSliceFileOffset is the start offset of the slice.
================
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;
----------------
Shankar Kalpathi Easwaran wrote:
> Michael Spencer wrote:
> > Michael Spencer wrote:
> > > MathExtras.h `RoundUpToAlignment`.
> > Why create a new temp? Just use curSliceFileOffset.
> No curSliceFileOffset is the start offset of the slice.
Changed.
================
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;
----------------
Michael Spencer wrote:
> `RoundUpToAlignment`
Changed.
================
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) {
----------------
Michael Spencer wrote:
> Michael Spencer wrote:
> > Range based for loop.
> std::find.
Changed.
================
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) {
----------------
Shankar Kalpathi Easwaran wrote:
> Michael Spencer wrote:
> > Michael Spencer wrote:
> > > Range based for loop.
> > std::find.
> Changed.
Its calling a function for every call, not sure how to use std::find here. Added a TODO for cleanup.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:829
@@ +828,3 @@
+ if (!slice) {
+ slice = new SegmentSlice<target_endianness, is64Bits>();
+ _segmentSlices.push_back(slice);
----------------
Michael Spencer wrote:
> This should use the writer's `BumpPtrAllocator`.
Done. Created a allocator for Segment.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:834
@@ +833,3 @@
+ slice->setChunks(startChunkIter, endChunkIter);
+ slice->setSize(curSliceSize);
+ slice->setAlign(sliceAlign);
----------------
Michael Spencer wrote:
> This doesn't seem to include the size of the Section we just added.
curSliceSize always has the section filesize added to it.
================
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;
----------------
Michael Spencer wrote:
> `RoundUpToAlignment`
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:846-847
@@ +845,4 @@
+ sliceAlign = section->align2();
+ }
+ else {
+ if (sliceAlign < section->align2())
----------------
Michael Spencer wrote:
> No newline before else.
Changed.
================
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) {
----------------
Michael Spencer wrote:
> std::find.
added a TODO for cleanup.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:865
@@ +864,3 @@
+ if (!slice) {
+ slice = new SegmentSlice<target_endianness, is64Bits>();
+ _segmentSlices.push_back(slice);
----------------
Michael Spencer wrote:
> BumpPtrAllocator.
Changed.
================
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());
----------------
Michael Spencer wrote:
> Since this can't be a range based for loop:
>
> for (auto sei = slices_begin(), see = slices_end(); sei != see; ++sei)
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:886-888
@@ +885,5 @@
+ // which is the segment address
+ if (isFirstSegment && firstSlice) {
+ (*sei)->setStart(this->startAddr());
+ }
+ else {
----------------
Michael Spencer wrote:
> No braces needed.
Changed.
================
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());
+ }
----------------
Michael Spencer wrote:
> `RoundUpToAlignment`
Changed.
================
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);
----------------
Michael Spencer wrote:
> Don't reevaluate chunks_end() each time through the loop.
Changed.
================
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) {
----------------
Michael Spencer wrote:
> `RoundUpToAlignment`
Changed.
================
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);
----------------
Michael Spencer wrote:
> Don't evaluate {slices/chunks}_end() each time through the loop.
changed.
================
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;
+ }
+
----------------
Michael Spencer wrote:
> Not needed.
Removed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:951
@@ +950,3 @@
+
+ int pgsz() const { return _options.pageSize(); }
+
----------------
Michael Spencer wrote:
> `pageSize()`
Changed.
================
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;
----------------
Michael Spencer wrote:
> who is responsible for deleting these?
Now uses Allocator.
================
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;
+ }
+
----------------
Michael Spencer wrote:
> Not needed.
Removed.
================
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;
+ }
----------------
Michael Spencer wrote:
> This is broken. Chunk already has a Kind with the same value.
Chunk doesnot have such a type called StringTable.
================
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());
----------------
Michael Spencer wrote:
> Range based for loop.
Changed.
================
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)
+
----------------
Michael Spencer wrote:
> Style.
Changed.
================
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;
+ }
----------------
Michael Spencer wrote:
> Broken. Same issue as StringTable.
Chunk doesnot have SymbolTable enumeration.
================
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;
+ }
+
----------------
Michael Spencer wrote:
> Not needed.
Removed.
================
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:
> Michael Spencer wrote:
> > Spaces around `=`.
> Better names:
>
> atom
> sectionIndex
Changed and added 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());
----------------
Michael Spencer wrote:
> Better names please.
Used better names, binding and type.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1086
@@ +1085,3 @@
+ case DefinedAtom::typeCode:
+ // TODO: add offset
+ symbol->st_value = addr;
----------------
Michael Spencer wrote:
> Which offset?
Removed the TODO.
================
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;
+ }
----------------
Michael Spencer wrote:
> Indented one too far.
Changed.
================
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;
----------------
Michael Spencer wrote:
> std::stable_sort(_symbolTable.begin(), _symbolTable.end(),
> [](const Elf_Sym *A, const Elf_Sym *B) {
> return A->getBinding() < B->getBinding();
> });
Changed.
================
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));
----------------
Michael Spencer wrote:
> Range based for loop.
Changed.
================
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)
----------------
Michael Spencer wrote:
> Style
Changed.
================
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);
----------------
Michael Spencer wrote:
> std::fill_n
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1221
@@ +1220,3 @@
+private:
+ Elf_Ehdr _eh;
+};
----------------
Michael Spencer wrote:
> Extra whitespace.
Changed.
================
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)
+ {
+ }
----------------
Michael Spencer wrote:
> Style.
Changed.
================
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()) {
----------------
Michael Spencer wrote:
> Don't evaluate slices_end() each time through the loop.
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1242
@@ +1241,3 @@
+ ++sei) {
+ if (_phi == _ph.end()) {
+ phdr = new(_allocator.Allocate<Elf_Phdr>()) Elf_Phdr;
----------------
Michael Spencer wrote:
> Better names.
ph is the program header.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1247-1248
@@ +1246,4 @@
+ ret = true;
+ }
+ else {
+ phdr = (*_phi);
----------------
Michael Spencer wrote:
> No newline.
Changed.
================
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;
----------------
Michael Spencer wrote:
> `RoundUpToAlignment`
Changed.
================
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));
----------------
Michael Spencer wrote:
> Range based for loop.
Changed.
================
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));
----------------
Michael Spencer wrote:
> Range based for loop.
Changed.
================
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
----------------
Michael Spencer wrote:
> Please make a proper data structure.
TODO: to clean it up in the next patch by changing pair to appropriate struct
================
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,
----------------
Michael Spencer wrote:
> Same here.
TODO: to clean it up in the next patch by changing pair to appropriate struct
================
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
----------------
Michael Spencer wrote:
> ChunkIter
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1453
@@ +1452,3 @@
+ typedef std::map<StringRef, MergedSection<target_endianness, is64Bits> *>
+ mergedSectionMap_t;
+ typedef typename std::vector
----------------
Michael Spencer wrote:
> `MergedSectionMapT`
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1455
@@ -726,1 +1454,3 @@
+ typedef typename std::vector
+ <MergedSection<target_endianness, is64Bits> *>::iterator merged_section_iter;
----------------
Michael Spencer wrote:
> `MergedSectionIter`
Changed.
================
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);
+ };
----------------
Michael Spencer wrote:
> Use the hashing stuff in llvm/Support/Hashing.h
>
> Most importantly use hash_combine instead of some custom mechanism.
Changed.
================
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,
----------------
Michael Spencer wrote:
> `SectionMapT`
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1481
@@ -802,1 +1480,3 @@
+ Segment<target_endianness, is64Bits>*,
+ SegmentHashKey> segmentMap_t;
----------------
Michael Spencer wrote:
> `SegmentMapT`
Changed.
================
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) {
----------------
Michael Spencer wrote:
> Line up arguments.
Changed.
================
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;
----------------
Michael Spencer wrote:
> llvm::StringSwitch in llvm/ADT/StringSwitch.h
How do you do startswith with StringSwitch ?
================
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;
+ }
----------------
Michael Spencer wrote:
> Indented one too far.
Changed.
================
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;
+ }
----------------
Michael Spencer wrote:
> Same thing needs to happen with .bss I believe. I'm not sure what criteria ld uses to do this.
I havent seen multiple .bss.* names. ld uses a similiar mapping. Once we are close to get to work, lets bring all the map that ld uses and put it here.
================
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;
+ }
----------------
Michael Spencer wrote:
> Indented one too far.
Changed.
================
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;
+ }
----------------
Michael Spencer wrote:
> Indented one too far.
Changed.
================
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
----------------
Michael Spencer wrote:
> No newline between > and (.
Changed.
================
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> *>
----------------
Michael Spencer wrote:
> Range based for loop.
Changed.
================
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;
----------------
Michael Spencer wrote:
> No newline before {.
Changed.
================
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
----------------
Michael Spencer wrote:
> No newline before {.
Changed.
================
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
----------------
Michael Spencer wrote:
> std::stable_sort(_sections.begin(), _sections.end(),
> [](Chunk<target_endianness, is64Bits> *A, Chunk<target_endianness, is64Bits> *B) {
> return A->order() < B->order();
> });
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1676
@@ +1675,3 @@
+ int ordinal = 1;
+ for (auto msi = _mergedSections.begin(); msi != _mergedSections.end();
+ ++msi) {
----------------
Michael Spencer wrote:
> Range based for loop.
Changed.
================
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);
----------------
Michael Spencer wrote:
> Don't evaluate end_sections() each time through the loop.
Changed.
================
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) {
----------------
Michael Spencer wrote:
> Don't evaluate end condition each time through the loop.
Changed.
================
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) {
----------------
Michael Spencer wrote:
> This is never used.
Removed.
================
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;
----------------
Michael Spencer wrote:
> `assignFileOffsets`
Changed.
================
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();
+ }
----------------
Michael Spencer wrote:
> This is never used.
Removed.
================
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;
----------------
Michael Spencer wrote:
> Align arguments.
Changed.
================
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);
----------------
Michael Spencer wrote:
> Michael Spencer wrote:
> > Range based for loop.
> Why are these two loops split?
Changed.
================
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);
----------------
Michael Spencer wrote:
> Why is _segments not an array of Segment*?
Changed.
================
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);
----------------
Michael Spencer wrote:
> Range based for loop.
Changed.
================
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();
+ }
+ }
----------------
Shankar Kalpathi Easwaran wrote:
> Michael Spencer wrote:
> > Michael Spencer wrote:
> > > Range based for loop.
> > Why are these two loops split?
> Changed.
merged into one loop
================
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);
----------------
Michael Spencer wrote:
> Range based for loop.
Changed.
================
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);
----------------
Michael Spencer wrote:
> Range based for loop.
Changed.
================
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);
----------------
Michael Spencer wrote:
> Range based for loop.
Changed.
================
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);
----------------
Michael Spencer wrote:
> `RoundUpToAlignment`
Changed.
================
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()));
----------------
Michael Spencer wrote:
> Word wrap at 80 col.
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:1820-1821
@@ +1819,4 @@
+ segment->setMemSize(address - startAddress);
+ startAddress = Chunk<target_endianness, is64Bits>::alignTo(address,
+ _options.pageSize());
+ }
----------------
Michael Spencer wrote:
> `RoundUpToAlignment`
Changed
================
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);
----------------
Michael Spencer wrote:
> Range based for loop.
Changed.
================
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();
----------------
Michael Spencer wrote:
> Range based for loop.
Changed.
================
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() ) {
----------------
Michael Spencer wrote:
> void ELFExecutableWriter<target_endianness, is64Bits>
> ::buildChunks(const lld::File &file){
Changed.
================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:2163-2165
@@ -1423,2 +2162,5 @@
+
return buffer->commit();
+
+ return error_code::success();
}
----------------
Michael Spencer wrote:
> Which return?
Removed the success return
http://llvm-reviews.chandlerc.com/D222
More information about the llvm-commits
mailing list