[PATCH] [PECOFF][Writer] Emit .reloc section.
Richard
legalize at xmission.com
Mon Jul 15 13:09:14 PDT 2013
This is my first review comments for llvm, hopefully I'm not getting in the way.
================
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:594-596
@@ +593,5 @@
+class BaseRelocChunk : public SectionChunk {
+ typedef std::vector<std::unique_ptr<Chunk>> ChunkVectorT;
+ typedef std::map<uint64_t, std::vector<uint16_t>> PageOffsetT;
+
+public:
----------------
Is this the convention in this code base to put private typedefs at the opening of a class? I always found it odd to read private implementation details of a class as the first thing in it's declaration.
================
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:607-608
@@ +606,4 @@
+ /// By dividing 32 bit RVAs into blocks, COFF saves disk and memory space for
+ /// the base relocation. A block consists with a 32 bit page RVA and 16 bit
+ /// relocation entries which represent offsets in the page. That is a compact
+ /// represetation than a simple vector of 32 bit RVAs.
----------------
"A block consists of a 32 bit page RVA and ..."
"That is a more compact representation than..."
================
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:615
@@ +614,3 @@
+ uint64_t pageAddr = i.first;
+ std::vector<uint16_t> offsetsInPage = i.second;
+ appendAtom(createBaseRelocBlock(_file, pageAddr, offsetsInPage));
----------------
Does this need to be a copy or can it be a const reference?
================
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:647
@@ +646,3 @@
+ DefinedAtom *createBaseRelocBlock(const File &file, uint64_t pageAddr,
+ std::vector<uint16_t> &offsets) {
+ uint32_t size = 8 + offsets.size() * 2;
----------------
offsets is not modified; pass by const reference
================
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:649-665
@@ +648,19 @@
+ uint32_t size = 8 + offsets.size() * 2;
+ std::vector<uint8_t> contents(size);
+
+ // The first four bytes is the page RVA.
+ *reinterpret_cast<llvm::support::ulittle32_t *>(&contents[0]) = pageAddr;
+
+ // The second four bytes is the size of the block, including the the page
+ // RVA and this size field.
+ *reinterpret_cast<llvm::support::ulittle32_t *>(&contents[4]) = size;
+
+ // The rest of the block consists of offsets in the page.
+ size_t i = 8;
+ for (uint16_t offset : offsets) {
+ assert(offset < PAGE_SIZE);
+ uint16_t val = (llvm::COFF::IMAGE_REL_BASED_HIGHLOW << 12) | offset;
+ *reinterpret_cast<llvm::support::ulittle16_t *>(&contents[i]) = val;
+ i += 2;
+ }
+ return new (_alloc) BaseRelocAtom(file, std::move(contents));
----------------
yuck, seems like we should have a better facility for building up these COFF chunks. This seems like something that is going to appear in all the code that builds COFF chunks and all these 4, 8, += 2, type magic numbers are easy to get wrong. Does something exist that encapsulates these details?
================
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:319
@@ +318,3 @@
+ for (const auto *layout : _atomLayouts) {
+ const DefinedAtom *atom = dyn_cast<const DefinedAtom>(layout->_atom);
+ for (const Reference *ref : *atom)
----------------
Can dyn_cast<T> return nullptr?
================
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:675-676
@@ -542,4 +674,4 @@
class ExecutableWriter : public Writer {
private:
/// Apply relocations to the output file buffer. This two pass. In the first
----------------
Can we move all the private members after the public members?
http://llvm-reviews.chandlerc.com/D1126
COMMIT
http://llvm-reviews.chandlerc.com/rL186336
More information about the llvm-commits
mailing list