This is an archive of the discontinued LLVM Phabricator instance.

[libFuzzer] Port to Windows
ClosedPublic

Authored by metzman on Aug 20 2018, 7:20 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

metzman created this revision.Aug 20 2018, 7:20 PM
metzman updated this revision to Diff 161636.Aug 20 2018, 7:27 PM
metzman changed the visibility from "Public (No Login Required)" to "metzman (Jonathan Metzman)".
metzman removed subscribers: srhines, kubamracek, mgorny, hiraditya.
metzman updated this revision to Diff 161775.Aug 21 2018, 11:23 AM
metzman updated this revision to Diff 161780.Aug 21 2018, 11:36 AM
metzman updated this revision to Diff 161783.Aug 21 2018, 11:40 AM
metzman edited the summary of this revision. (Show Details)Aug 21 2018, 11:50 AM
metzman edited the summary of this revision. (Show Details)
metzman edited the summary of this revision. (Show Details)Aug 21 2018, 12:40 PM
metzman changed the visibility from "metzman (Jonathan Metzman)" to "Public (No Login Required)".
metzman updated this revision to Diff 161797.Aug 21 2018, 12:47 PM

Run clang-format

metzman added subscribers: kcc, morehouse.

@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.

metzman edited the summary of this revision. (Show Details)Aug 21 2018, 12:50 PM
morehouse added inline comments.Aug 21 2018, 2:12 PM
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
815 ↗(On Diff #161797)

Nit: else is unnecessary.

morehouse added inline comments.Aug 21 2018, 2:12 PM
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 updated this revision to Diff 161850.Aug 21 2018, 5:14 PM
metzman marked 2 inline comments as done.

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

metzman added inline comments.Aug 21 2018, 5:29 PM
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 added inline comments.Aug 21 2018, 6:12 PM
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 updated this revision to Diff 161876.Aug 21 2018, 7:07 PM

Make lit test minimal

metzman marked an inline comment as done.Aug 21 2018, 7:09 PM
metzman added inline comments.
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 updated this revision to Diff 161987.Aug 22 2018, 10:16 AM
metzman marked an inline comment as done.

Replace solution for stack-depth with a prettier one.

metzman edited the summary of this revision. (Show Details)Aug 22 2018, 10:37 AM
morehouse added inline comments.Aug 22 2018, 10:44 AM
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 marked 2 inline comments as done.Aug 22 2018, 10:59 AM
metzman added inline comments.
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 added inline comments.Aug 22 2018, 11:11 AM
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 updated this revision to Diff 162052.Aug 22 2018, 2:14 PM

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

metzman edited the summary of this revision. (Show Details)Aug 22 2018, 2:16 PM
metzman updated this revision to Diff 162054.Aug 22 2018, 2:19 PM

Update comments

metzman marked 3 inline comments as done.Aug 22 2018, 2:25 PM
metzman added inline comments.
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.

morehouse accepted this revision.Aug 22 2018, 3:09 PM
morehouse added a reviewer: rnk.
This revision is now accepted and ready to land.Aug 22 2018, 3:09 PM

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 added inline comments.Aug 22 2018, 4:55 PM
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.

@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 updated this revision to Diff 162190.Aug 23 2018, 8:27 AM

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

morehouse added inline comments.Aug 23 2018, 9:07 AM
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 updated this revision to Diff 162207.Aug 23 2018, 9:47 AM
metzman marked an inline comment as done.

Undo unnecessary change to unittests.

metzman added inline comments.Aug 23 2018, 10:36 AM
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 added inline comments.Aug 23 2018, 3:31 PM
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 retitled this revision from Port libFuzzer to Windows to [libFuzzer] Port to Windows.Aug 23 2018, 4:57 PM
metzman updated this revision to Diff 162392.Aug 24 2018, 9:12 AM

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

metzman marked an inline comment as done.Aug 24 2018, 9:28 AM
metzman added inline comments.
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 updated this revision to Diff 162398.Aug 24 2018, 9:41 AM
metzman marked an inline comment as done.

run clang format.

metzman updated this revision to Diff 162415.Aug 24 2018, 10:36 AM
rnk added a comment.Aug 27 2018, 2:36 PM

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 updated this revision to Diff 162760.Aug 27 2018, 3:17 PM

Merge SCOVP section with .rdata

rnk accepted this revision.Aug 27 2018, 3:19 PM

lgtm

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 added a comment.Aug 27 2018, 5:18 PM

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!

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

metzman updated this revision to Diff 162784.Aug 27 2018, 6:02 PM

Disable libFuzzer tests on Windows.

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 updated this revision to Diff 162786.Aug 27 2018, 6:15 PM
metzman updated this revision to Diff 162787.Aug 27 2018, 6:18 PM
metzman edited the summary of this revision. (Show Details)

Improve comment

metzman updated this revision to Diff 162852.Aug 28 2018, 7:08 AM

Improve comments.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: Restricted Project. · View Herald Transcript
metzman updated this revision to Diff 163093.Aug 29 2018, 8:11 AM
metzman removed a subscriber: llvm-commits.

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

metzman updated this revision to Diff 163097.Aug 29 2018, 8:14 AM

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

metzman reopened this revision.Aug 30 2018, 8:09 AM
This revision is now accepted and ready to land.Aug 30 2018, 8:09 AM
metzman updated this revision to Diff 163338.Aug 30 2018, 8:10 AM
  • Port libFuzzer to Windows
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.