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

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

My comments are mostly just things to be aware of.  No changes required.



================
Comment at: lib/Fuzzer/FuzzerUtil.cpp:205
+  }
+  return N;
 }
----------------
Implicit conversion from unsigned to signed.  Probably not an issue until processors get a lot bigger.

Relying on implicit conversions makes it hard to turn on conversion warnings, which might help detect real bugs in other parts of the code.


================
Comment at: lib/Fuzzer/FuzzerUtilPosix.cpp:56
+
+void SetTimer(int Seconds) {
+  struct itimerval T {{Seconds, 0}, {Seconds, 0}};
----------------
Be aware that this name clashes with the Windows function SetTimer that's in scope here.  You're OK, since this is a C++ file and so it'll be name-mangled and thus distinct.  If this had been a C source file instead, though, this would be a problem.


================
Comment at: lib/Fuzzer/FuzzerUtilWindows.cpp:159
+
+int GetPid() { return GetCurrentProcessId(); }
+
----------------
Be aware that this converts from an unsigned to a signed type.  This is not a big deal with modern Windows, since it's extremely unlikely you'll ever see a PID approaching 2^31, but it's at least theoretically possible.  If some logging somewhere prints this as a signed int, it may not look like the value you'd see in other Window stools.

Raymond Chen mentions seeing PIDs approaching 4 billion, but I suspect that's when they were actually pointers in disguise.  http://stackoverflow.com/a/17868515/1386054




================
Comment at: lib/Fuzzer/FuzzerUtilWindows.cpp:186
+  if (!Data || !Patt || DataLen == 0 || PattLen == 0 || DataLen < PattLen)
+    return NULL;
+
----------------
I'm not sure if libfuzzer has its own coding style guidelines, but I'd probably use `nullptr` rather than `NULL`.


Repository:
  rL LLVM

https://reviews.llvm.org/D27234