WASM: add webassembly-micro-runtime port #120
Conversation
Summary of ChangesHello @R4ken, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new port for the WebAssembly Micro Runtime (WAMR) to the Phoenix-RTOS ecosystem. It establishes a foundational layer that allows WebAssembly modules to run on Phoenix-RTOS, complete with custom platform adaptations for file system access, threading primitives, and mathematical functions. The integration supports multiple architectures and includes configurations for resource-constrained environments, paving the way for deploying WebAssembly applications on the RTOS. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a port for the WebAssembly Micro Runtime (WAMR) to Phoenix-RTOS, which is a significant new feature. The changes include a build script, patches to adapt WAMR to the Phoenix-RTOS environment, and example files. The patches correctly disable features not supported on Phoenix-RTOS, such as AOT/JIT and dynamic linking, and provide workarounds for missing system calls and library functions. However, the review has identified several issues, including a critical compilation error, potential memory leaks, incorrect logic in file operations, and leftover debug code. These issues should be addressed to ensure the stability and correctness of the port.
| { | ||
| strncpy(msg, buffer, buf_len); | ||
| } |
There was a problem hiding this comment.
The use of strncpy is unsafe as it does not guarantee null-termination if the source string is as long as or longer than buf_len. This can lead to buffer overflows when the destination buffer msg is read as a string later. Additionally, the size of the destination buffer msg is not known, which is a security risk. The function signature should ideally be changed to include the destination buffer size. For a safer implementation within the current signature, you should ensure null-termination.
{
if (buf_len > 0) {
strncpy((char *)msg, (const char *)buffer, buf_len - 1);
msg[buf_len - 1] = '\0';
}
}There was a problem hiding this comment.
This code is shown as an example of usage of native libraries and is not functional part of runtime
| +double nearbyint(double x) | ||
| +{ | ||
| + return round(x); | ||
| +} | ||
| +float nearbyintf(float x) | ||
| +{ | ||
| + return round(x); | ||
| +} |
There was a problem hiding this comment.
The implementations of nearbyint and nearbyintf using round are not correct. round() rounds halfway cases away from zero and may raise the FE_INEXACT floating-point exception. In contrast, nearbyint() should round according to the current floating-point rounding mode (e.g., to nearest even) and must not raise the FE_INEXACT exception. This discrepancy can lead to subtle bugs and incorrect calculations in floating-point sensitive code.
There was a problem hiding this comment.
Phoenix does not support fenv.h floating point exceptions
| fi | ||
|
|
||
| mkdir -p "${PRODUCT_MINI_DIR}" | ||
| (cd "${PRODUCT_MINI_DIR}" && cmake ${WAMR_FLAGS} .. && make) |
There was a problem hiding this comment.
Double quotes preserve indentation between flags which breaks passing them to cmake
| fi | ||
|
|
||
| mkdir -p "${PRODUCT_MINI_DIR}" | ||
| (cd "${PRODUCT_MINI_DIR}" && cmake ${WAMR_FLAGS} .. && make) |
There was a problem hiding this comment.
[shellcheck (suggestion)] reported by reviewdog 🐶
| (cd "${PRODUCT_MINI_DIR}" && cmake ${WAMR_FLAGS} .. && make) | |
| (cd "${PRODUCT_MINI_DIR}" && cmake "${WAMR_FLAGS}" .. && make) |
|
Ported runtime seems to run well, though I have some nitpicks. Patching Also, instead of patching |
|
Actually, since we are planning on adding missing POSIX features, we can probably leave this patchset as it is. |
Add add webassembly-micro-runtime port JIRA: RTOS-1087
oI0ck
left a comment
oI0ck
left a comment
There was a problem hiding this comment.
This pull request is a bit hacky, due to Phoenix not being fully POSIX compilant.
The patchset should probably be reworked before upstreaming in the future, however, it makes little sense to change it now, before we implement missing POSIX features.
Porting this runtime was a considerable undertaking, thank you for your work.
| +/* Missing definitions */ | ||
| +double nearbyint(double x); | ||
| +float nearbyintf(float x); | ||
| +#define O_ACCMODE 3 // Mask for accessing R/W access mode |
There was a problem hiding this comment.
| +#define O_ACCMODE 3 // Mask for accessing R/W access mode |
O_ACCMODE is already defined in fcntl.h. Moreover its value here is incorrect.
#define O_RDONLY 0x00001U
#define O_WRONLY 0x00002U
#define O_RDWR 0x00004U
#define O_APPEND 0x00008U
#define O_CREAT 0x00100U
#define O_TRUNC 0x00200U
#define O_EXCL 0x00400U
#define O_SYNC 0x00800U
#define O_NONBLOCK 0x01000U
#define O_NDELAY O_NONBLOCK
#define O_NOCTTY 0x02000U
#define O_CLOEXEC 0x04000U
#define O_RSYNC 0x08000U
#define O_DSYNC 0x10000U
#define O_ACCMODE (O_RDONLY | O_WRONLY | O_RDWR)That'd make O_ACCMODE resolve to 7, not 3
|
Merge after phoenix-rtos/libphoenix#433 |
Add add webassembly-micro-runtime port
JIRA: RTOS-1087
Description
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist:
Special treatment