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

[Rui Ueyama]

================
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));
----------------
Richard wrote:
> Does this need to be a copy or can it be a const reference?
It can 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;
----------------
Sean Silva wrote:
> 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.
It looks to me that offset can be passed by const reference as Richard pointed out.

================
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));
----------------
Sean Silva wrote:
> 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.
There are structs defined in LLVM's include/llvm/Support/COFF.h, and I'm using them wherever possible. In this case, these fields are packed consecutively, so we can remove the magic numbers by replacing tehm with sizeof() of actual data.

================
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)
----------------
Sean Silva wrote:
> 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.
We are sure that all the atoms are defined atoms here. I'd like to continue using dyn_cast with null-checking assert() for safety.

================
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;
+
----------------
Sean Silva wrote:
> Do you know that the alignment is correct here? It's only a vector of uint8_t.
Good point! The spec does not say anything about the alignment, and it looks like Windows does not enforce any alignment of the .reloc section. However, the regular binary seems to have an optional empty field at the end of a block to align the block at 32-bit boundary.

================
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
----------------
Richard wrote:
> Can we move all the private members after the public members?
Will do in another patch.


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

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