This is an archive of the discontinued LLVM Phabricator instance.

[Support] Move llvm::MemoryBuffer to sys::fs::file_t
ClosedPublic

Authored by rnk on Jun 17 2019, 11:51 AM.

Details

Summary

On Windows, Posix integer file descriptors are a compatibility layer
over native file handles provided by the C runtime. There is a hard
limit on the maximum number of file descriptors that a process can open,
and the limit is 8192. LLD typically doesn't run into this limit because
it opens input files, maps them into memory, and then immediately closes
the file descriptor. This prevents it from running out of FDs.

For various reasons, I'd like to open handles to every input file and
keep them open during linking. That requires migrating MemoryBuffer over
to taking open native file handles instead of integer FDs.

Event Timeline

rnk created this revision.Jun 17 2019, 11:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2019, 11:51 AM
aganea added inline comments.Jun 17 2019, 2:56 PM
llvm/include/llvm/Support/FileSystem.h
938

Same question as below, why not just keep openNativeFile?

963

Why not use file_t everywhere? Do we still need POSIX FD's in user code? I find it error-prone having to juggle between the two.

llvm/lib/Support/Windows/Path.inc
738

A bit unrelated, but worth mentioning: I've noticed that status() (and thus getStatus()) came up in profiles as slower-than-they-should-be. Each call to status() issues 5 kernel calls and opens the file by the same occasion. In comparaison, MSVC uses the file metadata through the FindFirstFile/FindNextFile API, which is a lot faster. When using precompiled headers, this makes a difference because Clang calls status() on every file referenced by the PCH, whereas MSVC simply iterates through folders (for a given CPP using precompiled headers, MSVC was taking ~250 ms to compile, and Clang about ~1200 ms). Clang has some level of caching, but even at that it goes through the status() call. One of the ideas was maybe to hide a directory caching mechanism behind this status() API instead of letting it go directly to the OS layer.

1211

Just keep the API below that "works" all the time? (readNativeFileSlice)

Where do you want to keep these open? The default OS limit on open files for OSX is 256.

rnk marked 3 inline comments as done.Jun 18 2019, 9:46 AM
rnk added a subscriber: silvas.

Where do you want to keep these open? The default OS limit on open files for OSX is 256.

In COFF LLD.

By the way, that is... disturbingly low. How is this not already a huge problem? Is this why clang doesn't keep open FDs to the header files it maps? That in itself seemed like a bug to me. It invites TOCTOU issues. Users pretty routinely race with the compiler while saving files in their editors.

Right now COFF LLD tries to mmap files asynchronously or in parallel, and that adds a lot of complexity. If I remove that complexity, then I should be able to open each input once, one at a time, get the inode / unique id, close the file if already open, map it, and continue.

llvm/include/llvm/Support/FileSystem.h
963

No reason, other than that there's a lot of code to migrate. The refactoring is also made slightly more difficult by the move to use llvm::Error instead of std::error_code.

llvm/lib/Support/Windows/Path.inc
738

I think @silvas was telling me more or less the same thing based on his experience at Sony. He had a similar suggestion. At this point, I think it would be best to add the cache to the VirtualFileSystem layer, since then the cache won't be global.

1211

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.

aganea added inline comments.Jun 18 2019, 10:43 AM
llvm/include/llvm/Support/FileSystem.h
963

Do you think that's something that should be done? (not having FDs anymore)

llvm/lib/Support/Windows/Path.inc
738

FileSystemStatCache seems to do that (?) But then a new sys::fs API would be needed to go through the NTFS directory metadata instead of the current call to sys::fs::status().

1211

I meant forward to a common implementation for both readNativeFile and readNativeFileSlice

rnk updated this revision to Diff 205406.Jun 18 2019, 11:34 AM
rnk marked 2 inline comments as done.
  • common ReadFile code
rnk added inline comments.Jun 18 2019, 11:34 AM
llvm/include/llvm/Support/FileSystem.h
963

Yes.

llvm/lib/Support/Windows/Path.inc
1211

Sure, done.

In D63453#1548672, @rnk wrote:

