[PATCH] D20943: [LibFuzzer] [WIP] Declare and use sanitizer functions in ``fuzzer::ExternalFunctions``

[Kostya Serebryany via llvm-commits]

kcc added inline comments.

================
Comment at: lib/Fuzzer/FuzzerExtFunctions.def:24
@@ +23,3 @@
+// Sanitizer functions
+EXT_FUNC(__sanitizer_print_stack_trace, void, (), true);
+EXT_FUNC(__sanitizer_reset_coverage, void, (), true);
----------------
delcypher wrote:
> kcc wrote:
> > This last boolean argument is hard to reason about.
> > BTW, I don't know why you need it at all. 
> > We must be able to link w./o any of those symbols (see test/UninstrumentedTest.cpp)
> The meaning of the last boolean argument is whether or not to emit a warning during initialization of `ExternalFunctions` if the function cannot be found at runtime. 
> 
> What I was suggesting is that this last argument could be turned into an enum to indicate what to do when function cannot be found at runtime during initialization. This could be one of three things.
> 
> * Do Nothing
> * Emit a warning
> * Emit an error and exit
> 
> This does not affect the compile time linking, it affects what happens at run time.
> 
> What this would change is that we would immediately exit at runtime if the essential functions are not available rather than waiting until they are called (the currently behaviour). This would then allow the `CHECK_WEAK_API_FUNCTION` macro to be removed.
> 
> How do you feel about this proposal?
At the very least that's a change in behavior. 
Try not to combine refactoring and changing the behavior it complicates reasoning during the code review. 
If you will this functionality is needed let's do it separately. 
In this change, please just drop this. 

================
Comment at: lib/Fuzzer/FuzzerLoop.cpp:49
@@ -72,2 +48,3 @@
   do {                                                                         \
-    if (!fn)                                                                   \
+    /* FIXME: What a hack!*/                                                   \
+    if (!EF.fn)                                                                \
----------------
delcypher wrote:
> kcc wrote:
> > what do you want to fix here? 
> * The name of the macro is now wrong for OSX (we don't use weak symbols)
> * The suggestion I have made to check for function availability in the `ExternalFunctions` CTOR means we could get rid of this macro entirely.
> * What I've done is a hack to keep the tests passing because the stringified name `#fn` gets given to `MissingWeakApiFunction(const char *FnName)` I had to have users of the macro do
> 
> `CHECK_WEAK_API_FUNCTION(__sanitizer_reset_coverage)`
> 
> rather than
> 
> `CHECK_WEAK_API_FUNCTION(EF.__sanitizer_reset_coverage)`
Remove the FIXME for now.


http://reviews.llvm.org/D20943