This is an archive of the discontinued LLVM Phabricator instance.

[libfuzzer][MSVC] Make calls to builtin functions work with MSVC
ClosedPublic

Authored by metzman on Jan 8 2019, 6:45 AM.

Details

Summary

Replace calls to builtin functions with macros or functions that call the
Windows-equivalents when targeting windows and call the original
builtin functions everywhere else.
This change makes more parts of libFuzzer buildable with MSVC.

Diff Detail

Event Timeline

metzman created this revision.Jan 8 2019, 6:45 AM
metzman retitled this revision from [fuzzer] Replace calls to builtin functions with macros to [libfuzzer] Replace calls to builtin functions with macros.Jan 8 2019, 6:47 AM
metzman edited the summary of this revision. (Show Details)
metzman updated this revision to Diff 180668.Jan 8 2019, 7:24 AM
  • less casting and use intrisics.
metzman updated this revision to Diff 180675.Jan 8 2019, 8:11 AM
  • include <stdlib.>
  • reword
metzman updated this revision to Diff 180676.Jan 8 2019, 8:16 AM
  • add nonwin implementations
metzman updated this revision to Diff 180681.Jan 8 2019, 8:39 AM
metzman marked 2 inline comments as done.
  • fix spacing
  • remove newline
metzman retitled this revision from [libfuzzer] Replace calls to builtin functions with macros to [libfuzzer][MSVC] Replace calls to builtin functions with macros.Jan 8 2019, 8:40 AM
metzman marked 2 inline comments as not done.

Vitaly, could you please take a look?

I think Kostya wants to try making LF compilable with MSVC so he asked me to split up https://reviews.llvm.org/D56363

This change specifically handles replacing calls to builtin functions with things that can be compiled with MSVC

Thank you

compiler-rt/lib/fuzzer/FuzzerDefs.h
143 ↗(On Diff #180676)

I added reinterpret_cast<uintptr_t>() to GET_CALLER_PC since every caller casts the result, was this the right call?

206 ↗(On Diff #180676)

In my current approach I defined wrapper functions for Windows and non-Windows (which call the builtins).
An alternative is to continue calling __builtin_* everywhere and simply define these when compiling with MSVC. I thought that approach was too hacky and that this is better WDYT?
Another approach is using macros, but I think the inline functions are preferable (though I'll do what ever you think is best).

CC @rnk and @thakis (since rnk is OOO)

vitalybuka added inline comments.Jan 8 2019, 4:49 PM
compiler-rt/lib/fuzzer/FuzzerDefs.h
197 ↗(On Diff #180681)

Could you please move them into:
fuzzer/FuzzerBuiltins.h
fuzzer/FuzzerBuiltinsMsvc.h

include thsese headers unconditionally
but inside of the header put

#if defined(_MSC_VER)
...
and
#if !defined(_MSC_VER)

143 ↗(On Diff #180676)

GET_CALLER_PC is defined in sanitizer_common, so it should probably defined there
also if we probably want to keep __builtin_return_address if we compile with clang,

metzman marked an inline comment as done.Jan 9 2019, 7:45 AM
metzman added inline comments.
compiler-rt/lib/fuzzer/FuzzerDefs.h
143 ↗(On Diff #180676)

GET_CALLER_PC is defined in sanitizer_common, so it should probably defined there

Can libFuzzer depend on sanitizer_common?

also if we probably want to keep __builtin_return_address if we compile with clang,

I'll do this, but I figured it would be simpler to do things one way on Windows and another way everywhere else instead of having two ways to do things on Windows.

metzman retitled this revision from [libfuzzer][MSVC] Replace calls to builtin functions with macros to [libfuzzer][MSVC] Make calls to builtin functions cross platform.Jan 9 2019, 9:15 AM
metzman edited the summary of this revision. (Show Details)
metzman updated this revision to Diff 180871.Jan 9 2019, 10:09 AM
  • Move builtin wrappers to their own files.
metzman retitled this revision from [libfuzzer][MSVC] Make calls to builtin functions cross platform to [libfuzzer][MSVC] Make calls to builtin functions work with MSVC.Jan 9 2019, 10:10 AM
metzman updated this revision to Diff 180876.Jan 9 2019, 10:27 AM
metzman marked 2 inline comments as done.
  • move Bswap to FuzzerBuiltin headers for consistency.
metzman added inline comments.Jan 9 2019, 10:30 AM
compiler-rt/lib/fuzzer/FuzzerDefs.h
197 ↗(On Diff #180681)

Done.

143 ↗(On Diff #180676)

Made both of these changes.

metzman updated this revision to Diff 180880.Jan 9 2019, 10:40 AM
  • define LIBFUZZER_MSVC
vitalybuka accepted this revision.Jan 9 2019, 11:48 AM

LGTM

compiler-rt/lib/fuzzer/FuzzerBuiltins.h
22 ↗(On Diff #180880)

could you please separate namespace from the other code with emty line
same for closing } and other files
just for consistency

compiler-rt/lib/fuzzer/FuzzerBuiltinsMsvc.h
40 ↗(On Diff #180880)

#error ?

This revision is now accepted and ready to land.Jan 9 2019, 11:48 AM
metzman updated this revision to Diff 180899.Jan 9 2019, 12:15 PM
metzman marked 4 inline comments as done.
  • less casting and use intrisics.
  • fmt
  • include properly
  • reword
  • add nonwin implementations
  • fix spacing
  • remove newline
  • Move builtin wrappers to their own files.
  • fix headers
  • fix
  • fix
  • fmt
  • fmt
  • move Bswap to FuzzerBuiltin headers for consistency.
  • define LIBFUZZER_MSVC
  • add spacing and ensure not on x86
metzman updated this revision to Diff 180904.Jan 9 2019, 12:31 PM
  • Add space before end of namespace
vitalybuka accepted this revision.Jan 9 2019, 1:26 PM
metzman added inline comments.Jan 9 2019, 1:45 PM
compiler-rt/lib/fuzzer/FuzzerBuiltins.h
22 ↗(On Diff #180880)

Done in both builtins headers.

compiler-rt/lib/fuzzer/FuzzerBuiltinsMsvc.h
40 ↗(On Diff #180880)

Done.

This revision was automatically updated to reflect the committed changes.
Herald added subscribers: Restricted Project, llvm-commits. · View Herald TranscriptJan 9 2019, 1:49 PM
This comment was removed by asl.
asl added a subscriber: asl.Jan 14 2019, 5:30 AM