<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jul 15, 2013 at 1:09 PM, Richard <span dir="ltr"><<a href="mailto:legalize@xmission.com" target="_blank">legalize@xmission.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
This is my first review comments for llvm, hopefully I'm not getting in the way.<br>
<br></blockquote><div><br></div><div>FWIW, more code reviewers are very welcome in LLVM-land. Thanks for contributing, Richard.</div><div><br></div><div>Eli</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:594-596<br>
@@ +593,5 @@<br>
+class BaseRelocChunk : public SectionChunk {<br>
+ typedef std::vector<std::unique_ptr<Chunk>> ChunkVectorT;<br>
+ typedef std::map<uint64_t, std::vector<uint16_t>> PageOffsetT;<br>
+<br>
+public:<br>
----------------<br>
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.<br>
<br>
================<br>
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:607-608<br>
@@ +606,4 @@<br>
+ /// By dividing 32 bit RVAs into blocks, COFF saves disk and memory space for<br>
+ /// the base relocation. A block consists with a 32 bit page RVA and 16 bit<br>
+ /// relocation entries which represent offsets in the page. That is a compact<br>
+ /// represetation than a simple vector of 32 bit RVAs.<br>
----------------<br>
"A block consists of a 32 bit page RVA and ..."<br>
<br>
"That is a more compact representation than..."<br>
<br>
================<br>
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:615<br>
@@ +614,3 @@<br>
+ uint64_t pageAddr = i.first;<br>
+ std::vector<uint16_t> offsetsInPage = i.second;<br>
+ appendAtom(createBaseRelocBlock(_file, pageAddr, offsetsInPage));<br>
----------------<br>
Does this need to be a copy or can it be a const reference?<br>
<br>
================<br>
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:647<br>
@@ +646,3 @@<br>
+ DefinedAtom *createBaseRelocBlock(const File &file, uint64_t pageAddr,<br>
+ std::vector<uint16_t> &offsets) {<br>
+ uint32_t size = 8 + offsets.size() * 2;<br>
----------------<br>
offsets is not modified; pass by const reference<br>
<br>
================<br>
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:649-665<br>
@@ +648,19 @@<br>
+ uint32_t size = 8 + offsets.size() * 2;<br>
+ std::vector<uint8_t> contents(size);<br>
+<br>
+ // The first four bytes is the page RVA.<br>
+ *reinterpret_cast<llvm::support::ulittle32_t *>(&contents[0]) = pageAddr;<br>
+<br>
+ // The second four bytes is the size of the block, including the the page<br>
+ // RVA and this size field.<br>
+ *reinterpret_cast<llvm::support::ulittle32_t *>(&contents[4]) = size;<br>
+<br>
+ // The rest of the block consists of offsets in the page.<br>
+ size_t i = 8;<br>
+ for (uint16_t offset : offsets) {<br>
+ assert(offset < PAGE_SIZE);<br>
+ uint16_t val = (llvm::COFF::IMAGE_REL_BASED_HIGHLOW << 12) | offset;<br>
+ *reinterpret_cast<llvm::support::ulittle16_t *>(&contents[i]) = val;<br>
+ i += 2;<br>
+ }<br>
+ return new (_alloc) BaseRelocAtom(file, std::move(contents));<br>
----------------<br>
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?<br>
<br>
================<br>
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:319<br>
@@ +318,3 @@<br>
+ for (const auto *layout : _atomLayouts) {<br>
+ const DefinedAtom *atom = dyn_cast<const DefinedAtom>(layout->_atom);<br>
+ for (const Reference *ref : *atom)<br>
----------------<br>
Can dyn_cast<T> return nullptr?<br>
<br>
================<br>
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:675-676<br>
@@ -542,4 +674,4 @@<br>
<br>
class ExecutableWriter : public Writer {<br>
private:<br>
/// Apply relocations to the output file buffer. This two pass. In the first<br>
----------------<br>
Can we move all the private members after the public members?<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D1126" target="_blank">http://llvm-reviews.chandlerc.com/D1126</a><br>
<br>
COMMIT<br>
<a href="http://llvm-reviews.chandlerc.com/rL186336" target="_blank">http://llvm-reviews.chandlerc.com/rL186336</a><br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>