aarch64 AOT module fails to load on android

[bianchui]

Fixes: #2274

[bianchui]

nuttx build always fail with error /usr/bin/bash: line 1: /opt/wasi-sdk/bin/clang: No such file or directory

[yamt]

nuttx build always fail with error /usr/bin/bash: line 1: /opt/wasi-sdk/bin/clang: No such file or directory

this error is not the reason of failure.

[bianchui]

I add a function merge_data_and_text only for BUILD_TARGET_AARCH64 now, but it may fix other platform need data an text sections merged together.

[bianchui]

@wenyongh needs help about test error in spec test on nuttx, same code working well on android
did i miss something or how can i test locally

[wenyongh]

@wenyongh needs help about test error in spec test on nuttx, same code working well on android did i miss something or how can i test locally

I didn't have nuttx environment to test the spec cases either, not sure whether it is caused by this sentence if (total_size > (uint64)code_size) {. I replied some comments, could you help have a look and try again?

[wenyongh]

@bianchui Thanks for addressing the the comments. I tested the PR in my local repo, it seems that the nuttx CI failures were caused by partly os_mmap the merged data and aot with extra MMAP_PROT_EXEC flag: runtime os_mmap module->merged_data_text_sections with MMAP_PROT_READ | MMAP_PROT_WRITE and then os_mprotect the first part (aot text) of it with MMAP_PROT_READ | MMAP_PROT_WRITE | MMAP_PROT_EXEC. Some device may not support that. If I changed os_mmap the whole region with the latter, it works:
https://github.com/wenyongh/wasm-micro-runtime/actions
The PR is:
wenyongh@ef1ec7b

I think we need to discuss how to figure out it, maybe os_mmap with extra MMAP_PROT_EXEC flag for some devices. Or check the return value of os_mprotect.

[bianchui]

I think we need to discuss how to figure out it, maybe os_mmap with extra MMAP_PROT_EXEC flag for some devices. Or check the return value of os_mprotect.

Thanks, that's solve my confusing.
so merge .data is safe because they do not need flag MMAP_PROT_EXEC.
merge .data and .text is safe on android and windows at least, because they support page align protect change.
other platforms maybe not support part protect change.
macos with MAP_JIT maybe affected too Hardened Runtime that`s needs test.
i will try check the mprotect return value first.

[bianchui]

another question is they dose not support change protection of sub section of one mmaped memory?
or they only dose not support mixed MMAP_PROT_EXEC and without that flag memories in one mmaped?

[bianchui]

according to man of macos mprotect
The mprotect() system call changes the specified pages to have protection prot. Not all implementations will guarantee protection on a page basis but Mac OS X's current implementation does.

[wenyongh]

according to man of macos mprotect The mprotect() system call changes the specified pages to have protection prot. Not all implementations will guarantee protection on a page basis but Mac OS X's current implementation does.

Yes, and some platforms' os_mprotect just returns 0, like nuttx and esp-idf, they may assume their os_mmap has correctly set the protection flags and won't change again. My suggestion is (1) os_mmap with read/write/exec flags for these platforms and not call os_mprotect again, (2) os_mmap with read/write flags other platforms, and os_mprotect the aot text part with extra exec flag, if fails, os_munmap merged data and text, and return false. The patch can be found:
wenyongh@1ae1f24

Could you help check it? If it is good, could you please update your PR and maybe we can merge the PR for more test?

[bianchui]

Could you help check it? If it is good, could you please update your PR and maybe we can merge the PR for more test?

done with cherry-pick that commit.
is that safe to leave the MMAP_PROT_EXEC bit on for nuttx and esp-idf?

[wenyongh]

Could you help check it? If it is good, could you please update your PR and maybe we can merge the PR for more test?

done with cherry-pick that commit. is that safe to leave the MMAP_PROT_EXEC bit on for nuttx and esp-idf?

I am not sure, this means that the data sections in the merged data and text are also marked as executable. I think we need the help of nuttx and esp-idf guys.

@yamt, @no1wudi how do you think? Thanks.

[yamt]

Could you help check it? If it is good, could you please update your PR and maybe we can merge the PR for more test?

done with cherry-pick that commit. is that safe to leave the MMAP_PROT_EXEC bit on for nuttx and esp-idf?

I am not sure, this means that the data sections in the merged data and text are also marked as executable. I think we need the help of nuttx and esp-idf guys.

@yamt, @no1wudi how do you think? Thanks.

for nuttx, depending on the configuration, os_mmap with MMAP_PROT_EXEC allocates memory from a separate region, which might be scarce and even might involve special access restrictions.
on the other hand, the region allocated by os_mmap w/o MMAP_PROT_EXEC might not be executed.
(note: esp32 qemu used by our CI doesn't emulate these bits well. you need to test on a real hardware.)

unless you are very interested in working on these exotic platforms, i guess it's safer to avoid merging text and data for nuttx.

although i have no idea about esp-idf, i guess it's similar. (as it runs on the same esp32 chips)

[bianchui]

i guess it's safer to avoid merging text and data for nuttx.

although i have no idea about esp-idf, i guess it's similar. (as it runs on the same esp32 chips)

@yamt so disable merge .text and .data for nuttx and esp32 is better?

[no1wudi]

On resource-constrained embedded platforms, we indeed cannot guarantee that functions like os_mmap, typically implemented with an MMU, will behave consistently with complex Unix-based platforms. Therefore, it's best to assume that such functionalities are disabled by default.

[wenyongh]

LGTM

[wenyongh]

@bianchui I refactored the code of aot loader in another PR #3711, could you help have a look? Thanks.

[bianchui]

@bianchui I refactored the code of aot loader in another PR #3711, could you help have a look? Thanks.

I read the PR and everything is fine