[PATCH] D27234: [libFuzzer] Diff 4 - Split FuzzerUtil implementation for Posix and Windows.

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: lib/Fuzzer/FuzzerUtil.cpp:199
+int NumberOfCpuCores() {
+  unsigned N = std::thread::hardware_concurrency();
+  if (!N) {
----------------
I noticed that in the `SleepSeconds` function, there is a comment that they couldn't use stl because they wanted to avoid coverage of instrumented libc++.  You might wait for comments from kcc@ about whether this is relevant here as well.  If so, we might need to go back to pure C implementations for each platform.


================
Comment at: lib/Fuzzer/FuzzerUtilWindows.cpp:169
+bool ExecuteCommandAndReadOutput(const std::string &Command, std::string *Out) {
+  FILE *Pipe = _popen(Command.c_str(), "r");
+  if (!Pipe) return false;
----------------
This function appears almost entirely the same except `_popen` vs `popen`.  Perhaps this could be hidden behind a function called `OpenProcessPipe` and then `ExecuteCommandAndReadOutput` could be shared.



================
Comment at: lib/Fuzzer/FuzzerUtilWindows.cpp:173-174
+  size_t N;
+  while ((N = fread(Buff, 1, sizeof(Buff), Pipe)) > 0)
+    Out->append(Buff, N);
+  return true;
----------------
This looks wrong to me.  `fread` does not indicate an error by returning a value less than 0, it indicates an error by returning a value different from the requested number of bytes. Then, you check `feof` and `ferr` to determine whether it was an error or an end of file.  This code doesn't seem to handle the case where there was an error reading the process output.


Repository:
  rL LLVM

https://reviews.llvm.org/D27234