[XRay][Arm] Enable back XRay testing on Arm32 and fix the failing tests

[rSerge]

Summary

Testing of XRay was occasionally disabled on 32-bit Arm targets (someone assumed that XRay was supported on 64-bit targets only). This patch should fix that problem. Also here the instruction&data cache incoherency problem is fixed, because it may be causing a test to fail.
This patch is one of a series: see also

• https://reviews.llvm.org/D28624

[dberris]

Yes, that was me assuming that XRay wouldn't be supported in 32-bit systems. But I'm happy to be proven wrong there. :D

test/xray/lit.cfg

35

I'm not too familiar with the arm triples, but don't we only support xray on arm7? Does that distinction matter?

[rSerge]

test/xray/lit.cfg

35

AFAIK, build-bots are currently only armv7 . Furthermore, I understood that tests can only execute on architectures in ALL_XRAY_SUPPORTED_ARCH from llvm\projects\compiler-rt\cmake\config-ix.cmake , and this makes me question why 64-bitness is checked here again...

[rengolin]

test/xray/lit.cfg

35

FWIW, all bots are ARMv7 and ARMv8. But that can break if someone is running the tests on, for example, a Raspberry Pi 1.

[rengolin]

In D28623#644550, @dberris wrote:

Yes, that was me assuming that XRay wouldn't be supported in 32-bit systems. But I'm happy to be proven wrong there. :D

Well, before we get this in, it would be good to make sure they actually pass all the tests on an ARMv7 system. I can run it locally on a system like the buildbots to be sure.

[rSerge]

@rengolin , yes, please. This patch should be tested together with https://reviews.llvm.org/D28624 , because this patch only enables the tests and that one repairs XRay on Arm32.

test/xray/lit.cfg

35

Oh, I meant "at least armv7". Armv8 should be ok. Armv6 is not fully supported by XRay (because movw and movt are written to the code at runtime).

[rSerge]

test/xray/lit.cfg

35

Actually, I'm not sure about armv6, may be it's ok because movw and movt appear in armv6t2 .

[rengolin]

test/xray/lit.cfg

35

Raspberry Pi 1 should be fine, then.

[rengolin]

I just tested on ARM and got these errors:

TestCases/Linux/patching-unpatching.cc

patching-unpatching.cc:28:17: error: expected string not found in input
// CHECK-NEXT: patching status: 1
               ^
<stdin>:3:1: note: scanning from here
patching status: 0
^

TestCases/Linux/argv0-log-file-name.cc

xray.log.file.name:2:11: error: expected string not found in input
// CHECK: xray-log.argv0-log-file-name.cc.tmp.{{.*}}
          ^
<stdin>:1:1: note: scanning from here
Output
^
<stdin>:2:1: note: possible intended match here
xray.log.file.name
^

TestCases/Linux/fixedsize-logging.cc

fixedsize-logging.cc:17:12: error: expected string not found in input
// CHECK: XRay: Log file in 'fixedsize-logging-{{.*}}'
          ^
<stdin>:1:1: note: scanning from here
==27375==XRay instrumentation map missing. Not initializing XRay.
^
<stdin>:1:38: note: possible intended match here
==27375==XRay instrumentation map missing. Not initializing XRay.
                                     ^

[rSerge]

@rengolin , did you first apply patch https://reviews.llvm.org/D28624 before testing the current patch?
Especially the last quoted failed test seems to behave as if emission of XRay table is still broken.

[rengolin]

In D28623#645269, @rSerge wrote:

@rengolin , did you first apply patch https://reviews.llvm.org/D28624 before testing the current patch?
Especially the last quoted failed test seems to behave as if emission of XRay table is still broken.

Dang, no. I'll do it.

[rengolin]

Almost. After applying both patches, I still get:

TestCases/Linux/patching-unpatching.cc

patching-unpatching.cc:30:17: error: expected string not found in input
 // CHECK-NEXT: called: {{.*}}, type=0
                ^
<stdin>:3:1: note: scanning from here
always instrumented called
^
<stdin>:3:21: note: possible intended match here
always instrumented called
                    ^

[rSerge]

In D28623#645392, @rengolin wrote:

Almost. After applying both patches, I still get:

TestCases/Linux/patching-unpatching.cc

patching-unpatching.cc:30:17: error: expected string not found in input
 // CHECK-NEXT: called: {{.*}}, type=0
                ^
<stdin>:3:1: note: scanning from here
always instrumented called
^
<stdin>:3:21: note: possible intended match here
always instrumented called
                    ^

