[PATCH] Add test showing error in StreamingMemoryObject.setKnownObjectSize().

[Jan Voung]

Mostly thinking out loud, but a few suggestions in the sea of text too.


================
Comment at: include/llvm/Support/StreamingMemoryObject.h:78
@@ +77,3 @@
+      if (ObjectSize && BytesRead > ObjectSize) {
+        BytesRead = ObjectSize;
+        EOOReached = true;
----------------
Just thinking out loud here to think through what's going on... it is a bit confusing that this has to deal with an ObjectSize, and the fact that it can be different from BytesRead is just more fun =)

If ObjectSize is known, and BytesRead goes above ObjectSize, doesn't that mean that it already read beyond the ObjectSize from the perspective of the Streamer? The amount to read is just kChunkSize. This will adjust the "position" of the Streamer for its next call to GetBytes... if there is a next call. The Streamer doesn't let you seek backward, so you can no longer make the next GetBytes start at exactly the ObjectSize boundary.

So, why is this okay? My impression is that this is okay because the Streamer is owned by this class, and so it should not be shared, so adjusting the position of Streamer beyond the ObjectSize doesn't matter too much.

Okay, so what does matter?

The bounds determined by BytesRead does matter, since that affects what readBytes() will do. readBytes() does not check ObjectSize... and that's what this extra check fixes. On the other hand readBytes() calls fetchToPos, so it is in a way checking ObjectSize (just tucked away here).

================
Comment at: include/llvm/Support/StreamingMemoryObject.h:81
@@ +80,3 @@
+      } else if (bytes == 0) { // reached EOF/ran out of bytes
+        if (ObjectSize == 0)
+          ObjectSize = BytesRead;
----------------
Following up on Rafael's example, why not set ObjectSize = BytesRead here unconditionally? At this point, if ObjectSize != 0, then BytesRead <= ObjectSize.

Otherwise, if you call fetchToPos(Pos) afterwards with BytesRead < Pos < ObjectSize, then EOOReached is true and it will say that Pos is readable (since the check on line 71 is Pos < ObjectSize, instead of Pos < BytesRead).

Maybe that earlier condition on line 71 should just be Pos < BytesRead to be consistent with readBytes using "BytesRead" as the boundary at EOF, instead of ObjectSize as the boundary at EOF. It will also be consistent with the "return Pos < BytesRead;" at the end of this function on line 87.

On the other hand, getExtent() and isValidAddress() use ObjectSize, and it will be inconsistent if the stream is ever truncated to be less than the known ObjectSize, but the callers will just have to rely on what readBytes ultimately says.

================
Comment at: unittests/Support/StreamingMemoryObject.cpp:22
@@ -21,3 +21,3 @@
   }
 };
 }
----------------
How about having a second unittest class which sets a limit on how many GetBytes can be pulled. Something like a SizedNullDataStreamer class (or whatever you want to call it).

Then you can have a tests for:

(a) the size isn't known, and the end of object is solely based on what GetBytes() returns vs the requested len.

(b) the one case Rafael brought up -- when the size is "known", but something unexpected happens like the http connection dies before hitting the known size. It could also be that the wrapper lies about the size (maybe someone is trying to trigger a bug).

http://reviews.llvm.org/D8931

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/