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

[Eli Bendersky]

On Mon, Jul 15, 2013 at 1:09 PM, Richard <legalize at xmission.com> wrote:

>
>   This is my first review comments for llvm, hopefully I'm not getting in
> the way.
>
>
FWIW, more code reviewers are very welcome in LLVM-land. Thanks for
contributing, Richard.

Eli




>
> ================
> 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
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130715/b58224eb/attachment.html>