<div class="gmail_quote">On 20 January 2012 11:55, Derek Schuff <span dir="ltr"><<a href="mailto:dschuff@google.com" target="_blank">dschuff@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
And finally, the StreamingMemoryObject implementation, modified BitcodeReader, and modifed llvm-dis.cpp using the streaming interface.<div>Please take a look</div></blockquote><div><br></div><div>Overall this looks good. I'm especially happy with some of the refactoring inside BitcodeReader! Comments:</div>
<div><br></div><div><div>--- a/include/llvm/Bitcode/ReaderWriter.h</div><div>+++ b/include/llvm/Bitcode/ReaderWriter.h</div><div>@@ -21,31 +21,41 @@ namespace llvm {</div><div> class MemoryBuffer;</div><div> class ModulePass;</div>
<div> class BitstreamWriter;</div><div>+ class DataStreamer;</div><div> class LLVMContext;</div><div> class raw_ostream;</div></div><div><br></div><div>I realize these were unsorted when you got here, but please alphabetize them.</div>
<div><br></div><div><div>+ /// If 'verify' is true, check that the file fits in the buffer.</div><div>+ static inline bool SkipBitcodeWrapperHeader(const unsigned char *&BufPtr,</div><div>+ const unsigned char *&BufEnd,</div>
<div>+ bool Verify) {</div></div><div><br></div><div>I didn't really understand the comment. I think what you're doing is disabling the check that the buffer contained the whole header. Could you make it "bool SkipPastEnd" instead?</div>
<div><br></div><div><div><div>+DataStreamer *getDataFileStreamer(const std::string &Filename,</div><div>+ std::string *Err);</div></div></div><div><br></div><div>Please line up the argument to the (.</div>
<div><br></div><div><div>--- a/include/llvm/Support/StreamableMemoryObject.h</div><div>+++ b/include/llvm/Support/StreamableMemoryObject.h</div><div>@@ -12,6 +12,9 @@</div><div> #define STREAMABLEMEMORYOBJECT_H_</div><div>
</div><div> #include "llvm/Support/MemoryObject.h"</div><div>+#include <vector></div><div>+#include "llvm/ADT/OwningPtr.h"</div><div>+#include "llvm/Support/DataStream.h"</div></div><div>
<br></div><div>ADT, Support, then <system> headers. See <a href="http://llvm.org/docs/CodingStandards.html#scf_includes" target="_blank">http://llvm.org/docs/CodingStandards.html#scf_includes</a> .</div><div><br></div>
<div><div>+/// StreamingMemoryObject - interface to data which is actually streamed from</div>
<div>+/// at DataStreamer. In addition to inherited members, it has the</div><div>+/// dropLeadingBytes and setKnownObjectSize methods which are not applicable</div><div>+/// to non-streamed objects</div><div>+class StreamingMemoryObject : public StreamableMemoryObject {</div>
</div><div><br></div><div>I think that's a full sentence missing a period. It feels *awfully weird* to have StreamableMemoryObject and StreamingMemoryObject, and both of them are interfaces. The comment doesn't seem to sufficiently explain what's going on here. (The DataStreamer can stream from a Streaming but not with a Streamable? What?)</div>
<div><br></div><div><div>+ // fetch enough bytes such that Pos can be read or EOF is reached</div><div>+ // (i.e. BytesRead > Pos). Return true if Pos can be read.</div><div>+ // Unlike most of the functions in BitcodeReader, returns true on success.</div>
<div>+ bool fetchToPos(size_t Pos) {</div></div><div><br></div><div>Comment should start with a capital.</div><div><br></div><div><div>+ bool fetchToPos(size_t Pos) {</div><div>+ if (EOFReached) return Pos < ObjectSize;</div>
<div>+ while (Pos >= BytesRead) {</div><div>+ Bytes.resize(BytesRead + kChunkSize);</div><div>+ size_t bytes = Streamer->GetBytes(&Bytes[BytesRead + BytesSkipped],</div><div>+ kChunkSize);</div>
</div><div><br></div><div>Why is kChunkSize so special? Why not ask for all the bytes up until Pos? The comment on DataStreamer::GetBytes doesn't give any reason not to ask for as many bytes as you want?</div><div><br>
</div><div><div>+bool BitcodeReader::SuspendModuleParse() {</div><div>+ // save our current position</div><div>+ NextUnreadBit = Stream.GetCurrentBitNo();</div><div>+ return false;</div><div>+}</div></div><div><br></div>
<div>What's up with that returning bool?</div><div><div><br></div><div>+ // ParseModule will parse the next body in the stream and set its</div><div>+ // position in the DeferredFunctionInfo map</div></div><div>
<br></div><div>Sentence needs period.</div><div><br></div><div><div>+ unsigned char buf[16];</div><div>+ if (Bytes->readBytes(0, 16, buf, NULL) == -1)</div><div>+ return Error("Bitcode stream must be at least 16 bytes in length");</div>
<div>+</div><div>+ if (!isBitcode(buf, buf + 16)) {</div><div>+ return Error("Invalid bitcode signature");</div><div>+ }</div></div><div><br></div><div>So, uh, braces or no braces around one-line return statements? :-)</div>
<div><br></div><div><div>+Module *llvm::getStreamedBitcodeModule(const std::string &name,</div><div>+ DataStreamer *streamer,</div><div>+ LLVMContext &Context,</div>
<div>+ std::string *ErrMsg) {</div></div><div><br></div><div>These args don't line up.</div><div><br></div><div>--- /dev/null</div><div><div>+++ b/lib/Support/DataStream.cpp</div><div>
@@ -0,0 +1,96 @@</div><div>+//===--- llvm/Support/DataStream.cpp - Lazy streamed Data -*- C++ -*-===//</div></div><div><br></div><div>Don't include emacs mode markers (the -*- C++ -*- bit) on .cpp files, only on .h files.</div>
<div><br></div><div><div>+// Very simple stream backed by a file. Mostly useful for stdin and debugging;</div><div>+// actual file access is probably still best done with mmap</div><div>+class DataFileStreamer : public DataStreamer {</div>
</div><div><div>+ int Fd;</div></div><div><br></div><div>Sentence seeking full stop.</div><div><br></div><div><div>+DataStreamer *getDataFileStreamer(const std::string &Filename,</div><div>+ std::string *StrError) {</div>
</div><div><br></div><div>Line up to the ( again.</div><div><br></div><div><div>+ if (e != success) {</div><div>+ *StrError = std::string() + "Could not open " + Filename + ": "</div><div>+ + e.message() + "\n";</div>
<div>+ return NULL;</div><div>+ }</div></div><div><br></div><div>Optional: std::string("Could not open ") + Filename, and also putting the + on the previous line instead of starting a line with the operator.</div>
<div><br></div><div><div>--- a/lib/Support/StreamableMemoryObject.cpp</div><div>+++ b/lib/Support/StreamableMemoryObject.cpp</div></div><div><br></div><div>In this file you added some spurious blank lines. Please don't do that.</div>
<div><br></div><div><div>+bool StreamingMemoryObject::isObjectEnd(uint64_t address) {</div><div>+ if (ObjectSize) return address == ObjectSize;</div><div>+ fetchToPos(address);</div><div>+ return address == BytesRead;</div>
<div>+}</div></div><div><br></div><div>Shouldn't that end with "return address == ObjectSize"? If the file is larger than 'address' bytes, won't this read up to address bytes, then stop, leaving BytesRead == address? ObjectSize on the other hand isn't set until EOF is reached.</div>
<div><br></div><div><div>+int StreamingMemoryObject::readBytes(uint64_t address,</div><div>+ uint64_t size,</div><div>+ uint8_t* buf,</div><div>+ uint64_t* copied) {</div>
</div><div><br></div><div>Misaligned.</div><div><br></div><div><div>+ //StreamableMemoryObject</div><div>+</div></div><div><br></div><div>Please remove.</div><div><br></div><div>Nick</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div>thanks,</div><div>-Derek<div><div><br><br><div class="gmail_quote">
On Wed, Jan 18, 2012 at 4:05 PM, Derek Schuff <span dir="ltr"><<a href="mailto:dschuff@google.com" target="_blank">dschuff@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Chris & Nick,<div>Attached is a very slightly updated version of patch number 1, and patch number 2, with a new StreamableMemoryObject (derived from MemoryObject), suitable for streaming usage.</div><div>One consequence of deriving from MemoryObject is that I had to make the getExtent and readByte/readBytes methods of MemoryObject no longer const, since they are definitely not const in the StreamableMemoryObject. This resulted in having to remove const in several usages of MemoryObject. It seemed less bad than adding a bunch of mutable data members in StreamableMemoryObject or creating a near-duplicate of MemoryObject.</div>
<div><br>The third patch is nearly ready as well, please let me know what you think.</div><div>thanks,</div><div>-Derek<div><div><br><br><div class="gmail_quote">On Thu, Jan 12, 2012 at 3:09 PM, Derek Schuff <span dir="ltr"><<a href="mailto:dschuff@google.com" target="_blank">dschuff@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Overall, my recommendation would be to split this into three patches: the first patch would just increase the abstraction level of the bitcode reader, by adding various predicates that you need and tidy things up. The second would extend MemoryObject as needed to add the functionality that you need. The third would actually switch the meat of the bitcode reader to be lazy-streaming, and switch a tool to use it.<br>
<font color="#888888"><br></font></blockquote><div><br></div><div><br></div></div><div>Attached is patch 1 as listed above: a refactor of BistreamCursor to use an offset rather than raw pointers, and abstract the relevant operations into functions. For now BitstreamReader is the same. I've tested it locally and it works in isolation and is hopefully ready to apply. Patch 2, a subclass of MemoryObject (StreamableMemoryObject) to replace the BitstreamBytes class here, is upcoming next.</div>
<div><br></div><div>thanks,</div><div>-Derek</div></div>
</blockquote></div><br></div></div></div>
</blockquote></div><br></div></div></div>
</blockquote></div><br>