[PATCH] D63453: [Support] Move llvm::MemoryBuffer to sys::fs::file_t

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added inline comments.


================
Comment at: llvm/lib/Support/Windows/Path.inc:1211
+
+std::error_code readNativeFile(file_t FileHandle, MutableArrayRef<char> Buf,
+                               size_t *BytesRead) {
----------------
rnk wrote:
> aganea wrote:
> > rnk wrote:
> > > aganea wrote:
> > > > Just keep the API below that "works" all the time? (`readNativeFileSlice`)
> > > Are you suggesting simplifying the code by forwarding the call with offset 0, or reducing the API to one entry point?
> > > 
> > > Stream devices like pipes don't support seeking or pread, so to change the public API, I would need to make the offset optional, which I think would be less clear. Perhaps I can simplify this Windows specific code to delegate to the slice version, but that was my concern.
> > I meant forward to a common implementation for both `readNativeFile` and `readNativeFileSlice`
> Sure, done.
Sorry for not being clearer. I was thinking more along the lines of:
```
static std::error_code readNativeFileImpl(file_t FileHandle,
                                          MutableArrayRef<char> Buf,
                                          size_t *BytesRead, size_t Offset) {
  char *BufPtr = Buf.data();
  size_t BytesLeft = Buf.size();

  if (BytesRead)
    *BytesRead = 0;

  while (BytesLeft) {
    uint64_t CurOff = Buf.size() - BytesLeft + Offset;
    OVERLAPPED Overlapped{};
    Overlapped.Offset = uint32_t(CurOff);
    Overlapped.OffsetHigh = uint32_t(uint64_t(CurOff) >> 32);

    // ReadFile can only read 2GB at a time.
    DWORD BytesToRead32 = std::min(1 << 31, BytesToRead);
    DWORD BytesRead32 = 0;
    bool Success =
        ::ReadFile(FileHandle, BufPtr, BytesToRead32, &BytesRead32, Overlap);
    if (!Success) {
      DWORD Err = ::GetLastError();
      // Pipe EOF is not an error.
      if (Err != ERROR_BROKEN_PIPE)
        return mapWindowsError(Err);
    }
    if (BytesRead)
      *BytesRead += BytesRead32;

    // Once we reach EOF, zero the remaining bytes in the buffer.
    if (BytesRead32 == 0) {
      memset(BufPtr, 0, BytesLeft);
      break;
    }
    BytesLeft -= BytesRead32;
    BufPtr += BytesRead32;
  }
  return std::error_code();
}

std::error_code readNativeFile(file_t FileHandle, MutableArrayRef<char> Buf,
                               size_t *BytesRead) {
  return readNativeFileImpl(FileHandle, Buf, BytesRead, /*Offset*/ 0);
}

std::error_code readNativeFileSlice(file_t FileHandle,
                                    MutableArrayRef<char> Buf, size_t Offset) {
  return readNativeFileImpl(FileHandle, Buf, /*BytesRead*/ nullptr, Offset);
}
```

The [[ https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-readfile | doc ]] says:
```
For an hFile that supports byte offsets, if you use this parameter you must specify a byte offset at which to start reading from the file or device. This offset is specified by setting the Offset and OffsetHigh members of the OVERLAPPED structure. For an hFile that does not support byte offsets, Offset and OffsetHigh are ignored.
```
So I think we can use OVERLAPPED with pipes as long as we don't write anything in there.
It'd be interesting to also test that 2 GB read limit :-) My whole point was, if that limit still exists, it should not leak outside of this implementation. Meaning that users should not have to loop to read files larger than 2 GB.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63453