[PATCH] [PECOFF][Writer] Emit .reloc section.

Sean Silva silvas at purdue.edu
Mon Jul 15 13:20:48 PDT 2013



================
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:
----------------
Richard wrote:
> 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.
We don't really have a set coding convention for this AFAIK. Putting private stuff first is fine.

================
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));
----------------
Richard wrote:
> 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?
I also find this proliferation of magic constants to be a bit troubling.

================
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;
----------------
Richard wrote:
> offsets is not modified; pass by const reference
The assignments via the reinterpret_cast<> are modifying it. As you point out though, all those reinterpret_cast<>'s suggest that there is a safer way to do all this.

================
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:652
@@ +651,3 @@
+    // The first four bytes is the page RVA.
+    *reinterpret_cast<llvm::support::ulittle32_t *>(&contents[0]) = pageAddr;
+
----------------
Do you know that the alignment is correct here? It's only a vector of uint8_t.

================
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)
----------------
Richard wrote:
> Can dyn_cast<T> return nullptr?
Good catch. If you know it is a DefinedAtom, then just use cast<>. Also, you should be able to just do dyn_cast<DefinedAtom> since I believe returned result is always const.


http://llvm-reviews.chandlerc.com/D1126

COMMIT
  http://llvm-reviews.chandlerc.com/rL186336



More information about the llvm-commits mailing list