[PATCH] D27233: [libFuzzer] Diff 3 - Split IO implementation for Windows and Posix.

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

The Windows APIs have lots of gotchas.



================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:28
+bool IsFile(const std::string &Path) {
+  DWORD Att = GetFileAttributesA(Path.c_str());
+
----------------
This is OK as long as you know the path names will contain only characters in the user's code page (which isn't UTF-8).

If the path name being passed in is UTF-8, then you'll need to convert it to a `std::wstring` and call the -W versions instead of the -A versions of these file functions.

The -W versions also make it possible to deal with very long path names.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:83
+  // Get the first directory entry.
+  WIN32_FIND_DATAA FirstFind;
+  HANDLE FindHandle(FindFirstFileA(Path.c_str(), &FirstFind));
----------------
`FirstFind` doesn't seem like a great name, since it's also used for the subsequent files.  `FindInfo` or something like that is probably better.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:100
+
+    DWORD Att = GetFileAttributesA(FileName.c_str());
+
----------------
You don't need to call GetFileAttributesA.  The attributes are already available in the WIN32_FIND_DATA.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:102
+
+    if (Att == INVALID_FILE_ATTRIBUTES) {
+      Printf("GetFileAttributesA() failed for \"%s\" (Error code: %lu).\n",
----------------
If you use the attributes you already have in the WIN32_FIND_DATA, then you don't need code here to handle an error from GetFileAttributesA.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:109
+
+    if (Att & FILE_ATTRIBUTE_DIRECTORY)
+      ListFilesInDirRecursive(FileName, Epoch, V, false);
----------------
You could put the test for `.` and `..` in this block, which should make things faster.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:110
+    if (Att & FILE_ATTRIBUTE_DIRECTORY)
+      ListFilesInDirRecursive(FileName, Epoch, V, false);
+    else if (IsFile(FileName))
----------------
I generally prefer an iterative solution instead of recursive.  I've found it's slightly faster to iterate one directory completely before descending to a subdirectory.  It also uses fewer system resources, as you have only one FindHandle open at a time.  But this is probably fine, and I don't know if the caller is expecting the files in the recursive order.

The iterative solution is to have a queue of directories.  Each time you find a new directory, push it onto the queue.  Iterate until the queue is empty.


Repository:
  rL LLVM

https://reviews.llvm.org/D27233