Likely it is the same problem with ARM instruction&data cache incoherency we've seen on AArch64. Let me replicate that fix here too...

[rengolin]

In D28623#645398, @rSerge wrote:

Likely it is the same problem with ARM instruction&data cache incoherency we've seen on AArch64. Let me replicate that fix here too...

Makes sense.

[rSerge]

PTAL.

[rengolin]

Seems to work on my end. Thanks!

[rSerge]

Reached mainline in 292211. Thanks all!

[rSerge]

Added detection of -mthumb test compiler option, so to mark tests as unsupported on Thumb because XRay doesn't yet support Thumb CPU mode on Arm32.

[rengolin]

Makes sense. LGTM. Let's try again. :)

[rSerge]

With Thumb fix, in mainline at 292517.

lib/xray/xray_arm.cc

 //===----------------------------------------------------------------------===//
 #include "sanitizer_common/sanitizer_common.h"
 #include "xray_defs.h"
 #include "xray_emulate_tsc.h"
 #include "xray_interface_internal.h"
 #include <atomic>
 #include <cassert>
 
+extern "C" void __clear_cache(void* start, void* end);
+
 namespace __xray {
 
 uint64_t cycleFrequency() XRAY_NEVER_INSTRUMENT {
   // There is no instruction like RDTSCP in user mode on ARM.  ARM's CP15 does
   //   not have a constant frequency like TSC on x86[_64]; it may go faster or
   //   slower depending on CPU's turbo or power saving modes.  Furthermore, to
   //   read from CP15 on ARM a kernel modification or a driver is needed.
   //   We can not require this from users of compiler-rt.
inline static bool patchSled(const bool Enable, const uint32_t FuncId,
   // operation, so that the user code which reaches the sled concurrently
   // either jumps over the whole sled, or executes the whole sled when the
   // latter is ready.
   //
   // When |Enable|==false, we set back the first instruction in the sled to be
   //   B #20
 
   uint32_t *FirstAddress = reinterpret_cast<uint32_t *>(Sled.Address);
-  if (Enable) {
-    uint32_t *CurAddress = FirstAddress + 1;
+  if (Enable) {
     CurAddress =
         Write32bitLoadR0(CurAddress, reinterpret_cast<uint32_t>(FuncId));
     CurAddress =
         Write32bitLoadIP(CurAddress, reinterpret_cast<uint32_t>(TracingHook));
     *CurAddress = uint32_t(PatchOpcodes::PO_BlxIp);
     CurAddress++;
     *CurAddress = uint32_t(PatchOpcodes::PO_PopR0Lr);
+    CurAddress++;
     std::atomic_store_explicit(
         reinterpret_cast<std::atomic<uint32_t> *>(FirstAddress),
         uint32_t(PatchOpcodes::PO_PushR0Lr), std::memory_order_release);
   } else {
     std::atomic_store_explicit(
         reinterpret_cast<std::atomic<uint32_t> *>(FirstAddress),
         uint32_t(PatchOpcodes::PO_B20), std::memory_order_release);
   }
+  __clear_cache(reinterpret_cast<char*>(FirstAddress),
+      reinterpret_cast<char*>(CurAddress));
   return true;
 }
 
 bool patchFunctionEntry(const bool Enable, const uint32_t FuncId,
                         const XRaySledEntry &Sled) XRAY_NEVER_INSTRUMENT {
   return patchSled(Enable, FuncId, Sled, __xray_FunctionEntry);
 }
 

test/xray/lit.cfg

 config.substitutions.append(
     ('%clang_xray ', build_invocation(clang_xray_cflags)))
 config.substitutions.append(
     ('%clangxx_xray', build_invocation(clang_xray_cxxflags)))
 
 # Default test suffixes.
 config.suffixes = ['.c', '.cc', '.cpp']
 
-if config.host_os not in ['Linux'] or config.host_arch.find('64') == -1:
+if config.host_os not in ['Linux']:
+  config.unsupported = True
+elif '64' not in config.host_arch:
+  if 'arm' in config.host_arch:
+    if '-mthumb' in config.target_cflags:
+      config.unsupported = True
+  else:
     config.unsupported = True
 
 # Allow tests to use REQUIRES=stable-runtime.  For use when you cannot use XFAIL
 # e.g. because the test sometimes passes, sometimes fails.
 if config.target_arch != 'aarch64':
   config.available_features.add('stable-runtime')