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

Android wasi support. #590

Merged
merged 6 commits into from
Mar 24, 2021
Merged

Android wasi support. #590

merged 6 commits into from
Mar 24, 2021

Conversation

JavanZhu
Copy link
Contributor

Android WASI support

Hi, I am trying to adapt WASI for Android platform. But there is some build error on Android because some system APIS are
not well supported by some versions of Android system.

API name ANDROID_API
preadv 24
pwritev 24
posix_fadvise 21
posix_fallocate 21
linkat 21
symlinkat 21
readlinkat 21
telldir 23
seekdir 23
futimens 19
getrandom 28

So, here is some points to adapt it:

  1. If ANDROID_PLATFORM is not set in CMakeLists.txt, the default value of ANDROID_API is 16. In that situation,
    we define some empty functions to avoid build error and also print some errors to notice users.
  2. And if ANDROID_PLATFORM is set to 28, all system api that libc WASI needed is supported, so there is no need to change current source code.

@@ -5,6 +5,10 @@

#include "platform_api_vmcore.h"
#include "platform_api_extension.h"
#include "bh_log.h"

#define API_NOT_SUPPORT_ERROR(API, LEVEL) LOG_ERROR("%s() is only supported when compile option ANDROID_PLATFORM >= %s.", #API, #LEVEL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In WAMR's design, the platform layer is the lowest layer, it should not depend any upper layer, so here we should not include bh_log.h and call LOG_ERROR. Could you please call __android_log_print instead, how about:

#define API_NOT_SUPPORT_ERROR(API, VERSION) __android_log_print("%s() is only supported when __ANDROID_API__ >= %s.", #API, #VERSION)

@@ -33,3 +37,69 @@ int os_vprintf(const char *fmt, va_list ap)
return __android_log_vprint(ANDROID_LOG_INFO, "wasm_runtime::", fmt, ap);
}

#if __ANDROID_API__ < 19

int futimens(int __dir_fd, const struct timespec __times[2]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coding style, please put '{' to the next line:

int futimens(int __dir_fd, const struct timespec __times[2])
{
  ...
}


#if __ANDROID_API__ < 21

int posix_fallocate(int __fd, off_t __offset, off_t __length) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

return -1;
}

int linkat(int __old_dir_fd, const char *__old_path, int __new_dir_fd, const char *__new_path, int __flags) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coding style, the line is too long, please change it to

int linkat(int __old_dir_fd, const char *__old_path,
           int __new_dir_fd, const char *__new_path, int __flags)
{
   ...
}


int posix_fadvise(int fd, off_t offset, off_t len, int advice);

int linkat(int __old_dir_fd, const char *__old_path, int __new_dir_fd, const char *__new_path, int __flags);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, change to:

int linkat(int __old_dir_fd, const char *__old_path,
           int __new_dir_fd, const char *__new_path, int __flags);

@@ -11,6 +11,7 @@ set (ANDROID_NDK $ENV{ANDROID_NDK_HOME})
set (ANDROID_SDK $ENV{ANDROID_SDK_HOME})
set (ANDROID_ABI "x86")
set (ANDROID_LD lld)
set (ANDROID_PLATFORM 24)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that it isn't used, in fact the macro ANDROID_API is used instead, could we remove this line?

Copy link
Contributor Author

@JavanZhu JavanZhu Mar 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some reasons make me think that we should "set (ANDROID_PLATFORM 24)" by default.

  1. refer to Android NDK ANDROID PLATFORM Doc,
    seems that we should set "ANDROID_PLATFORM" to determine the value of macro ANDROID_API.
  2. API level 24, equals Android system version 7.0, most of current android system version is larger that 7.0 .
    image
    In that situation, wamr libc WASI could supports most of APIs except getrandom().
    (if we remove this line, instead we could tell developers that they must config "ANDROID_PLATFORM" if they want to support libc WASI in the WAMR developer Document).

If need, I can remove this line. How do you think? thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, but had better not hardcode it to 24, could you please modify it to

if (NOT DEFINED ANDROID_PLATFORM)
    set (ANDROID_PLATFORM 24)
endif ()

So developer can use cmake -DANDROID_PLATFORM=xx to set the version number if his version number is different.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll change and test it later.

@wenyongh wenyongh merged commit 8f902fa into bytecodealliance:main Mar 24, 2021
wenyongh referenced this pull request in wenyongh/wasm-micro-runtime Mar 24, 2021
Enable Android libc wasi support. (#590)
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
Some libc APIs required by wasi native lib are missing in some Android API versions, only when the version >= 24, all APIs are supported. Add the missing APIs in android platform layer, leave them empty, report error and return failed if they are called. Also update CMakeLists.txt to enable libc wasi by default.

Co-authored-by: Wenyong Huang <wenyong.huang@intel.com>
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