Enable extra coverage counters on Windows

[MichaelSquires]

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.

[MichaelSquires]

Formatted with clang-format

[MichaelSquires]

Formatted with clang-format

[rnk]

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.

[MichaelSquires]

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.

[MichaelSquires]

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]

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

[kcc]

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.

[MichaelSquires]

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

[morehouse]

LGTM

[kcc]

compiler-rt/test/fuzzer/extra-counters.test

1 ↗

(On Diff #363793)

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

[MichaelSquires]

Mark extra counters test as unsupported on Darwin.

[MichaelSquires]

compiler-rt/test/fuzzer/extra-counters.test

1 ↗

(On Diff #363793)

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

[MichaelSquires]

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

[mstorsjo]

compiler-rt/lib/fuzzer/FuzzerExtraCountersWindows.cpp

15 ↗

(On Diff #364563)

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

compiler-rt/lib/fuzzer/FuzzerExtraCounters.cpp

 
 }  // namespace fuzzer
 
+#elif LIBFUZZER_WINDOWS
+
+namespace fuzzer {
+
+//
+// The __start___libfuzzer_extra_counters variable is size 16 to account for
+// 16-byte alignment on amd64 and 8-byte alignment on x86. This means the next
+// variable in this section (either __libfuzzer_extra_counters or
+// __stop___libfuzzer_extra_counters) will be located at
+// (__start___libfuzzer_extra_counters +
+// sizeof(__start___libfuzzer_extra_counters)).
+//
+// The section name, __libfuzzer_extra_counterr ends with "rr", so it is one
+// less than __libfuzzer_extra_counters ("rs") alphabetically. We want the start
+// symbol to be placed in the section just before the user supplied counters (if
+// present).
+//
+#pragma section(".data$__libfuzzer_extra_counterr")
+__declspec(allocate(".data$__libfuzzer_extra_counterr")) uint8_t
+    __start___libfuzzer_extra_counters[16] = {0};
+
+//
+// Example of what the user-supplied counters should look like. First, the
+// pragma to create the section name. It will fall alphabetically between
+// ".data$__libfuzzer_extra_counterr" and ".data$__libfuzzer_extra_countert".
+// Next, the declspec to allocate the variable inside the specified section.
+// Finally, some array, struct, whatever that is used to track the counter data.
+// The size of this variable is dynamically computed by finding the difference
+// of __stop___libfuzzer_extra_counters and __start___libfuzzer_extra_counters.
+//
+
+//
+//     #pragma section(".data$__libfuzzer_extra_counters")
+//     __declspec(allocate(".data$__libfuzzer_extra_counters"))
+//         uint8_t any_name_variable[64 * 1024];
+//
+
+//
+// Here, the section name, __libfuzzer_extra_countert ends with "rt", so it
+// is one more than __libfuzzer_extra_counters ("rs") alphabetically. We want
+// the stop symbol to be placed in the section just after the user supplied
+// counters (if present).
+//
+#pragma section(".data$__libfuzzer_extra_countert")
+__declspec(allocate(".data$__libfuzzer_extra_countert")) uint8_t
+    __stop___libfuzzer_extra_counters = 0;
+
+uint8_t *ExtraCountersBegin() {
+return __start___libfuzzer_extra_counters +
+       sizeof(__start___libfuzzer_extra_counters);
+}
+
+uint8_t *ExtraCountersEnd() { return &__stop___libfuzzer_extra_counters; }
+
+ATTRIBUTE_NO_SANITIZE_ALL
+void ClearExtraCounters() { // hand-written memset, don't asan-ify.
+  uintptr_t *Beg = reinterpret_cast<uintptr_t *>(ExtraCountersBegin());
+  uintptr_t *End = reinterpret_cast<uintptr_t *>(ExtraCountersEnd());
+  for (; Beg < End; Beg++) {
+    *Beg = 0;
+  }
+}
+
+} // namespace fuzzer
+
 #else
 // TODO: implement for other platforms.
 namespace fuzzer {