[PATCH] D27179: LibFuzzer - Implement timers for Windows and improve synchronization.

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: lib/Fuzzer/FuzzerInternal.h:146-147
   std::atomic<size_t> CurrentUnitSize;
+  bool RunningCB;
+  std::mutex RunningCBMtx;
   uint8_t BaseSha1[kSHA1NumBytes];  // Checksum of the base unit.
----------------
It seems like we could use `std::atomic_bool` here.  Does that not work for some reason?  The only thing I see that is questionable is that in `FuzzerCallback::CrashCallback` we check the value, and then run multiple lines of code before returning.

But on the other hand, in `Fuzzer::ExecuteCallback`, we always set it to true before executing any callback, and false after.  So maybe there is no race condition?

`atomic_bool` is more lightweight than grabbing a full blown mutex, so if we can be sure it's safe, then it seems better.


================
Comment at: lib/Fuzzer/FuzzerUtilWindows.cpp:89-90
+  ~TimerQ() {
+    if (TimerQueue)
+      DeleteTimerQueueEx(TimerQueue, NULL);
+  };
----------------
mpividori wrote:
> zturner wrote:
> > mpividori wrote:
> > > zturner wrote:
> > > > Shouldn't use all delete the timer as well?  You store it locally in `SetTimer` but then let the handle leak.  So every time we call `SetTimer` it will leak a handle.
> > > According to the documentation, the timers which were added to the `TimerQueue` are automatically deleted when deleting the `TimerQueue`.
> > Ahh, I didn't notice that.  But it seems we can still create multiple timers here.  For example if you call the `SetTimer` twice it will add a new timer instead of overwriting the old timer.  Is this desired?  If a user is not supposed to call `SetTimer` twice, then maybe we should add `assert(!TimerQueue)`.  But if it is ok to call `SetTimer` more than once and have subsequent calls overwrite the existing timer value, then we should probably store the `HANDLE` in the class and delete it and recreate on subsequent calls.
> > 
> > Also, one thing I just thought of that might be a problem.  The timer callback is not going to be invoked on the fuzzer thread, but rather a thread in the windows system thread pool.  But I looked at the Alarm callback function and it begins like this:
> > ```
> >   if (!InFuzzingThread()) return;
> > ```
> > 
> > So, I think this check is going to always fail.  If we need to guarantee that alarms happen on the fuzzing thread, we need a different strategy.
> > 
> > What do you think?
> @zturner thanks for your comments.
> + Yes, we can create multiple timers here, but we don't expose the function `SetTimer` any more (I removed that), so the user doesn't have access to it. The Timer instance is only accessed by `SetSignalHandler()` function. Anyway, I can add the changes you proposed.
> Also, the code could be simplified by using a separate thread for the timer instead of using `TimerQueues`. Something like:
> ```
> void TimerThread(int Seconds) {
>   while (1) {
>     sleep(Seconds);
>     AlrmCallback();
>   }
> }
> ```
> I wasn't sure which option is prefered. What do you think? If we consider a separate thread, we could use the same implementation for Posix too, and get rid of SIGALRM.
> 
> + The `if (!InFuzzingThread()) return;` line was removed in these changes. Yes, I adapted the code to handle that situation.
>   Previous implementation assumed that the ALRM signals would be handled by the main thread, which is not necessarily true. In POSIX it is unspecified with thread handle signals.
>   With the changes included in this diff, we can be sure that Alarm callback is executed by a thread different to the main thread in both Posix and Windows implementations:
>  -  In Windows: by a thread from the windows system thread pool.
>  -  In Posix: by a separate thread that waits for SIGALRM signals.
> 
I don't think we need to create a thread for it, using the windows threadpool is more efficient.


Repository:
  rL LLVM

https://reviews.llvm.org/D27179