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

Anna Zaks via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 1 09:15:59 PDT 2016


zaks.anna added inline comments.

================
Comment at: lib/Fuzzer/FuzzerExtFunctionsDlsym.cpp:15
@@ +14,3 @@
+#include "FuzzerInternal.h"
+#if LIBFUZZER_APPLE
+
----------------
kcc wrote:
> mehdi_amini wrote:
> > zaks.anna wrote:
> > > mehdi_amini wrote:
> > > > 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).
> > > I find this much less readable.
> > You're saying that
> > 
> > ```
> > #ifdef LIBFUZZER_USE_DLOPEN
> > ...
> > #endif
> > ```
> > 
> > is less readable than
> > 
> > ```
> > #ifdef LIBFUZZER_APPLE
> > ...
> > #endif
> > ```
> > 
> > I'm missing your point here.
> Either is fine for me here. 
> directly using #ifdef *_APPLE and *_LINUX is shorter. 
> Using a proxy macros *_USE_* shows the intent more clear but adds clatter.  
> 
> Decide between yourselves. :) 
> You're saying that
> #ifdef LIBFUZZER_USE_DLOPEN
> ...
> #endif
> is less readable than
> #ifdef LIBFUZZER_APPLE
> ...
> #endif
> I'm missing your point here.

Yes, it would be more difficult to understand if code is executed on the given platform or not as you are introducing a level of indirection. 

> I'd rather write it in a way that can be overridden through a build setting,

Overriding a build setting would add yet another level of indirection. Debugging this won't be fun especially given all the different ways in which this could be built by various parties.

Unless there is a real reason we want to override it, I'd rather keep things simple.




http://reviews.llvm.org/D20741





More information about the llvm-commits mailing list