Where do you want to keep these open? The default OS limit on open files for OSX is 256.

In COFF LLD.

FWIW, we do use lld-link to cross-compile from Darwin to Windows, and it would be awesome if that kept working :)

aganea added inline comments.Jun 18 2019, 7:28 PM
llvm/lib/Support/Windows/Path.inc
1211

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 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.

rnk marked an inline comment as done.Jun 19 2019, 12:25 PM

FWIW, we do use lld-link to cross-compile from Darwin to Windows, and it would be awesome if that kept working :)

I promise not to regress that functionality. :)

Still, I don't think that's a good reason not to do this change, although I certainly have less motivation now.

llvm/lib/Support/Windows/Path.inc
1211

The Unix implementation doesn't loop, so it can theoretically return with a short read. I think if I make this one loop, I should make the Unix one loop, and then promise the caller to block until the requested number of bytes are read or EOF is hit. However, because this API doesn't prescribe any particular kind of buffer, the caller typically has to loop anyway to allocate more memory for the next read. I did it that way to make it as compatible with read as possible for easy migration.

So, given that short reads theoretically exist on Unix, do you think the code sharing (beyond what we have already) for Windows is worth adding the inconsistency in behavior between Windows and Unix?

aganea accepted this revision.Jun 19 2019, 2:15 PM

In D63453#1550865, @rnk wrote:
Still, I don't think that's a good reason not to do this change, although I certainly have less motivation now.

I think this is a good change, and it's going into the right direction, regardless of what you plan on doing in LLD. LGTM!

llvm/lib/Support/Windows/Path.inc
1211

The API behavior should not diverge between Windows and Unix, I think we both agree on this. However I find a bit awkward to serve short reads, when we could serve complete reads, as the user requested in the first place (in the worst case, the caller expects a full read anyway for readNativeFile isn't it?). But perhaps this is OT? Let's leave it the way you did it, and discuss this later if you wish?

This revision is now accepted and ready to land.Jun 19 2019, 2:15 PM
rnk marked an inline comment as done.Jul 9 2019, 5:17 PM

I got back to work and found some time, so now I have easy access to Linux and Windows, and should be able to finish this.

llvm/lib/Support/Windows/Path.inc
1211

I guess it's OK if the Windows API guarantees that there are no short reads. If it ever becomes a problem, we can teach the Unix one to loop until EOF.

rnk marked an inline comment as done.Jul 9 2019, 5:30 PM
rnk added inline comments.
llvm/lib/Support/Windows/Path.inc
1211

I tried the loop you wrote, but it seems to interfere with the outer loop for reading from pipes until EOF. I think we should stick with this for now.

This revision was automatically updated to reflect the committed changes.
labath added a subscriber: labath.Aug 16 2019, 6:18 AM
labath added inline comments.
llvm/trunk/lib/Support/Windows/Path.inc
1258–1262 ↗(On Diff #208854)

BTW, I've was looking at this code in context of D66344, and I have found this quirk of this API to be very surprising and unexpected. Zeroing the remainder of the buffer may be the right behavior for MemoryBuffer::getFileSlice (though I'm not even convinced of that), but I definitely wouldn't expect that to happen in readNativeFileSlice. I think that an additional size_t *BytesRead read argument would be more natural here (and consistent with readNativeFile).

What would you say to a patch which adds a BytesRead argument to this function and moves the zeroing code to the MemoryBuffer class (currently the only user of this function)?

aganea added inline comments.Aug 16 2019, 7:56 AM
llvm/trunk/lib/Support/Windows/Path.inc
1258–1262 ↗(On Diff #208854)

Sounds good to me! ErrorOr<size_t> readNativeFileSlice(..) ? (and same thing for readNativeFile ?)

labath added inline comments.Aug 20 2019, 5:14 AM
llvm/trunk/lib/Support/Windows/Path.inc
1258–1262 ↗(On Diff #208854)

Cool. I've created D66471 for that. I've went for Expected<size_t> instead of ErrorOr, as (IIUC) the plan is to eventually get rid of ErrorOr..