[libFuzzer] Enable for SystemZ

[iii]

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.

[iii]

Fixed Reviewers: tag.

[kcc]

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

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?

[iii]

Ok, I'll split the patch.

compiler-rt/lib/fuzzer/FuzzerTracePC.h

198

I think this might not work as expected on 32-bit systems. I'll add a separate function.

[kcc]

compiler-rt/lib/fuzzer/FuzzerTracePC.h

198

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]

compiler-rt/lib/fuzzer/FuzzerTracePC.h

198

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.

[kcc]

compiler-rt/lib/fuzzer/FuzzerTracePC.h

198

Sounds good, thanks!

[iii]

Split in three commits, removed #ifdef from ForEachNonZeroByte() body.

[iii]

Second attempt to push all 3 commits with arcanist. Sorry for the noise
if it doesn't work out :-/

[iii]

arc which origin/master tells me that arc diff origin/master will
send all the 3 commits. Fingers crossed.

[iii]

• [libFuzzer] Fix endianness issue in ForEachNonZeroByte()
• [libFuzzer] Fix types of stop* and start* symbols
• [libFuzzer] Enable for SystemZ

Somehow having Differential Revision: in commit descriptions prevented arcanist from sending all commits. Removing it helped.

[iii]

Two minor fixes:

• Remove unneeded <endian.h> include.
• Make clang-tidy happy by renaming HToLE to Htole.

[iii]

• Htole -> htole (I missed clang-tidy suggestion)
• [libFuzzer] -> [SanitizerCoverage] in the 2nd patch title

[iii]

@kcc Could you have another look please?

[kcc]

Please fix two nits, then good to go.
Thanks!

compiler-rt/lib/fuzzer/FuzzerTracePC.h

196–200

