[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/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;
----------------
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.


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


Repository:
  rL LLVM

https://reviews.llvm.org/D27179