<div style="font-size:13.1999998092651px;line-height:20px">Hi Rafael,</div><div style="font-size:13.1999998092651px;line-height:20px"><br></div><div style="font-size:13.1999998092651px;line-height:20px">We will try out your patch and check to see how it will fit.</div><div style="font-size:13.1999998092651px;line-height:20px"><br></div><div style="font-size:13.1999998092651px;line-height:20px">You also talked about "<span style="font-size:13.1999998092651px;line-height:19.7999992370605px">It might be even possible to drop the requirement for the size to be </span><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">known: Replace the call to AtEndOfStream by just trying to read more </span><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">and checking if it failed, but that is a bit more than I wanted to do </span><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">for this."</span></div><div style="font-size:13.1999998092651px;line-height:20px"><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div><div style="font-size:13.1999998092651px;line-height:20px"><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">That is to remove some calls to getSize()? Is there any expectation that getPointer() will always a pointer within a contiguous region of memory? That is, getPointer() is non-virtual and always refers to the Data field, but the overriding implementation could still dynamically grow some buffer and change Data to point to a new buffer (could that be "protected" instead?)?</span></div><div style="font-size:13.1999998092651px;line-height:20px"><br></div><div style="font-size:13.1999998092651px;line-height:20px">*) We do have a fork of the bitcode reader and bitstream reader. That said, we<span style="font-size:13.1999998092651px"> still use the upstream bitcode reader / bitstream reader in the browser w/ PNaCl. This is used for debugging non-guaranteed-to-be-stable temporary copies of apps in the browser (</span><a href="https://developer.chrome.com/native-client/devguide/devcycle/debugging#debugging-pnacl-pexes-pepper-35-or-later" target="_blank">https://developer.chrome.com/native-client/devguide/devcycle/debugging#debugging-pnacl-pexes-pepper-35-or-later</a>). <span style="font-size:13.1999998092651px">That said, the overlapped compile/download is probably not a big deal for the debugging use case.</span></div><div style="font-size:13.1999998092651px;line-height:20px"><br></div><div style="font-size:13.1999998092651px;line-height:20px"><div style="font-size:13.1999998092651px"><span style="font-size:13.1999998092651px">*) It looks like gzip encoded files will have content-length set to the size of the *gzipped* body, not the size of the original bitcode: </span><a href="http://stackoverflow.com/questions/3819280/content-length-when-using-http-compression" target="_blank">http://stackoverflow.com/questions/3819280/content-length-when-using-http-compression</a>. So we'll need to be careful and treat that as "unknown", which is a bit unfortunate because gzip helps reduce bandwidth requirements.</div><div><br></div></div><div style="font-size:13.1999998092651px;line-height:20px"><span style="font-size:13.1999998092651px">*) Otherwise, not all HTTP responses include a content-length, for various reasons, but I don't know how common this is, but I can try to ask chromium-dev or some other mailing list and see how common that is.</span></div><div style="font-size:13.1999998092651px;line-height:20px"><br></div><div style="font-size:13.1999998092651px;line-height:20px"><span style="font-size:13.1999998092651px">*) The HTTP response can lie, or the server can be buggy, and give the wrong content-length. I think we can try to do the safe thing here, but will have to be careful.</span></div><div style="font-size:13.1999998092651px;line-height:20px"><span style="font-size:13.1999998092651px"><br></span></div><div style="font-size:13.1999998092651px;line-height:20px"><br></div><div style="font-size:13.1999998092651px;line-height:20px">=====</div><div style="font-size:13.1999998092651px;line-height:20px"><br></div><div style="font-size:13.1999998092651px;line-height:20px">Misc review comments:</div><div style="font-size:13.1999998092651px;line-height:20px"><br></div><div style="font-size:13.1999998092651px;line-height:20px"><div> class DataStreamer {</div><div>-public:</div><div>- /// Fetch bytes [start-end) from the stream, and write them to the</div><div>- /// buffer pointed to by buf. Returns the number of bytes actually written.</div><div>- virtual size_t GetBytes(unsigned char *buf, size_t len) = 0;</div><div>+ uint64_t Size;</div><div>+ const uint8_t *Data;</div><div>+</div><div>+ virtual bool advanceToPosImpl(uint64_t Pos) = 0;</div><div> </div><div>+public:</div><div>+ const uint8_t *getPointer(uint64_t Pos, unsigned Size);</div><div>+ DataStreamer();</div><div> virtual ~DataStreamer();</div><div>-};</div></div><div style="font-size:13.1999998092651px;line-height:20px"><div> </div><div>-DataStreamer *getDataFileStreamer(const std::string &Filename,</div><div>- std::string *Err);</div><div>+ uint64_t getSize() const { return Size; }</div><div>+ void setSize(uint64_t Val) { Size = Val; }</div><div>+</div><div>+protected:</div><div>+ // It is valid to access Data[0..MaxPos)</div><div>+ uint64_t MaxPos;</div><div> </div><div>+ void init(uint64_t Size, const uint8_t *Data);</div><div>+};</div><div> }</div></div><div style="font-size:13.1999998092651px;line-height:20px"><br></div><div style="font-size:13.1999998092651px;line-height:20px">Would be nice to document why there is an advanceToPosImpl, etc. Otherwise the patch is removing a bunch of code/comment that refer to the streaming use case, so if that use case is no longer known, then I imagine folks would try to simplify things further =)</div><div style="font-size:13.1999998092651px;line-height:20px"><br></div><div style="font-size:13.1999998092651px;line-height:20px"><div>- // At this point, if there are any function bodies, the current bit is</div><div>- // pointing to the END_BLOCK record after them. Now make sure the rest</div><div>- // of the bits in the module have been read.</div><div>- if (NextUnreadBit)</div><div>- ParseModule(true);</div><div><span style="font-size:13.1999998092651px"><br></span></div><div><span style="font-size:13.1999998092651px">Hmm, didn't read too closely, but just looks different from what it used to do...</span></div></div><div style="font-size:13.1999998092651px;line-height:20px"><span style="font-size:13.1999998092651px"><br></span></div><div style="font-size:13.1999998092651px;line-height:20px"><div>+ const uint8_t *OrigBufPtr = BufPtr;</div><div> // If we have a wrapper header, parse it and ignore the non-bc file contents.</div><div> // The magic number is 0x0B17C0DE stored in little endian.</div><div> if (isBitcodeWrapper(BufPtr, BufEnd))</div><div> if (SkipBitcodeWrapperHeader(BufPtr, BufEnd, true))</div><div> return Error(BitcodeError::InvalidBitcodeWrapperHeader);</div><div> //...</div></div><div style="font-size:13.1999998092651px;line-height:20px"><div>- if (isBitcodeWrapper(buf, buf + 4)) {</div><div>- const unsigned char *bitcodeStart = buf;</div><div>- const unsigned char *bitcodeEnd = buf + 16;</div><div>- SkipBitcodeWrapperHeader(bitcodeStart, bitcodeEnd, false);</div><div>- Bytes.dropLeadingBytes(bitcodeStart - buf);</div><div>- Bytes.setKnownObjectSize(bitcodeEnd - bitcodeStart);</div><div>- }</div><div>+ uint64_t Skip = BufPtr - OrigBufPtr;</div><div>+ uint64_t Size = BufEnd - BufPtr + Skip;</div><div>+ StreamFile->setSize(Size);</div><div>+ Stream.JumpToBit(Skip * 8);</div></div><div style="font-size:13.1999998092651px;line-height:20px"><br></div><div style="font-size:13.1999998092651px;line-height:20px">Not quite clear to me that this is the same. Do we have a test of llvm-dis w/ files that have a bitcode wrapper + misc data surrounding the true bitcode contents?</div><div style="font-size:13.1999998092651px;line-height:20px"><br></div><div style="font-size:13.1999998092651px;line-height:20px">The old dropLeadingBytes would track how many bytes were skipped so that later, reading address N would mean reading N + BytesSkipped. It also subtracted the skipped bytes from BytesRead, and I haven't looked closely enough to see if there is similar state being tracked anymore.</div><div style="font-size:13.1999998092651px;line-height:20px"><div><br></div><div><div>@@ -218,18 +214,13 @@ public:</div><div> void freeState();</div><div> </div></div><div> bool canSkipToPos(size_t pos) const {</div></div><div style="font-size:13.1999998092651px;line-height:20px"><span style="font-size:13.1999998092651px">- // pos can be skipped to if it is a valid address or one byte past the end.</span><br></div><div style="font-size:13.1999998092651px;line-height:20px"><div>- return pos == 0 || BitStream->getBitcodeBytes().isValidAddress(</div><div>- static_cast<uint64_t>(pos - 1));</div><div>+ size_t Size = BitStream->getSize();</div><div>+ return pos < Size;</div><div> }</div></div><div style="font-size:13.1999998092651px;line-height:20px"><br></div><div style="font-size:13.1999998092651px;line-height:20px">Is the "pos can be skipped ... or one byte past the end" behavior still needed, or was it some artifact of how isValidAddress worked?</div><div style="font-size:13.1999998092651px;line-height:20px"><br></div><div style="font-size:13.1999998092651px;line-height:20px"><br></div><br><div class="gmail_quote">On Fri Dec 19 2014 at 1:03:30 PM Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 19 December 2014 at 15:59, JF Bastien <<a href="mailto:jfb@chromium.org" target="_blank">jfb@chromium.org</a>> wrote:<br>
> Hi Rafael,<br>
><br>
> Would you mind waiting for Derek to come back from vacation to discuss this?<br>
> We do use this code and could improve how it's used and tested within LLVM.<br>
> Derek is the best person to discuss this, he'll be back in mid-January.<br>
<br>
What about the known-size patch? Can you check if that would work for you?<br>
<br>
The issue is not so much having it tested. It is having a sane and<br>
easy to understand interface.<br>
<br>
Cheers,<br>
Rafael<br>
</blockquote></div>