Filesystem/Windows: fix inconsistency in readNativeFileSlice API

[labath]

Summary

The windows version implementation of readNativeFileSlice, was trying to
match the POSIX behavior of not treating EOF as an error, but it was
only handling the case of reading from a pipe. Attempting to read past
the end of a regular file returns a slightly different error code, which
needs to be handled too. This patch adds ERROR_HANDLE_EOF to the list of
error codes to be treated as an end of file, and adds some unit tests
for the API.

This issue was found while attempting to land D66224, which caused a bunch of
lldb tests to start failing on windows.

[labath]

• change the test memset to a different value to properly test the zeroing aspect of the API

[aganea]

lib/Support/Windows/Path.inc

1233

Zachary also handles ERROR_OPERATION_ABORTED in lldb/source/Host/windows/ConnectionGenericFileWindows.cpp which corresponds to the user hitting Ctrl-C.

Also small nit not related to your change: we can't discriminate between a "full success" and a "eof success" from outside of this API, which could cause an extra loop to execute below? (and possibly trigger a different return code in readNativeFileSlice)

[rnk]

lib/Support/Windows/Path.inc

1233

Handling ctrl C seems reasonable to me, but it's not unit testable, so if you want to do it separately and manually test it, that's fine with me too.

I guess the concern with the loop for slice reading is that successive calls to ReadFile may not return the same error code. Right now, the loop is basically dead code since no test reads a slice of more than 4GB of data so far as I know, so it's hard to know what happens. Prior to my changes, I think the loop structure was exactly the same, though: it called ::read until it got back EOF or zero bytes read. We could check what read does in the CRT to see what we should be doing.

[aganea]

lib/Support/Windows/Path.inc

1233

CRT ::read is actually a wrapper around ReadFile. It returns 0 if it's at EOF or if GetLastError() == ERROR_BROKEN_PIPE; -1 if there's any other error; or the actual bytes read. Apparently, the doc states that ReadFile should return TRUE and bytesRead == 0 if reaching the end of file, and ERROR_HANDLE_EOF is reserved only for async/overlapped IO which we don't use here (setting Overlapped.Offset and Overlapped.OffsetHigh is not considered as overlapped IO), so I'm a bit surprised by its usage in this patch, but it's not the first time the behavior is different from what the doc states.
@labath Pavel, on which version/build of Windows are you testing? I'll give a try on mine to see if the return value from ReadFile is the same.

[aganea]

lib/Support/Windows/Path.inc

1233

Ah I see, the first read past the end of the file returns "Success" but with less bytesRead than asked for. And then any subsequent call with an offset larger that the length of file would return ERROR_HANDLE_EOF. At the expense of an extra trip to kernel space, which could be slow, considering recent mitigations and CFG (unrelated to this patch, but it'd worth the mention):

[aganea]

LGTM!

lib/Support/Windows/Path.inc

1233

Sorry for the spam. On a afterthought I think we should leave ERROR_OPERATION_ABORTED as an error. Otherwise the input stream will be broken and we might see an (unrelated) error later down the line. It maybe makes sense for LLDB.

As for my last post, ReadFile can be used in a different ways for files and pipes if we want to be optimal; but at this point we don't know so we should leave it the way it is.

[labath]

lib/Support/Windows/Path.inc

1233

No worries. Thanks for investigating this and for the pointers. I'm not very familiar with windows, so this was actually a very interesting and enlightening read for me.

lib/Support/Windows/Path.inc

std::error_code readNativeFileImpl(file_t FileHandle, char *BufPtr, size_t BytesToRead,
   DWORD BytesToRead32 =
       std::min(size_t(std::numeric_limits<DWORD>::max()), BytesToRead);
   DWORD BytesRead32 = 0;
   bool Success =
       ::ReadFile(FileHandle, BufPtr, BytesToRead32, &BytesRead32, Overlap);
   *BytesRead = BytesRead32;
   if (!Success) {
     DWORD Err = ::GetLastError();
-    // Pipe EOF is not an error.
-    if (Err == ERROR_BROKEN_PIPE)
+    // EOF is not an error.
+    if (Err == ERROR_BROKEN_PIPE || Err == ERROR_HANDLE_EOF)
       return std::error_code();
     return mapWindowsError(Err);
   }
   return std::error_code();
 }
 
 std::error_code readNativeFile(file_t FileHandle, MutableArrayRef<char> Buf,
                                size_t *BytesRead) {

unittests/Support/Path.cpp

 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/raw_ostream.h"
-#include "gtest/gtest.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
+#include "gtest/gtest.h"
 
 #ifdef _WIN32
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/Chrono.h"
 #include <windows.h>
 #include <winerror.h>
 #endif
 
TEST_F(FileSystemTest, ReadWriteFileCanReadOrWrite) {
   int FD;
   ASSERT_NO_ERROR(fs::openFileForReadWrite(NonExistantFile, FD,
                                            fs::CD_OpenExisting, fs::OF_None));
   FileDescriptorCloser Closer(FD);
   verifyRead(FD, "Fizz", true);
   verifyWrite(FD, "Buzz", true);
 }
 
+TEST_F(FileSystemTest, readNativeFileSlice) {
+  char Data[10] = {'0', '1', '2', '3', '4', 0, 0, 0, 0, 0};
+  createFileWithData(NonExistantFile, false, fs::CD_CreateNew,
+                     StringRef(Data, 5));
+  FileRemover Cleanup(NonExistantFile);
+  Expected<fs::file_t> FD = fs::openNativeFileForRead(NonExistantFile);
+  ASSERT_THAT_EXPECTED(FD, Succeeded());
+  char Buf[10];
+  const auto &Read = [&](size_t Offset,
+                         size_t ToRead) -> Expected<ArrayRef<char>> {
+    std::memset(Buf, 0, sizeof(Buf));
+    if (std::error_code EC = fs::readNativeFileSlice(
+            *FD, makeMutableArrayRef(Buf, ToRead), Offset))
+      return errorCodeToError(EC);
+    return makeArrayRef(Buf, ToRead);
+  };
+  EXPECT_THAT_EXPECTED(Read(0, 5), HasValue(makeArrayRef(Data + 0, 5)));
+  EXPECT_THAT_EXPECTED(Read(0, 3), HasValue(makeArrayRef(Data + 0, 3)));
+  EXPECT_THAT_EXPECTED(Read(2, 3), HasValue(makeArrayRef(Data + 2, 3)));
+  EXPECT_THAT_EXPECTED(Read(0, 6), HasValue(makeArrayRef(Data + 0, 6)));
+  EXPECT_THAT_EXPECTED(Read(2, 6), HasValue(makeArrayRef(Data + 2, 6)));
+  EXPECT_THAT_EXPECTED(Read(5, 5), HasValue(makeArrayRef(Data + 5, 5)));
+}
+
 TEST_F(FileSystemTest, is_local) {
   bool TestDirectoryIsLocal;
   ASSERT_NO_ERROR(fs::is_local(TestDirectory, TestDirectoryIsLocal));
   EXPECT_EQ(TestDirectoryIsLocal, fs::is_local(TestDirectory));
 
   int FD;
   SmallString<128> TempPath;
   ASSERT_NO_ERROR(