[PATCH] D26956: Initial changes for porting libFuzzer to Windows.

[Marcos Pividori via llvm-commits]

mpividori added inline comments.


================
Comment at: lib/Fuzzer/FuzzerDefs.h:31
+#elif _WIN32
+#define LIBFUZZER_WINDOWS 1
 #else
----------------
zturner wrote:
> mpividori wrote:
> > zturner wrote:
> > > I think you will need to add
> > > 
> > > ```
> > > #define LIBFUZZER_POSIX 0
> > > #define LIBFUZZER_LINUX 0
> > > #define LIBFUZZER_APPLE 0
> > > ```
> > > 
> > > similarly, in the previous 2 branches of the `#if`, you will need to `#define LIBFUZZER_WINDOWS 0`
> > Hi,
> > I omitted them to simplify the code. (when not defined, the macros assume value 0).
> > If you think it is clearer, I can include them.
> As long as we're consistent it's fine.  If we're going to remove the 0 values, then remove them from the other cases too.
Yes, I agree.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:29
+bool IsFile(const std::string &Path) {
+  HANDLE FileHandle(
+      CreateFileA(Path.c_str(), 0, FILE_SHARE_READ, NULL, OPEN_EXISTING,
----------------
zturner wrote:
> mpividori wrote:
> > zturner wrote:
> > > Check the code in `llvm/lib/Support/Windows/Path.inc` for the `status()` function.  There are some subtleties that you should be aware of that this code doesn't handle.  You can probably just rip the code straight from there, changing `W` to `A` so we don't have to worry about widening the paths.
> > Hi,
> > Thanks. Yes, I based my implementation in the implementation of `status()` / `getStatus()` from the Support Library.
> > What I haven't included is the function `isReservedName`, because I assumed that reserved names would return `GetFileType()` different to `FILE_TYPE_DISK`  (probably `FILE_TYPE_CHAR` ?). Do you think I should include check for reserved names? The rest of the code is similar to the implementation of `status()`.
> Hmm.  The function I'm looking at first calls `GetFileAttributes()`, and then only if it is a reparse point (kinda like a symlink) does it open the handle.  This allows it to "follow the symlink" to get info about the target.  But the advantage of that approach is that `GetFileAttributes()` doesn't actually open the handle -- which can fail in certain scenarios -- unless it has to.
> 
> Are we looking at the same function?  The one I'm looking at is `lib/Support/Windows/Path.inc` line 464.
Hi, in fact the: ` if (attr & FILE_ATTRIBUTE_REPARSE_POINT) ` in `lib/Support/Windows/Path.inc` line 482, doesn't make sense. It does exactly the same when it is a reparse point than when it isn't .....
If it is not a reparse point, it also uses `CreateFileView` to open a handle...
I think there are some errors in that code, or the `if` statement is redundant...


Repository:
  rL LLVM

https://reviews.llvm.org/D26956