Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix missing ___clear_cache when targetting iOS #294

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MrCyjaneK
Copy link

Without the patch I get the following issue

Undefined symbols for architecture arm64:
  "___clear_cache", referenced from:
      void randomx::JitCompilerA64::generateSuperscalarHash<8ul>(randomx::SuperscalarProgram (&) [8ul], std::__1::vector<unsigned long long, std::__1::allocator<unsigned long long>>&) in librandomx.a[27](jit_compiler_a64.cpp.o)
      randomx::JitCompilerA64::JitCompilerA64() in librandomx.a[27](jit_compiler_a64.cpp.o)
      randomx::JitCompilerA64::generateProgram(randomx::Program&, randomx::ProgramConfiguration&) in librandomx.a[27](jit_compiler_a64.cpp.o)
      randomx::JitCompilerA64::generateProgramLight(randomx::Program&, randomx::ProgramConfiguration&, unsigned int) in librandomx.a[27](jit_compiler_a64.cpp.o)
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

I've found the very same fix used in xmrig/xmrig@10d2920

I didn't check the fix on iOS yet, but the library linked properly, so I guess it is good.

MrCyjaneK added a commit to MrCyjaneK/monero_c that referenced this pull request Apr 2, 2024
@SChernykh
Copy link
Collaborator

This is not a fix, you just removed (using #ifdef) the code that breaks the build. You still need to clear the code cache on arm64 CPUs. If you read the discussion in xmrig/xmrig#1246 and check the code in https://github.com/xmrig/xmrig/blob/master/src/crypto/common/VirtualMemory_unix.cpp#L206 you'll see there are more things needed to be done here to fix it.

I'm not sure this fix is even needed because all recent clang versions have __GNUC__ and __builtin___clear_cache defined and automatially translate it to the sys_icache_invalidate call.

@MrCyjaneK
Copy link
Author

This is not a fix, you just removed (using #ifdef) the code that breaks the build. You still need to clear the code cache on arm64 CPUs. If you read the discussion in xmrig/xmrig#1246 and check the code in https://github.com/xmrig/xmrig/blob/master/src/crypto/common/VirtualMemory_unix.cpp#L206 you'll see there are more things needed to be done here to fix it.

Sorry! I've missed that part of the discussion, is fix like this more suitable or should I leave __GNUC__ check in place?

#ifdef HAVE_BUILTIN_CLEAR_CACHE
__builtin___clear_cache(reinterpret_cast<char*>(code), reinterpret_cast<char*>(code + CodeSize));
#else
sys_icache_invalidate(code, code + CodeSize);
#endif

I'm not sure this fix is even needed because all recent clang versions have GNUC and __builtin___clear_cache defined and automatially translate it to the sys_icache_invalidate call.

I'm currently building with the toolchain that ships with latest Xcode on macos, so at least on my machine it didn't work.

@SChernykh
Copy link
Collaborator

The C++ code has __builtin___clear_cache but the linker complains about ___clear_cache so the clang compiler does know about this function and replaces it with the call to __clear_cache, the problem is that the code is not linked against the correct library (libgcc probably). IMHO even the XMRig's solution is not a proper one, it just works around the problem which is somewhere in the CMake build step. Can you try iains/gcc-darwin-arm64#33 (comment) and other suggestions there?

@MrCyjaneK
Copy link
Author

I've seen these solutions (and tried some), and they target macos instead of ios, and according to (a rather old) issue https://gitlab.haskell.org/ghc/ghc/-/issues/8561 ___clear_cache is a thing on MacOS but not iOS, people there also mention that JIT is not really a possible option on non-jailbroken iOS device, which also makes RandomX unusable, at least for the vast majority of iOS users.

luajit case mentioned in that issue also seems to treat iOS as a special thing when it comes to clearning cache https://repo.or.cz/luajit-2.0.git/blob/HEAD:/src/lj_mcode.c#l51

#if LJ_TARGET_X86ORX64
  UNUSED(start); UNUSED(end);
#elif LJ_TARGET_WINDOWS
  FlushInstructionCache(GetCurrentProcess(), start, (char *)end-(char *)start);
#elif LJ_TARGET_IOS
  sys_icache_invalidate(start, (char *)end-(char *)start);
#elif LJ_TARGET_PPC
  lj_vm_cachesync(start, end);
#elif defined(__GNUC__) || defined(__clang__)
  __clear_cache(start, end);
#else
#error "Missing builtin to flush instruction cache"
#endif

src/jit_compiler_a64.cpp Outdated Show resolved Hide resolved
@MrCyjaneK MrCyjaneK force-pushed the cyjan-fix-ios branch 3 times, most recently from 0ecc9f2 to 9803b04 Compare April 2, 2024 20:23
@MrCyjaneK MrCyjaneK marked this pull request as draft April 4, 2024 12:42
@MrCyjaneK
Copy link
Author

Do not merge, aarch64-linux-android was broken in this port, I'll take look at it in a bit.

@MrCyjaneK MrCyjaneK marked this pull request as ready for review April 14, 2024 12:23
@MrCyjaneK
Copy link
Author

Do not merge, aarch64-linux-android was broken in this port, I'll take look at it in a bit.

Resolved. Testing on iOS is still pending - but it should work without issues, it doesn't break windows, linux and macos builds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants