This is an archive of the discontinued LLVM Phabricator instance.

Enable extra coverage counters on Windows
ClosedPublic

Authored by MichaelSquires on Jul 23 2021, 8:55 AM.

Details

Summary
  • Enable extra coverage counters on Windows.
  • Update extra_counters.test to run on Windows also.
  • Update TableLookupTest.cpp to include the required pragma/declspec for the extra coverage counters.

Diff Detail

Event Timeline

MichaelSquires requested review of this revision.Jul 23 2021, 8:55 AM
MichaelSquires created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2021, 8:55 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Formatted with clang-format

Formatted with clang-format

timurrrr edited reviewers, added: kcc, rnk; removed: timurrrr, samsonov.Jul 23 2021, 10:49 AM
rnk added a comment.Jul 23 2021, 11:41 AM

Seems reasonable to me, I have no familiarity with the extra counters feature, though. You might consider using explicit alignments.

Something else to consider is that the MSVC incremental linker adds padding between variables to allow resizing them between links. I think these days we just don't care about that anymore. In the past, we did work to make sure asan would work under those conditions.

compiler-rt/lib/fuzzer/FuzzerExtraCounters.cpp
46–48

Maybe __libfuzzer_extra_counter_a / __libfuzzer_extra_counter_z would make that clearer. "r, s, t" seems like a surprising part of the alphabet to use when establishing an ordering with lexicographic sorting.

89

Standard optimizations like loop idiom recognition will undo this. Is this enough to prevent ASan instrumentation? I'd suggest making these pointers volatile to be really confident that this won't become instrumented in the future.

Updates based on code review.

  • Added explicit alignment to start and stop sections.
  • Updated comments to better explain the alignment/padding requirements of the sections.
  • Update the section names to be more intuitive with regards to sorting.
  • Updated manual memset to use SecureZeroMemory to ensure it doesn't get optimized out.

Ping.

I made updates based on recommendations from @rnk.

I'm also realizing I likely don't have commit access and would appreciate if someone could commit on my behalf (assuming review approval).

Thanks!

MichaelSquires marked 2 inline comments as done.Aug 3 2021, 6:01 AM

Marking previous comments as done. I didn't realize this was something I could do - sorry, first time using this code review system.

kcc added a comment.Aug 3 2021, 7:48 AM

One comment that I have is a request to limit the number of #ifdefs in the code to at most one.
We typically achieve this by having platform-specific code in a platform-specific file, guarded with an ifdef.

an extra ifdef in a test is probably unavoidable.

Moved platform specific code into separate files per request from @kcc.

This revision is now accepted and ready to land.Aug 3 2021, 10:31 AM
kcc added inline comments.Aug 3 2021, 12:45 PM
compiler-rt/test/fuzzer/extra-counters.test
1

does this imply that the test will now work on Mac?

Mark extra counters test as unsupported on Darwin.

MichaelSquires marked an inline comment as done.Aug 3 2021, 1:22 PM
MichaelSquires added inline comments.
compiler-rt/test/fuzzer/extra-counters.test
1

No. Good catch, thanks! I updated the test to be unsupported on darwin.

MichaelSquires marked an inline comment as done.Aug 4 2021, 6:15 AM

Can someone commit this for me please? I don't have access.

This revision was automatically updated to reflect the committed changes.
mstorsjo added inline comments.
compiler-rt/lib/fuzzer/FuzzerExtraCountersWindows.cpp
15

This broke cross building from case sensitive filesystems with mingw; I pushed a fix for it in ab737d5367cdea30df71ef66f87fe2c6d88e7246.