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

[Marcos Pividori via Phabricator via llvm-commits]

mpividori added inline comments.


================
Comment at: lib/Fuzzer/FuzzerUtil.h:47-54
+  HandlerT sigalrm_cb;
+  HandlerT sigsegv_cb;
+  HandlerT sigbus_cb;
+  HandlerT sigabrt_cb;
+  HandlerT sigill_cb;
+  HandlerT sigfpe_cb;
+  HandlerT sigint_cb;
----------------
zturner wrote:
> mpividori wrote:
> > zturner wrote:
> > > How about getting rid of the constructor and initializing all these inline?
> > > 
> > > ```
> > > int timeout = 0;
> > > HandlerT sigalrm_cb = nullptr;
> > > // etc
> > > ```
> > @zturner , I think using a constructor is clearer. Do you mean to avoid the static constructor? In that case, I can remove it and initialize that members inline inside `SetSignalHandler()`.
> I don't know if it's stated anywhere in the coding standards, but there has been an push in the LLVM community recently to use inline initialization instead of constructor initializer list in cases such as this where there are long lists of member variables initialized with constant values.  So your entire class definition would look like:
> 
> ```
> struct SignalHandlerOptions {
>   int timeout = 0;
>   HandlerT sigalrm_cb = nullptr;
>   HandlerT sigsegv_cb = nullptr;
>   HandlerT sigbus_cb = nullptr;
>   HandlerT sigabrt_cb = nullptr;
>   HandlerT sigill_cb = nullptr;
>   HandlerT sigfpe_cb = nullptr;
>   HandlerT sigint_cb = nullptr;
>   HandlerT sigterm_cb = nullptr;
> };
> ```
> 
> And there would be no constructor definition.  In this case it's not critical, but the motivation is that when you have multiple constructor overloads that all repeat the same exact initializer list, this syntax reduces boilerplate and reduces the chance of 2 constructors accidentally initializing things differently.
@zturner , Thanks. Yes, I agree. It is simpler and reduces boilerplate code. I will add that changes.


================
Comment at: lib/Fuzzer/FuzzerUtilWindows.cpp:89-90
+  ~TimerQ() {
+    if (TimerQueue)
+      DeleteTimerQueueEx(TimerQueue, NULL);
+  };
----------------
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.



Repository:
  rL LLVM

https://reviews.llvm.org/D27179