This is an archive of the discontinued LLVM Phabricator instance.

[libFuzzer] Diff 3 - Split IO implementation for Windows and Posix.
ClosedPublic

Authored by mpividori on Nov 29 2016, 3:24 PM.

Details

Summary

Divide the IO functions's implementation in 2 different files for differences between POSIX and Windows API.

Diff Detail

Repository
rL LLVM

Event Timeline

mpividori updated this revision to Diff 79654.Nov 29 2016, 3:24 PM
mpividori retitled this revision from to [libFuzzer] Diff 3 - Split IO implementation for Windows and Posix..
mpividori updated this object.
mpividori added reviewers: kcc, zturner.
mpividori set the repository for this revision to rL LLVM.
mpividori added a subscriber: llvm-commits.

The Windows APIs have lots of gotchas.

lib/Fuzzer/FuzzerIOWindows.cpp
29

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.

84

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.

101

You don't need to call GetFileAttributesA. The attributes are already available in the WIN32_FIND_DATA.

103

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.

110

You could put the test for . and .. in this block, which should make things faster.

111

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.

mpividori updated this revision to Diff 79690.Nov 29 2016, 6:49 PM

Update Diff.

zturner edited edge metadata.Nov 30 2016, 8:58 AM

Looks good to me with Adrian's suggested changes. I know you said you updated the diff, but I'm still seeing all of the issues Adrian mentioned as being unresolved?

Yes, I updated the diff to move some changes from Diff 2, suggested by kcc. I need to work on the issues mentioned by Adrian.

kcc edited edge metadata.Nov 30 2016, 10:01 AM

Note: I rely on the other reviewers for the windows-specific parts.

lib/Fuzzer/FuzzerIO.cpp
20

can you avoid this ifdef?
E.g. move whatever needs unistd.h into *Posix* file

lib/Fuzzer/FuzzerIOPlatform.h
1 ↗(On Diff #79690)

why do we need both FuzzerIO.h and FuzzerIOPlatform.h?

mpividori added inline comments.Nov 30 2016, 10:27 AM
lib/Fuzzer/FuzzerIOWindows.cpp
29

@amccarth thanks for your comments. Yes, we plan to modify the code in the future to use std::wstring, as Support library does.

84

Ok, I will change that.

101

@amccarth you are right. I will modify this.

110
111

@amccarth thanks. I adapted the previous implementation in libFuzzer for Posix, which was recursive. I don't think the order of files matters here, so I can change it to consider an iterative solution. I will add that changes in a new diff.

mpividori added inline comments.Nov 30 2016, 10:59 AM
lib/Fuzzer/FuzzerIO.cpp
20

@kcc Ok. I will move DeleteFile() implementation to the files FuzzerIOPosix.cpp and FuzzerIOWindows.cpp.

lib/Fuzzer/FuzzerIOPlatform.h
1 ↗(On Diff #79690)

@kcc , The initial idea was to separate FuzzerIO.h and FuzzerIOPlatform.h, so FuzzerIO.h includes functions used by the rest of the modules, and FuzzerIOPlatform.h includes the platform specific functions only included by the FuzzerIO.cpp implementation.
But with recent changes, this is not valid any more, because some implementations need to access some functions from FuzzerIO platform specific implementation like DeleteFile(), so I will merge both headers files into FuzzerIO.h.

mpividori updated this revision to Diff 79782.Nov 30 2016, 11:10 AM
mpividori edited edge metadata.

Include changes proposed by @kcc and @amccarth .

kcc accepted this revision.Nov 30 2016, 11:12 AM
kcc edited edge metadata.

LGTM

This revision is now accepted and ready to land.Nov 30 2016, 11:12 AM
This revision was automatically updated to reflect the committed changes.