[PATCH] D66471: [Support] Improve readNativeFile(Slice) interface

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Seems reasonable aside from the bug. :)



================
Comment at: lib/Support/MemoryBuffer.cpp:467
+    Expected<size_t> ReadBytes =
+        sys::fs::readNativeFileSlice(FD, Buf->getBuffer(), Offset);
+    if (!ReadBytes)
----------------
labath wrote:
> aganea wrote:
> > labath wrote:
> > > aganea wrote:
> > > > labath wrote:
> > > > > aganea wrote:
> > > > > > Shouldn't we use `ToRead` here instead of `Buf->getBuffer()`? Could you please test two sequential reads in the unittests?
> > > > > Woops, indeed it should. Thanks for catching that!
> > > > > 
> > > > > As for the test, I am not sure how I could force the read syscall to return partial data here. For the non-slice version, I suppose I could make this read from a pipe, and then make sure I write to the pipe very slowly. However, the slice version requires a seekable fd, and I don't know what to do there... Do you have any suggestion?
> > > > You can probably call `MemoryBuffer::getOpenFile` with a smaller `FileSize` & `MapSize` and `IsVolatile = true`? And then a subsequent call by setting `Offset` > 0? Would that work?
> > > I don't fully understand what you mean, but I don't see how could that work. Are you expecting that the second time around the OS will return the partial read first, because that data will already be cached and ready? If so, then I would be very surprised if that works, particularly for a file that has just been created because it will likely be present in the disk cache in its entirety.
> > You're right. You would need to test it from the shell with something like: `cat foo | pv --quiet --line-mode --rate-limit 1 | clang -emit-llvm -o -`. I don't know if `pv` is available everywhere, or if there's a better solution to slow down `cat`? What do you think?
> Ok, I think I am starting to understand what you mean. There's no problem with slowing down the input for a pipe. I can do that from a unit test relatively easily. The problem is that that the the pipe is not seekable (at least on unix OSs, I don't know what would windows do here). The reason that `clang -` works is because it invokes a different `MemoryBuffer` function, which is implemented in terms of `readNativeFile` and so it works fine with a non-seekable fd. However, calling `readNativeFileSlice` with a non-seekable fd would result in an error. And unfortunately, I don't know if there's a way to create a seekable fd, which will not satisfy all read requests immediately (short of some FUSE mounts or similarly heroic efforts).
Right, testing a short read from a seekable file is... very hard. IIRC Linux goes out of its way to avoid surfacing short reads if it can. One way to do it would be to make a 6GB file on Windows, since it has the 2GB ReadFile limitation. Of course, I was happy to commit my code without such a test, so I can only apply the same standard to you. :)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66471/new/

https://reviews.llvm.org/D66471