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

[Marcos Pividori via Phabricator via llvm-commits]

mpividori added inline comments.


================
Comment at: lib/Fuzzer/FuzzerUtil.cpp:205
+  }
+  return N;
 }
----------------
amccarth wrote:
> 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.
@amccarth Ok, I will make an explicit cast.


================
Comment at: lib/Fuzzer/FuzzerUtilPosix.cpp:56
+
+void SetTimer(int Seconds) {
+  struct itimerval T {{Seconds, 0}, {Seconds, 0}};
----------------
amccarth wrote:
> 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.
@amccarth thanks. Anyway, I remove that function in the diff 7.


================
Comment at: lib/Fuzzer/FuzzerUtilWindows.cpp:159
+
+int GetPid() { return GetCurrentProcessId(); }
+
----------------
amccarth wrote:
> 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
> 
> 
@amccarth Ok. I will add that changes in a new diff.


================
Comment at: lib/Fuzzer/FuzzerUtilWindows.cpp:186
+  if (!Data || !Patt || DataLen == 0 || PattLen == 0 || DataLen < PattLen)
+    return NULL;
+
----------------
amccarth wrote:
> I'm not sure if libfuzzer has its own coding style guidelines, but I'd probably use `nullptr` rather than `NULL`.
@amccarth it doesn't have own style guidelines... I think we should follow LLVM guidelines. But this requires many changes... If @kcc agrees I can update the code to follow LLVM code guidelines in a new diff.


Repository:
  rL LLVM

https://reviews.llvm.org/D27234