[XRay][AArch64] An attempt to fix test patching-unpatching.cc by flushing the instruction cache after code modification

[rSerge]

Summary

This patch attempts to fix test patching-unpatching.cc . The new code flushes the instruction cache after modifying the program at runtime.

[rengolin]

LGTM. Thanks!

[rSerge]

Reached mainline in https://reviews.llvm.org/rL290354 .

[rengolin]

This broke the build. Did you manage to build this on AArch64? How did you test this?

[iid_iunknown]

@rSerge You forgot to add a forward declaration for __clear_cache to make it visible like e.g. trampoline_setup.c does.

[rSerge]

In D27996#629921, @rengolin wrote:

This broke the build. Did you manage to build this on AArch64? How did you test this?

I cross-compiled compiler-rt project from x86_64-Windows to AArch64-Linux and then tested on QEmu-AArch64-Linux . I thought that the built-in doesn't need any header file inclusion. Apparently for different host platforms the built-in behaves differently, perhaps that's due to compiler version. I'll look into this.

[rengolin]

Right, maybe the forward declaration as Oleg said would work.

[rSerge]

As discussed here , presence or absence of __clear_cache built-in depends on the compiler options. The built-in is common to gcc and clang, just currently in clang it is implemented via a called function in compiler-rt, but (another version of) gcc may inline it as an intrinsic. As discussed here , earlier there was a bug in gcc that gcc was expecting the parameters to be void*, in contradiction to the documentation which says they are char*, and clang developers implemented this bug too (so that parameters are void*), and I understood that clang developers were then fixing this bug.

However, in compiler-rt, file clear_cache.c the function is still defined as void __clear_cache(void *start, void *end) { ... }, and it seems used in file trampoline_setup.c via a forward declaration also as extern void __clear_cache(void* start, void* end);.

So it seems that the best I can do to let the code compile with different gcc and clang versions is to forward-declare the function as extern void __clear_cache(void* start, void* end); and keep the reinterpret_cast<char*> so that the code accepts both the function and the intrinsic (but this may lead to ambiguity for some compiler version and flags).

[rSerge]

Added the forward declaration for ____clear_cache() to fix the build.

[rengolin]

This should work. Let's have another try.

[rSerge]

The second iteration has reached mainline in revision 290452 .

[rengolin]

Still undefined symbol:

http://lab.llvm.org:8011/builders/clang-cmake-aarch64-39vma/builds/2086/steps/ninja%20check%201/logs/FAIL%3A%20XRay-aarch64-linux%3A%3Afixedsize-logging.cc

undefined reference to `__clear_cache(void*, void*)'

Reverted in r290453.

Next step, we'll test native first before commit.

Happy holidays!

[rSerge]

I have been able to reproduce the problem when cross-compiling from x86_64-Linux to AArch64-Linux. Investigating now...

[rSerge]

At a minimum, during x86_64-Linux to AArch64-Linux cross-compilation it didn't compile because of extern "C" missing. And the compiled program runs well on QEmu-AArch64-Linux .
@rengolin , what do you mean by "testing native" ? If that's compiling LLVM on AArch64 machine (i.e. without cross-compilation), then it would be challenging for me to do because on QEmu a LLVM build takes about a week, and the OS in the Arm32/AArch64 machines our team has is not exactly Linux.

[rSerge]

Ping?

[rovka]

Hi,

Looks like this slipped through the cracks, I'll do a native run for you today and let you know how it goes.

Thanks.

[rSerge]

In D27996#641145, @rovka wrote:

Hi,

Looks like this slipped through the cracks, I'll do a native run for you today and let you know how it goes.

Thanks.

Thanks!

[rovka]

Hi,

Looks good on our test AArch64 machine, feel free to commit.

Diana

[rSerge]

Reached mainline in revision 291568.

lib/xray/xray_AArch64.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 #32
 
   uint32_t *FirstAddress = reinterpret_cast<uint32_t *>(Sled.Address);
-  if (Enable) {
-    uint32_t *CurAddress = FirstAddress + 1;
+  if (Enable) {
     *CurAddress = uint32_t(PatchOpcodes::PO_LdrW0_12);
     CurAddress++;
     *CurAddress = uint32_t(PatchOpcodes::PO_LdrX16_12);
     CurAddress++;
     *CurAddress = uint32_t(PatchOpcodes::PO_BlrX16);
     CurAddress++;
     *CurAddress = FuncId;
     CurAddress++;
     *reinterpret_cast<void (**)()>(CurAddress) = TracingHook;
     CurAddress += 2;
     *CurAddress = uint32_t(PatchOpcodes::PO_LdpX0X30SP_16);
+    CurAddress++;
     std::atomic_store_explicit(
         reinterpret_cast<std::atomic<uint32_t> *>(FirstAddress),
         uint32_t(PatchOpcodes::PO_StpX0X30SP_m16e), std::memory_order_release);
   } else {
     std::atomic_store_explicit(
         reinterpret_cast<std::atomic<uint32_t> *>(FirstAddress),
         uint32_t(PatchOpcodes::PO_B32), 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/TestCases/Linux/patching-unpatching.cc

 // Check that we can patch and un-patch on demand, and that logging gets invoked
 // appropriately.
 //
 // RUN: %clangxx_xray -fxray-instrument -std=c++11 %s -o %t
 // RUN: XRAY_OPTIONS="patch_premain=false" %run %t 2>&1 | FileCheck %s
-// REQUIRES: stable-runtime
 
 #include "xray/xray_interface.h"
 
 #include <cstdio>
 
 bool called = false;
 
 void test_handler(int32_t fid, XRayEntryType type) {