[PATCH] D20741: [LibFuzzer] Reimplement how the optional user functions are called.

Mehdi AMINI via llvm-commits llvm-commits at lists.llvm.org
Tue May 31 22:02:33 PDT 2016


mehdi_amini added inline comments.

================
Comment at: lib/Fuzzer/FuzzerExtFunctionsDlsym.cpp:15
@@ +14,3 @@
+#include "FuzzerInternal.h"
+#if LIBFUZZER_APPLE
+
----------------
mehdi_amini wrote:
> zaks.anna wrote:
> > delcypher wrote:
> > > mehdi_amini wrote:
> > > > Again, there is nothing Apple specific here AFAIK, please rename.
> > > The comments above explain why this is the implementation we want to use on Apple platforms. Although this code compiles on Linux it does not work as intended unless an extra flag is passed to the linker by the client when linking against LibFuzzer. If we assume for the moment that we want LibFuzzer to be used with a platform's default linking behavior then this implementation is specific to the behavior of the Darwin linker (and possibly other non GNU linkers). If later it turns out we also need to use this implementation for other platforms (e.g. the *BSDs) then the #ifdef can be appropriately modified in the future.
> > > 
> > > If I had it my way I wouldn't use #ifdefs and would tell CMake to compile the appropriate file based on the host platform but that option is not available to me.
> > This code is currently only executed on "Apple" platforms so, in IMHO, spelling out which platform this code will be running on is better for readability than using some abstract name.
> > 
> > On the other hand, should we use the names consistent with what is declared in lib/sanitizer_common/sanitizer_platform.h? Ex: LIBFUZZER_MAC instead if LIBFUZZER_APPLE... LIBFUZZER_MAC is a horrible name (for including OSX and iOS), but consistency might be more important.
> None of what you're writing here is justifying the naming IMO, so again it should be something like:
> 
> 
> ```
> #ifdef LIBFUZZER_USE_DLOPEN
> ...
> #endif
> ```
> 
> I'm more flexible on how you get the default, but I'd rather write it in a way that can be overridden through a build setting, i.e. something along these lines (haven't checked the syntax):
> 
> ```
> #ifndef LIBFUZZER_USE_DLOPEN
>   #if LIBFUZZER_LINUX
>     #define LIBFUZZER_USE_WEAKLINK
>   #else
>     #define LIBFUZZER_USE_DLOPEN
>   #endif
> #else
>   #ifdef LIBFUZZER_USE_WEAKLINK
>     #error "Can't set both LIBFUZZER_USE_DLOPEN and LIBFUZZER_USE_WEAKLINK"
>   #endif
> #endif
> ```
(I was replying to delcypher and not Anna as the email thread suggests).


http://reviews.llvm.org/D20741





More information about the llvm-commits mailing list