-
Notifications
You must be signed in to change notification settings - Fork 627
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 compilation error on iOS due to macOS-specific APIs #2995
Conversation
@@ -5,7 +5,8 @@ | |||
|
|||
#include "platform_api_vmcore.h" | |||
|
|||
#if (defined(__APPLE__) || defined(__MACH__)) && defined(__arm64__) | |||
#if (defined(__APPLE__) || defined(__MACH__)) && defined(__arm64__) \ | |||
&& TARGET_OS_OSX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using TARGET_OS_OSX may cause compilation error in some platforms, do you mean defined(TARGET_OS_OSX)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iOS and MacOS both defined TARGET_OS_OSX.
MacOS:
#define TARGET_OS_OSX 1
iOS:
#define TARGET_OS_OSX 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"defined (__ APPLE__) | | defined (__ MACH__) " has already indicated that it is the Apple platform, and all Apple platforms have defined TARGET_OS_OSX
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So seems it had better be && defined(TARGET_OS_OSX) && TARGET_OS_OSX != 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks better now!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"defined (__ APPLE__) | | defined (__ MACH__) " has already indicated that it is the Apple platform, and all Apple platforms have defined TARGET_OS_OSX
OK, if that, TARGET_OS_OSX != 0
should be good.
Another question is should we apply similar change to sys_icache_invalidate? Is this API also only defined in macOS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I made a mistake. This method is defined on both iOS and macOS and has no restrictions on ARM architecture.
It would be better to change it to the following:
#if defined(__APPLE__) || defined(__MACH__)
#include <libkern/OSCacheControl.h>
#endif
#if defined(__APPLE__) || defined(__MACH__)
sys_icache_invalidate(start, len);
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, sounds good.
…d remove the limitations of this method on non-ARM architectures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for reviewing |
After #2995, AOT may stop working properly on arm MacOS: ```bash wasm-micro-runtime/core/iwasm/common/wasm_runtime_common.c, line 1270, WASM module load failed AOT module load failed: mmap memory failed ``` That's because, without `#include <TargetConditionals.h>`, `TARGET_OS_OSX` is undefined, since it's definition is in that header file.
…nce#2995) `pthread_jit_write_protect_np` is only available on macOS, and `sys_icache_invalidate` is available on both iOS and macOS and has no restrictions on ARM architecture.
After bytecodealliance#2995, AOT may stop working properly on arm MacOS: ```bash wasm-micro-runtime/core/iwasm/common/wasm_runtime_common.c, line 1270, WASM module load failed AOT module load failed: mmap memory failed ``` That's because, without `#include <TargetConditionals.h>`, `TARGET_OS_OSX` is undefined, since it's definition is in that header file.
This commit addresses a compilation issue on the iOS platform within the wasm-micro-runtime project-mini. The problem was caused by the usage of certain APIs that are exclusive to macOS. The code has been updated to include platform-specific preprocessor directives, ensuring that these macOS-specific APIs are only called when compiling for macOS. This change preserves the functionality on macOS while allowing successful compilation on iOS.