Download Raw Diff

Details

Reviewers

kcc
morehouse
uweigand
jonpa

Commits

rG153df1373e5d: [SanitizerCoverage] Fix types of __stop* and __start* symbols
rGa4e537d9c47a: [libFuzzer] Fix endianness issue in ForEachNonZeroByte()
rGea9b82da4180: [libFuzzer] Enable for SystemZ

Summary

Add SystemZ to the list of supported architectures.

Fix endianness issue in ForEachNonZeroByte().

Fix types of stop* and start* symbols. Adapt tests.

If a section is supposed to hold elements of type T, then the
corresponding CreateSecStartEnd()'s Ty parameter represents T*.
Forwarding it to GlobalVariable constructor causes the resulting
GlobalVariable's type to be T*, and its SSA value type to be T**, which
is one indirection too many. This issue is mostly masked by pointer
casts, however, the global variable still gets an incorrect alignment,
which causes SystemZ to choose wrong instructions to access the
section.

XFAIL a few tests.

Coverage reporting is broken, and is not easy to fix (see comment in
coverage.test). Interaction with sanitizers needs to be investigated
more thoroughly, since they appear to reduce coverage in certain cases.

Diff Detail

Repository: rG LLVM Github Monorepo

Unit TestsFailed

	Time	Test
	70 ms	linux > LLVM.Instrumentation/SanitizerCoverage::inline-8bit-counters.ll
	70 ms	linux > LLVM.Instrumentation/SanitizerCoverage::inline-bool-flag.ll
	100 ms	linux > LLVM.Instrumentation/SanitizerCoverage::tracing.ll
	100 ms	windows > LLVM.Instrumentation/SanitizerCoverage::inline-8bit-counters.ll
	80 ms	windows > LLVM.Instrumentation/SanitizerCoverage::inline-bool-flag.ll
		View Full Test Results (6 Failed)

Event Timeline

iii created this revision.Jul 30 2020, 12:51 AM

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 30 2020, 12:51 AM

Herald added subscribers: llvm-commits, Restricted Project, hiraditya, mgorny. · View Herald Transcript

Fixed Reviewers: tag.

iii edited the summary of this revision. (Show Details)Jul 30 2020, 12:53 AM

iii added reviewers: kcc, morehouse, uweigand, jonpa.

Harbormaster completed remote builds in B66331: Diff 281818.Jul 30 2020, 1:55 AM

Kai added a subscriber: Kai.Jul 30 2020, 2:05 AM

Harbormaster completed remote builds in B66330: Diff 281816.Jul 30 2020, 2:14 AM

The compiler change seems to be completely independent from the libFuzzer change.
Please split this change into two.

compiler-rt/lib/fuzzer/FuzzerTracePC.h
198 ↗	(On Diff #281818)	please avoid #ifdefs inside functions. If there is some logic that needs to depend on the platform, it needs to reside in a separate dedicated function. FuzzerBuiltins.h is probably the place for such a function. Also, what about #include <endian.h> uint64_t htole64(uint64_t host_64bits); is that portable?

Ok, I'll split the patch.

compiler-rt/lib/fuzzer/FuzzerTracePC.h
198 ↗	(On Diff #281818)	I think this might not work as expected on 32-bit systems. I'll add a separate function.

kcc added inline comments.Jul 30 2020, 9:20 AM

compiler-rt/lib/fuzzer/FuzzerTracePC.h
198 ↗	(On Diff #281818)	No strong preference. I've just checked, on Linux x86_64: it inlines and works great. Dunno if it will have portability issues. % cat x.cc #include <endian.h> #include <stdint.h> uint64_t foo(uint64_t x) { return htole64(x); } % clang -O2 -S -o - x.cc -m32 movl 4(%esp), %eax movl 8(%esp), %edx retl % clang -O2 -S -o - x.cc movq %rdi, %rax retq `htobe64` also looks nice, it translates into one `bswapq` on 64-bit and two `bswapl` on 32-bit, i.e. in your case this is what `htole64` will do.

iii added inline comments.Jul 30 2020, 9:28 AM

compiler-rt/lib/fuzzer/FuzzerTracePC.h
198 ↗	(On Diff #281818)	My thinking is that LargeType is uintptr_t, so it will be uint32_t on a 32-bit system. So if we have something like uint32_t Bundle = 0xAABBCCDD; htole64(Bundle) would give us (uint64_t)0xDDCCBBAA00000000, and then when we cast it back to uint32_t, we'll get just 0. Isn't that right? So the idea I'm currently regtesting is: --- a/compiler-rt/lib/fuzzer/FuzzerUtil.h +++ b/compiler-rt/lib/fuzzer/FuzzerUtil.h @@ -106,6 +106,12 @@ inline uint8_t RoundDownByPage(uint8_t P) { return reinterpret_cast<uint8_t *>(X); } +#if __BYTE_ORDER == __LITTLE_ENDIAN +template <typename T> T HToLE(T X) { return X; } +#else +template <typename T> T HToLE(T X) { return Bswap(X); } +#endif + } // namespace fuzzer #endif // LLVM_FUZZER_UTIL_H ... it's in FuzzerUtil.h and not in FuzzerBuiltins.h, because it uses Bswap, which is defined in a separate header for MSVC.