[libFuzzer] Generalize the code for getting the previous offset for different architectures

[george.karpenkov]

Summary

Without this change, tests in coverage.test and dump_coverage.test are failing on non-x86_64 platforms.
The diff is copied from sanitizer_common library, an alternative would be to link it together with libFuzzer.

[george.karpenkov]

@morehouse After this and the linked change, it should be safe to re-enable coverage.test on AArch64.

[Dor1s]

I should admit that I'm not familiar with ARM offsets, but since the change fixes the test, LGTM.

compiler-rt/lib/fuzzer/FuzzerTracePC.cpp

202 ↗

(On Diff #168874)

Are you sure you need -3 here, not -2? I'm definitely not an expert here, so it's more like a sanity check question :)

>>> hex((0x122 - 3) & (~1))
'0x11e'
>>> hex((0x122 - 2) & (~1))
'0x120'

213 ↗

(On Diff #168874)

some comment here might be useful too

[Dor1s]

Btw, @george.karpenkov, have you every experienced anything similar to https://bugs.chromium.org/p/chromium/issues/detail?id=892167 ?

[george.karpenkov]

compiler-rt/lib/fuzzer/FuzzerTracePC.cpp

202 ↗

(On Diff #168874)

I've copied this code from sanitizer_common/sanitizer_stracktrace.h.

Another possibility is to link to sanitizer_common instead, but I think at one point @kcc was against that (has that changed?)

[george.karpenkov]

@Dor1s I have definitely seen this error, but at some point it disappeared. I do not recall exactly the circumstances, maybe it appeared when the compiler version did not match exactly the runtime version.

[george.karpenkov]

@Dor1s this can't land without D53039, does that look fine as well?

[morehouse]

compiler-rt/lib/fuzzer/FuzzerTracePC.cpp

221 ↗

(On Diff #168874)

Could we avoid putting ifdefs here? Maybe they could go in FuzzerDefs.h.

[george.karpenkov]

compiler-rt/lib/fuzzer/FuzzerTracePC.cpp

221 ↗

(On Diff #168874)

I don't think this is a good idea.

1. FuzzerDefs contains constants, this is not a constant.
2. As the code is cloned, it would be harder to maintain parity with the code in sanitizer_common if it's significantly changed.

[morehouse]

compiler-rt/lib/fuzzer/FuzzerTracePC.cpp

221 ↗

(On Diff #168874)

Ok, then let's add a comment saying where this code came from.

[george.karpenkov]

compiler-rt/lib/fuzzer/FuzzerTracePC.cpp

221 ↗

(On Diff #168874)

OK!

compiler-rt/trunk/lib/fuzzer/FuzzerTracePC.cpp

 ATTRIBUTE_NO_SANITIZE_ALL
 void TracePC::HandleCallerCallee(uintptr_t Caller, uintptr_t Callee) {
   const uintptr_t kBits = 12;
   const uintptr_t kMask = (1 << kBits) - 1;
   uintptr_t Idx = (Caller & kMask) | ((Callee & kMask) << kBits);
   ValueProfileMap.AddValueModPrime(Idx);
 }
 
+/// \return the address of the previous instruction.
+/// Note: the logic is copied from `sanitizer_common/sanitizer_stacktrace.h`
+inline ALWAYS_INLINE uintptr_t GetPreviousInstructionPc(uintptr_t PC) {
+#if defined(__arm__)
+  // T32 (Thumb) branch instructions might be 16 or 32 bit long,
+  // so we return (pc-2) in that case in order to be safe.
+  // For A32 mode we return (pc-4) because all instructions are 32 bit long.
+  return (PC - 3) & (~1);
+#elif defined(__powerpc__) || defined(__powerpc64__) || defined(__aarch64__)
+  // PCs are always 4 byte aligned.
+  return PC - 4;
+#elif defined(__sparc__) || defined(__mips__)
+  return PC - 8;
+#else
+  return PC - 1;
+#endif
+}
+
+/// \return the address of the next instruction.
+/// Note: the logic is copied from `sanitizer_common/sanitizer_stacktrace.cc`
+inline ALWAYS_INLINE uintptr_t GetNextInstructionPc(uintptr_t PC) {
+#if defined(__mips__)
+  return PC + 8;
+#elif defined(__powerpc__) || defined(__sparc__) || defined(__arm__) || \
+    defined(__aarch64__)
+  return PC + 4;
+#else
+  return PC + 1;
+#endif
+}
+
 void TracePC::UpdateObservedPCs() {
   Vector<uintptr_t> CoveredFuncs;
   auto ObservePC = [&](uintptr_t PC) {
     if (ObservedPCs.insert(PC).second && DoPrintNewPCs) {
-      PrintPC("\tNEW_PC: %p %F %L", "\tNEW_PC: %p", PC + 1);
+      PrintPC("\tNEW_PC: %p %F %L", "\tNEW_PC: %p", GetNextInstructionPc(PC));
       Printf("\n");
     }
   };
 
   auto Observe = [&](const PCTableEntry &TE) {
     if (TE.PCFlags & 1)
       if (++ObservedFuncs[TE.PC] == 1 && NumPrintNewFuncs)
         CoveredFuncs.push_back(TE.PC);
if (NumInline8bitCounters == NumPCsInPCTables) {
             Observe(ModulePCTable[i].Start[j]);
       }
     }
   }
 
   for (size_t i = 0, N = Min(CoveredFuncs.size(), NumPrintNewFuncs); i < N;
        i++) {
     Printf("\tNEW_FUNC[%zd/%zd]: ", i + 1, CoveredFuncs.size());
-    PrintPC("%p %F %L", "%p", CoveredFuncs[i] + 1);
+    PrintPC("%p %F %L", "%p", GetNextInstructionPc(CoveredFuncs[i]));
     Printf("\n");
   }
 }
 
-inline ALWAYS_INLINE uintptr_t GetPreviousInstructionPc(uintptr_t PC) {
-  // TODO: this implementation is x86 only.
-  // see sanitizer_common GetPreviousInstructionPc for full implementation.
-  return PC - 1;
-}
-
-inline ALWAYS_INLINE uintptr_t GetNextInstructionPc(uintptr_t PC) {
-  // TODO: this implementation is x86 only.
-  // see sanitizer_common GetPreviousInstructionPc for full implementation.
-  return PC + 1;
-}
 
 static std::string GetModuleName(uintptr_t PC) {
   char ModulePathRaw[4096] = "";  // What's PATH_MAX in portable C++?
   void *OffsetRaw = nullptr;
   if (!EF->__sanitizer_get_module_and_offset_for_pc(
       reinterpret_cast<void *>(PC), ModulePathRaw,
       sizeof(ModulePathRaw), &OffsetRaw))
     return "";

compiler-rt/trunk/test/fuzzer/coverage.test

 # FIXME: Disabled on Windows because -fPIC cannot be used to compile for Windows.
-UNSUPPORTED: aarch64, windows
+UNSUPPORTED: windows
 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 -shared -o %dynamiclib1
 RUN: %cpp_compiler -mllvm -use-unknown-locations=Disable %S/DSO2.cpp -fPIC %ld_flags_rpath_so2 -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:13
 RUN: not %run %t-NullDerefTest -print_coverage=1 2>&1 | FileCheck %s

compiler-rt/trunk/test/fuzzer/dump_coverage.test

 # FIXME: Disabled on Windows because -fPIC cannot be used to compile for Windows.
 UNSUPPORTED: freebsd, windows
 RUN: %cpp_compiler -fsanitize-coverage=0 -fsanitize-coverage=trace-pc-guard %S/DSO1.cpp -fPIC -shared -o %dynamiclib1 %ld_flags_rpath_so1
 RUN: %cpp_compiler -fsanitize-coverage=0 -fsanitize-coverage=trace-pc-guard %S/DSO2.cpp -fPIC -shared -o %dynamiclib2 %ld_flags_rpath_so2
 RUN: %cpp_compiler -fsanitize-coverage=0 -fsanitize-coverage=trace-pc-guard %S/DSOTestMain.cpp %S/DSOTestExtra.cpp %ld_flags_rpath_exe1 %ld_flags_rpath_exe2 -o %t-DSOTest
 
 RUN: %cpp_compiler -fsanitize-coverage=0 -fsanitize-coverage=trace-pc-guard %S/NullDerefTest.cpp -o %t-NullDerefTest
 
 RUN: rm -rf %t_workdir && mkdir -p %t_workdir
 RUN: env ASAN_OPTIONS=coverage_dir='"%t_workdir"' not %run %t-NullDerefTest -dump_coverage=1 2>&1 | FileCheck %s
-RUN: sancov -covered-functions %t-NullDerefTest* %t_workdir/*.sancov | FileCheck %s --check-prefix=SANCOV
+RUN: sancov -covered-functions %t-NullDerefTest %t_workdir/*.sancov | FileCheck %s --check-prefix=SANCOV
 RUN: env ASAN_OPTIONS=coverage_dir='"%t_workdir"' %run %t-DSOTest -dump_coverage=1 -runs=0 2>&1 | FileCheck -allow-deprecated-dag-overlap %s --check-prefix=DSO
 RUN: env ASAN_OPTIONS=coverage_dir='"%t_workdir"' not %run %t-NullDerefTest -dump_coverage=0 2>&1 | FileCheck %s --check-prefix=NOCOV
 
 CHECK: SanitizerCoverage: {{.*}}NullDerefTest.{{.*}}.sancov: {{.*}} PCs written
 SANCOV: LLVMFuzzerTestOneInput
 
 DSO: SanitizerCoverage: {{.*}}DSOTest.{{.*}}.sancov: {{.*}} PCs written
 DSO-DAG: SanitizerCoverage: {{.*}}.{{.*}}.sancov: {{.*}} PCs written
 DSO-DAG: SanitizerCoverage: {{.*}}2.{{.*}}.sancov: {{.*}} PCs written
 
 NOCOV-NOT: SanitizerCoverage: {{.*}} PCs written

compiler-rt/trunk/test/fuzzer/handle-unstable.test

 # Tests -handle_unstable
 # FIXME: Disabled on Windows until symbolization works properly.
-UNSUPPORTED: aarch64, windows
+UNSUPPORTED: windows
 
 RUN: %cpp_compiler %S/PrintUnstableStatsTest.cpp -o %t-HandleUnstableTest
 
 ; Normal
 RUN: %run %t-HandleUnstableTest -print_coverage=1 -runs=100000 2>&1 | FileCheck %s --check-prefix=NORMAL
 NORMAL-DAG: det0()
 NORMAL-DAG: det1()
 NORMAL-DAG: det2()