please remove the { that weren't there.

compiler-rt/lib/fuzzer/FuzzerUtil.h

110 ↗

(On Diff #282179)

Maybe name it HostToLE?

[iii]

• Remove unnecessary {}
• htole -> HostToLE

[iii]

compiler-rt/lib/fuzzer/FuzzerTracePC.h

196–200

ok

compiler-rt/lib/fuzzer/FuzzerUtil.h

110 ↗

(On Diff #282179)

Ok - clang-tidy might not like it, but let's see.

[iii]

compiler-rt/lib/fuzzer/FuzzerUtil.h

110 ↗

(On Diff #282179)

Yeah: invalid case style for function 'HostToLE'. Suggested name is 'hostToLe'.
Would it be ok to commit with this warning?

[kcc]

compiler-rt/lib/fuzzer/FuzzerUtil.h

110 ↗

(On Diff #282179)

I don't mind.
hostToLe is fine too.

[iii]

I've committed with HostToLE, since it matches the existing code style.

Thanks for the review!

compiler-rt/cmake/config-ix.cmake

 
 if(ANDROID)
   set(OS_NAME "Android")
 else()
   set(OS_NAME "${CMAKE_SYSTEM_NAME}")
 endif()
 
 if(OS_NAME MATCHES "Linux")
-  set(ALL_FUZZER_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM64})
+  set(ALL_FUZZER_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM64} ${S390X})
 elseif (OS_NAME MATCHES "Windows")
   set(ALL_FUZZER_SUPPORTED_ARCH ${X86} ${X86_64})
 elseif(OS_NAME MATCHES "Android")
   set(ALL_FUZZER_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64})
 else()
   set(ALL_FUZZER_SUPPORTED_ARCH ${X86_64} ${ARM64})
 endif()
 

compiler-rt/lib/fuzzer/FuzzerTracePC.h

size_t ForEachNonZeroByte(const uint8_t *Begin, const uint8_t *End,
   const size_t StepMask = Step - 1;
   auto P = Begin;
   // Iterate by 1 byte until either the alignment boundary or the end.
   for (; reinterpret_cast<uintptr_t>(P) & StepMask && P < End; P++)
     if (uint8_t V = *P)
       Handle8bitCounter(FirstFeature, P - Begin, V);
 
   // Iterate by Step bytes at a time.
-  for (; P < End; P += Step)
-    if (LargeType Bundle = *reinterpret_cast<const LargeType *>(P))
+  for (; P < End; P += Step) {
+    if (LargeType Bundle = *reinterpret_cast<const LargeType *>(P)) {
+#if __BYTE_ORDER == __BIG_ENDIAN
+      Bundle = Bswap(Bundle);
+#endif
       for (size_t I = 0; I < Step; I++, Bundle >>= 8)
         if (uint8_t V = Bundle & 0xff)
           Handle8bitCounter(FirstFeature, P - Begin + I, V);
+    }
+  }
 
   // Iterate by 1 byte until the end.
   for (; P < End; P++)
     if (uint8_t V = *P)
       Handle8bitCounter(FirstFeature, P - Begin, V);
   return End - Begin;
 }
 

compiler-rt/test/fuzzer/coverage.test

 # FIXME: Disabled on Windows because -fPIC cannot be used to compile for Windows.
 UNSUPPORTED: windows
+# FIXME: CreatePCArray() emits PLT stub addresses for entry blocks, which are ignored by TracePC::PrintCoverage().
+XFAIL: s390x
 RUN: %cpp_compiler -mllvm -use-unknown-locations=Disable  %S/NullDerefTest.cpp -o %t-NullDerefTest
 RUN: %cpp_compiler -mllvm -use-unknown-locations=Disable %S/DSO1.cpp -fPIC %ld_flags_rpath_so1 -O0 -shared -o %dynamiclib1
 RUN: %cpp_compiler -mllvm -use-unknown-locations=Disable %S/DSO2.cpp -fPIC %ld_flags_rpath_so2 -O0 -shared -o %dynamiclib2
 RUN: %cpp_compiler -mllvm -use-unknown-locations=Disable %S/DSOTestMain.cpp %S/DSOTestExtra.cpp %ld_flags_rpath_exe1 %ld_flags_rpath_exe2 -o %t-DSOTest
 
 CHECK: COVERAGE:
 CHECK: COVERED_FUNC: {{.*}}LLVMFuzzerTestOneInput {{.*}}NullDerefTest.cpp:14
 RUN: not %run %t-NullDerefTest -print_coverage=1 2>&1 | FileCheck %s

compiler-rt/test/fuzzer/msan.test

+FIXME: Fails to find BINGO on s390x.
+XFAIL: s390x
 REQUIRES: msan
 RUN: %msan_compiler %S/SimpleTestStdio.cpp -o %t
 RUN: not %run %t -seed=1 -runs=10000000 2>&1 | FileCheck %s --check-prefix=NO-REPORT
 
 RUN: %msan_compiler %S/SimpleCmpTest.cpp -o %t
 RUN: not %run %t -seed=1 -runs=10000000 2>&1 | FileCheck %s --check-prefix=NO-REPORT
 
 RUN: %msan_compiler %S/MemcmpTest.cpp -o %t

compiler-rt/test/fuzzer/swap-cmp.test

+# FIXME: Fails to find BINGO with ASAN on s390x, work fine without ASAN.
+XFAIL: s390x
 RUN: %cpp_compiler %S/SwapCmpTest.cpp -o %t-SwapCmpTest
 CHECK: BINGO
 RUN: not %run %t-SwapCmpTest -seed=1 -runs=10000000 2>&1 | FileCheck %s

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

PreservedAnalyses ModuleSanitizerCoveragePass::run(Module &M,
   if (ModuleSancov.instrumentModule(M, DTCallback, PDTCallback))
     return PreservedAnalyses::none();
   return PreservedAnalyses::all();
 }
 
 std::pair<Value *, Value *>
 ModuleSanitizerCoverage::CreateSecStartEnd(Module &M, const char *Section,
                                            Type *Ty) {
-  GlobalVariable *SecStart =
-      new GlobalVariable(M, Ty, false, GlobalVariable::ExternalLinkage, nullptr,
-                         getSectionStart(Section));
+  GlobalVariable *SecStart = new GlobalVariable(
+      M, Ty->getPointerElementType(), false, GlobalVariable::ExternalLinkage,
+      nullptr, getSectionStart(Section));
   SecStart->setVisibility(GlobalValue::HiddenVisibility);
-  GlobalVariable *SecEnd =
-      new GlobalVariable(M, Ty, false, GlobalVariable::ExternalLinkage,
-                         nullptr, getSectionEnd(Section));
+  GlobalVariable *SecEnd = new GlobalVariable(
+      M, Ty->getPointerElementType(), false, GlobalVariable::ExternalLinkage,
   SecEnd->setVisibility(GlobalValue::HiddenVisibility);
   IRBuilder<> IRB(M.getContext());
-  Value *SecEndPtr = IRB.CreatePointerCast(SecEnd, Ty);
   if (!TargetTriple.isOSBinFormatCOFF())
-    return std::make_pair(IRB.CreatePointerCast(SecStart, Ty), SecEndPtr);
+    return std::make_pair(SecStart, SecEnd);
 
   // Account for the fact that on windows-msvc __start_* symbols actually
   // point to a uint64_t before the start of the array.
   auto SecStartI8Ptr = IRB.CreatePointerCast(SecStart, Int8PtrTy);
   auto GEP = IRB.CreateGEP(Int8Ty, SecStartI8Ptr,
                            ConstantInt::get(IntptrTy, sizeof(uint64_t)));
-  return std::make_pair(IRB.CreatePointerCast(GEP, Ty), SecEndPtr);
+  return std::make_pair(IRB.CreatePointerCast(GEP, Ty), SecEnd);
 }
 
 Function *ModuleSanitizerCoverage::CreateInitCallsForSections(
     Module &M, const char *CtorName, const char *InitFunctionName, Type *Ty,
     const char *Section) {
   auto SecStartEnd = CreateSecStartEnd(M, Section, Ty);
   auto SecStart = SecStartEnd.first;
   auto SecEnd = SecStartEnd.second;

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

 ; Test -sanitizer-coverage-inline-8bit-counters=1
 ; RUN: opt < %s -sancov -sanitizer-coverage-level=1 -sanitizer-coverage-inline-8bit-counters=1  -S -enable-new-pm=0 | FileCheck %s
 ; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=1 -sanitizer-coverage-inline-8bit-counters=1  -S | FileCheck %s
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
 target triple = "x86_64-unknown-linux-gnu"
 define void @foo() {
 entry:
 ; CHECK: section "__sancov_cntrs", comdat($foo), align 1
 ; CHECK:  %0 = load i8, i8* getelementptr inbounds ([1 x i8], [1 x i8]* @__sancov_gen_, i64 0, i64 0), align 1, !nosanitize
 ; CHECK:  %1 = add i8 %0, 1
 ; CHECK:  store i8 %1, i8* getelementptr inbounds ([1 x i8], [1 x i8]* @__sancov_gen_, i64 0, i64 0), align 1, !nosanitize
   ret void
 }
-; CHECK: call void @__sanitizer_cov_8bit_counters_init(i8* bitcast (i8** @__start___sancov_cntrs to i8*), i8* bitcast (i8** @__stop___sancov_cntrs to i8*))
+; CHECK: call void @__sanitizer_cov_8bit_counters_init(i8* @__start___sancov_cntrs, i8* @__stop___sancov_cntrs)

llvm/test/Instrumentation/SanitizerCoverage/inline-bool-flag.ll

 ; CHECK-NEXT:    store i1 true, i1* getelementptr inbounds ([1 x i1], [1 x i1]* @__sancov_gen_, i64 0, i64 0), align 1, !nosanitize !1
 ; CHECK-NEXT:    br label [[TMP3]]
 ; CHECK:       3:
 ; CHECK-NEXT:    ret void
 ;
 entry:
   ret void
 }
-; CHECK: call void @__sanitizer_cov_bool_flag_init(i1* bitcast (i1** @__start___sancov_bools to i1*), i1* bitcast (i1** @__stop___sancov_bools to i1*))
+; CHECK: call void @__sanitizer_cov_bool_flag_init(i1* @__start___sancov_bools, i1* @__stop___sancov_bools)

llvm/test/Instrumentation/SanitizerCoverage/tracing.ll

 ; CHECK_PC_GUARD-LABEL: @cond_longjmp
 ; CHECK_PC_GUARD: call void @__sanitizer_cov_trace_pc_guard
 ; CHECK_PC_GUARD: call void @__sanitizer_cov_trace_pc_guard
 ; CHECK_PC_GUARD: ret void
 ; CHECK_PC_GUARD: call void @__sanitizer_cov_trace_pc_guard
 ; CHECK_PC_GUARD: call void @longjmp
 ; CHECK_PC_GUARD: unreachable
 
-; CHECK_PC_GUARD: call void @__sanitizer_cov_trace_pc_guard_init(i32* bitcast (i32** @__start___sancov_guards to i32*), i32* bitcast (i32** @__stop___sancov_guards to i32*))
+; CHECK_PC_GUARD: call void @__sanitizer_cov_trace_pc_guard_init(i32* @__start___sancov_guards, i32* @__stop___sancov_guards)
 
 ; CHECK_PC_GUARD_DARWIN-LABEL: define void @foo
 ; CHECK_PC_GUARD_DARWIN: call void @__sanitizer_cov_trace_pc_guard
 ; CHECK_PC_GUARD_DARWIN: call void @__sanitizer_cov_trace_pc_guard
 ; CHECK_PC_GUARD_DARWIN: call void @__sanitizer_cov_trace_pc_guard
 ; CHECK_PC_GUARD_DARWIN-NOT: call void @__sanitizer_cov_trace_pc
 ; CHECK_PC_GUARD_DARWIN: ret void
-; CHECK_PC_GUARD_DARWIN: call void @__sanitizer_cov_trace_pc_guard_init(i32* bitcast (i32** @"\01section$start$__DATA$__sancov_guards" to i32*), i32* bitcast (i32** @"\01section$end$__DATA$__sancov_guards" to i32*))
+; CHECK_PC_GUARD_DARWIN: call void @__sanitizer_cov_trace_pc_guard_init(i32* @"\01section$start$__DATA$__sancov_guards", i32* @"\01section$end$__DATA$__sancov_guards")