[libFuzzer] Port to Windows

[metzman]

Summary

Port libFuzzer to windows-msvc.
This patch allows libFuzzer targets to be built and run on Windows, using -fsanitize=fuzzer and/or fsanitize=fuzzer-no-link. It allows these forms of coverage instrumentation to work on Windows as well.
It does not fix all issues, such as those with -fsanitize-coverage=stack-depth, which is not usable on Windows as of this patch.
It also does not fix any libFuzzer integration tests. Nearly all of them fail to compile, fixing them will come in a later patch, so libFuzzer tests are disabled on Windows until them.

[metzman]

Run clang-format

[metzman]

@morehouse Can you please take a look?

llvm/test/Instrumentation/SanitizerCoverage/coff-pc-table-inline-8bit-counters.ll

1 ↗

(On Diff #161783)

@kcc I wrote a lit test for a basic sanity check.
I think the instrumentation on windows is also worth testing in check-fuzzer, but I didn't add a test there because check-fuzzer is still broken on Windows.
I will add the test there with my patch fixing check-fuzzer.

[morehouse]

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

815 ↗

(On Diff #161797)

Nit: else is unnecessary.

[morehouse]

compiler-rt/lib/fuzzer/FuzzerTracePC.cpp

35 ↗

(On Diff #161797)

Can we make an OS-specific macro for __attribute__((tls_model("initial-exec"))) and/or thread_local instead?

compiler-rt/lib/sanitizer_common/sanitizer_coverage_win_sections.cc

23 ↗

(On Diff #161797)

What happens without align(1) and uint64_t?

llvm/test/Instrumentation/SanitizerCoverage/coff-pc-table-inline-8bit-counters.ll

23 ↗

(On Diff #161797)

Does body of foo matter for this test? Can it just return?

[metzman]

Fix nit (remove unnecessary else) and minimize integration test.

[metzman]

compiler-rt/lib/fuzzer/FuzzerTracePC.cpp

35 ↗

(On Diff #161797)

I'm looking into this, but it looks a bit complicated to get this working, as there are two problems here:

1. How to share the symbol with sanitizer_coverage_libcdep_new.cc (I assume techniques from sanitizer_win_defs.h can work).
2. How to combine thread_local with ATTRIBUTE_INTERFACE (__declspec(dllexport)) on windows since this isn't allowed (I think solution will be not using dllexport for __sancov_lowest_stack).

Will investigate this further.

compiler-rt/lib/sanitizer_common/sanitizer_coverage_win_sections.cc

23 ↗

(On Diff #161797)

Without align(1) the arrays get aligned so that in between __start___sancov_cntrs and __stop___sancov_cntrs and in between __start___sancov_pcs and __stop___sancov_pcs there are padding bytes, causing libFuzzer to think there is an incorrect number of PCs and counters, which causes an early exit.
Without uint64_t (but with align(1)) sancov_pcs gets word aligned (ie: there is a 4 byte gap between __start_sancov_pcs and the first element in the array), causing the same issue.

tl;dr without either (or without incremental:no) I get:

ERROR: The size of coverage PC tables does not match the
number of instrumented PCs. This might be a compiler bug,
please contact the libFuzzer developers.
Also check https://bugs.llvm.org/show_bug.cgi?id=34636
for possible workarounds (tl;dr: don't use the old GNU ld)

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

815 ↗

(On Diff #161797)

Done.

llvm/test/Instrumentation/SanitizerCoverage/coff-pc-table-inline-8bit-counters.ll

23 ↗

(On Diff #161797)

No. Yes it can return.
Good catch, I've fixed this.

[morehouse]

llvm/test/Instrumentation/SanitizerCoverage/coff-pc-table-inline-8bit-counters.ll

23 ↗

(On Diff #161797)

Can it be simplified more? For example, inline-8bit-counters.ll does:

define void @foo() {
entry:
  ret void
}

[metzman]

Make lit test minimal

[metzman]

llvm/test/Instrumentation/SanitizerCoverage/coff-pc-table-inline-8bit-counters.ll

23 ↗

(On Diff #161797)

Ah my bad. I think it is minimal now.

[metzman]

Replace solution for stack-depth with a prettier one.

[morehouse]

compiler-rt/lib/fuzzer/FuzzerTracePC.cpp

519 ↗

(On Diff #161987)

Rather than having these ifdefs, can we adjust the parameters passed to these init functions (i.e. modify SecStart and SecEnd in SanitizerCoverage.cpp)?

Then users of SanCov won't have to do this manual adjustment for Windows either.

[metzman]

compiler-rt/lib/fuzzer/FuzzerTracePC.cpp

519 ↗

(On Diff #161987)

I don't think we can adjust the parameters using the current approach.
Under the current approach __start___sancov_pcs (pcs_beg) doesnt point to the start of the pc array on windows, it points to data that comes before the array.
I'm pretty sure this was an issue with the initial port that used guards (ie: it should have added to the parameters), just no one noticed because there was no check to tell if the array size was correct.

[metzman]

compiler-rt/lib/fuzzer/FuzzerTracePC.cpp

519 ↗

(On Diff #161987)

Actually, I have an idea for how to do this. I'll get back to you.

[metzman]

Use less #ifdefs and fix issue with guards (because of failing lit test)

[metzman]

Update comments

[metzman]

compiler-rt/lib/fuzzer/FuzzerTracePC.cpp

519 ↗

(On Diff #161987)

I now do this addition in SanitizerCoverage.cpp as you suggested. There are no more #ifdefs in this file.

Thank you for helping me figure out how to do this offline.

[metzman]

Reid could you please take a look.
This patch gets libFuzzer working on Windows.
Most functionality seems to work, a few features are not yet supported and the tests don't yet work. I plan on fixing these issues soon.

[rnk]

compiler-rt/lib/fuzzer/tests/FuzzerUnittest.cpp

33–38 ↗

(On Diff #162054)

Basename should be fixed to handle / and \ on windows.

compiler-rt/lib/sanitizer_common/sanitizer_coverage_win_sections.cc

23 ↗

(On Diff #161797)

I'm not sure align(1) really addresses this problem. The Microsoft linker will add padding either way for incremental links. For ASan global metadata registration, we had to teach the runtime to tolerate zero padding.

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

276–281 ↗

(On Diff #162054)

I kind of prefer ifdefs in runtime C code over platform-specific instrumentation. @morehouse, wdyt?

826 ↗

(On Diff #162054)

I would recommend naming your sections:
.SCOV$CM
.SCOV$PM
.SCOV$GM

Your start markers and end markers will work just the same as .SCOV$[CPG][AZ].

You should also add a /merge:.SCOV=.data flag with #pragma comment linker in the sanitizer coverage runtime. That way the final PE file won't have a .SCOV section, it'll all be in .data, laid out the way you want.

[metzman]

@rnk thank you for the feedback.
I will fix the issues you have raised shortly, but I have some questions on the feedback you left.

compiler-rt/lib/sanitizer_common/sanitizer_coverage_win_sections.cc

23 ↗

(On Diff #161797)

Ouch.
Wouldn't incremental:no (which I set in MSVC.cpp) prevent incremental links?
Or is this insufficient since people should be able to link outside of clang?
(I assume it is not sufficient, since I got that idea from ASan).

I will take a look at what ASAN does to handle padding and try to implement something similar.

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

826 ↗

(On Diff #162054)

Can the PCTable which is constant go into .data?
Initially I tried having one section but if I recall correctly PCTable being constant caused issues.
I can look into this further though, maybe the solution is to make PCTable non-constant on windows

[metzman]

Fix BaseName to handle '/' and '\' on Windows.

[morehouse]

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

276–281 ↗

(On Diff #162054)

Why would you prefer that way?

My reasoning is that its better to fix this in one place rather than expect every client of SanCov to place ifdefs for Windows in their code.

[metzman]

Undo unnecessary change to unittests.

[metzman]

compiler-rt/lib/sanitizer_common/sanitizer_coverage_win_sections.cc

23 ↗

(On Diff #161797)

@rnk and @kcc (assuming we can't prevent incremental links and need to deal with padding):
Does searching for a magic string at runtime (and adding one during instrumentation) seem like a good way to determine the start and end of the array in spite of padding?
I don't think I can simply skip over zero bytes because of the PC Table. The PC Table can legitimately start with zeros since each entry in the table starts with an address.
The end of the PCTable can be determined without a magic string but the end of the guard and counter arrays cannot, unless they are initialized to something other than 0, so I think using magic strings to determine start and end of all arrays makes most sense.

[rnk]

compiler-rt/lib/sanitizer_common/sanitizer_coverage_win_sections.cc

23 ↗

(On Diff #161797)

I'm not sure we can. I think if your approach works, we might want to commit it and then debug the problems in alternative VC link configurations. It might be OK if libfuzzer only works on windows with lld.

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

826 ↗

(On Diff #162054)

I see, I checked and I thought I didn't see any constant data. That should use a separate section, maybe .SCOVP$M as originally designed. The important thing isn't having less COFF sections, it's having fewer PE sections. You can check that they get eliminated with dumpbin /summary on a libfuzzer exe.

[metzman]

Merge guard and counter sections with .data section.
Also add better comments.

[metzman]

compiler-rt/lib/sanitizer_common/sanitizer_coverage_win_sections.cc

23 ↗

(On Diff #161797)

I've merged the guard and counter array with the .data section.
The PCTable which is constant, is in its own section.

I think if your approach works, we might want to commit it and then debug the problems in alternative VC link configurations

My approach works with a decent amount of examples I've tried (both with use-ld=lld and without). I suppose we will learn more once we start using it in Chrome.

Have you seem the linker adding padding in spite of /incremental:no being used in ASAN builds?

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

276–281 ↗

(On Diff #162054)

@rnk do you still want me to change this?

[metzman]

run clang format.

[rnk]

One last thing and I think we can merge this.

compiler-rt/lib/sanitizer_common/sanitizer_coverage_win_sections.cc

23 ↗

(On Diff #161797)

Any reason not to do add this?

#pragma comment(linker, "/MERGE:.SCOVP=.rdata")

It should get the readonly behavior you want without making an extra PE section.

Have you seem the linker adding padding in spite of /incremental:no being used in ASAN builds?

No, in a non-incremental build, MSVC will only add padding to satisfy the alignment of each entry.

I guess I see why you downgrade the uin64_t's to one byte alignment, it's mainly to avoid the zero entry at the end of the array.

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

276–281 ↗

(On Diff #162054)

Normally we follow the principle that the more complexity we can move from instrumentation to runtime code, the better. If there are multiple clients, then keeping it here makes sense.

[metzman]

Merge SCOVP section with .rdata

[rnk]

lgtm

[metzman]

Thank you @rnk and @morehouse for your reviews.

This can't be landed if it causes ninja check-all to fail right?
This is happening on Windows now because of fuzzer test failures (check-all is passing on Linux and check-llvm check-clang and check-asan are passing on Windows).
If it can't be landed because of this then I guess I will disable libFuzzer on Windows from being built (or maybe just all of the fuzzer tests if possible).

Also, could one of you please commit this patch for me? I'm not an LLVM committer so I'm unable to do this myself.
(If it can't be landed then I'll just ping whoever volunteers)

Thanks!

[rnk]

In D51022#1215212, @metzman wrote:

Thank you @rnk and @morehouse for your reviews.

This can't be landed if it causes ninja check-all to fail right?
This is happening on Windows now because of fuzzer test failures (check-all is passing on Linux and check-llvm check-clang and check-asan are passing on Windows).
If it can't be landed because of this then I guess I will disable libFuzzer on Windows from being built (or maybe just all of the fuzzer tests if possible).

I thought we removed libfuzzer from check-all. I'll check the situation.

Also, could one of you please commit this patch for me? I'm not an LLVM committer so I'm unable to do this myself.
(If it can't be landed then I'll just ping whoever volunteers)

Thanks!

[morehouse]

We will need to disable failing tests for Windows. libFuzzer does run as part of check-all now.

[metzman]

Disable libFuzzer tests on Windows.

[metzman]

In D51022#1215225, @morehouse wrote:

We will need to disable failing tests for Windows. libFuzzer does run as part of check-all now.

Pretty much all lit tests are failing.

I've disabled all libFuzzer tests on Windows. check-all passes on Windows now.

[metzman]

Improve comment

[metzman]

Improve comments.

[metzman]

Fix failing assertions by using CreateGEP instead of CreateAdd to add to a pointer.

[metzman]

Trivial NFC to use local variable instead of accessing attribute twice.

[metzman]

• Port libFuzzer to Windows

lib/Driver/ToolChains/MSVC.cpp

void visualstudio::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   if (DLL) {
     CmdArgs.push_back(Args.MakeArgString("-dll"));
 
     SmallString<128> ImplibName(Output.getFilename());
     llvm::sys::path::replace_extension(ImplibName, "lib");
     CmdArgs.push_back(Args.MakeArgString(std::string("-implib:") + ImplibName));
   }
 
+  if (TC.getSanitizerArgs().needsFuzzer()) {
+    if (!Args.hasArg(options::OPT_shared))
+      CmdArgs.push_back(
+          Args.MakeArgString(std::string("-wholearchive:") +
+                             TC.getCompilerRTArgString(Args, "fuzzer", false)));
+    CmdArgs.push_back(Args.MakeArgString("-debug"));
+    // Prevent the linker from padding sections we use for instrumentation
+    // arrays.
+    CmdArgs.push_back(Args.MakeArgString("-incremental:no"));
+  }
+
   if (TC.getSanitizerArgs().needsAsanRt()) {
     CmdArgs.push_back(Args.MakeArgString("-debug"));
     CmdArgs.push_back(Args.MakeArgString("-incremental:no"));
     if (TC.getSanitizerArgs().needsSharedRt() ||
         Args.hasArg(options::OPT__SLASH_MD, options::OPT__SLASH_MDd)) {
       for (const auto &Lib : {"asan_dynamic", "asan_dynamic_runtime_thunk"})
         CmdArgs.push_back(TC.getCompilerRTArgString(Args, Lib));
       // Make sure the dynamic runtime thunk is not optimized out at link time
else
           (Twine("msvc") + MSVT.getAsString() + Twine('-') + ObjFmt).str());
   }
   return Triple.getTriple();
 }
 
 SanitizerMask MSVCToolChain::getSupportedSanitizers() const {
   SanitizerMask Res = ToolChain::getSupportedSanitizers();
   Res |= SanitizerKind::Address;
+  Res |= SanitizerKind::Fuzzer;
+  Res |= SanitizerKind::FuzzerNoLink;
   Res &= ~SanitizerKind::CFIMFCall;
   return Res;
 }
 
 static void TranslateOptArg(Arg *A, llvm::opt::DerivedArgList &DAL,
                             bool SupportsForcingFramePointer,
                             const char *ExpandChar, const OptTable &Opts) {
   assert(A->getOption().matches(options::OPT__SLASH_O));