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

Refactor/sdk preview #83

Merged

Conversation

L-jasmine
Copy link
Collaborator

Refactor the SDK to resolve memory-unsafe bugs caused by its usage.

L-jasmine and others added 16 commits October 31, 2023 16:34
…un_func_async_with_timeout` (WasmEdge#76)

* [Feat] run func timeout use std::time::Duration

Signed-off-by: csh <[email protected]>

* [Doc]: update api docs

Signed-off-by: csh <[email protected]>

---------

Signed-off-by: csh <[email protected]>
(cherry picked from commit 07e84b7)
Signed-off-by: csh <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: csh <[email protected]>
Signed-off-by: csh <[email protected]>
Copy link
Member

juntao commented Nov 1, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Overall Summary:

In general, the patches seem to address various issues and introduce changes to improve the codebase. However, there are several potential problems and errors that need to be addressed before merging these changes:

  1. Lack of documentation and context: Many of the patches lack sufficient documentation or explanation for the changes being made. It would be beneficial to provide more context and rationale for the modifications.

  2. Incomplete or missing tests: Some of the patches do not include tests to verify the effectiveness of the changes. It is important to have appropriate tests in place to ensure correct behavior and prevent future regressions.

  3. Deletions and additions without clear explanation: In some patches, there are deletions of files or additions of new files or structs without clear explanations or justifications. It is essential to understand the motivations behind these changes and their impact on the overall codebase.

  4. Performance implications: Some of the refactoring changes involve a large number of insertions and deletions, which may have performance implications. It is important to analyze the performance impact of these changes and ensure they do not introduce any regressions.

  5. Improvements needed in error handling and code readability: There are instances where error handling can be improved, and code readability can be enhanced by using more descriptive variable names.

In summary, while the patches address certain issues and introduce improvements, further investigation and improvements are required in terms of documentation, tests, and addressing potential problems before merging these changes.

Details

Commit ecaa6cb9da0f3a25d771ce653e61350522966252

Key Changes:

  • The patch is refactoring the WasmEdgeError enum in the error.rs file of the wasmedge-types crate.
  • The changes include adding #[from] attribute for subtypes of WasmEdgeError.
  • The #[error] attribute is modified to use string interpolation for formatting error messages.

Potential Problems:

  • There are no apparent issues with the changes in this patch. The refactoring seems to be a straightforward and appropriate modification to the codebase.

Commit 2116d0b7125504d8b3323365ff41cf3e85761243

The key changes in this patch include:

  • Fixes memory leaks in the codebase.
  • Refactors various modules and files.
  • Adds new files related to asynchronous handling.

Potential problems:

  • The patch makes changes to a large number of files. It would be helpful to have more context about the changes and the overall goals of this refactoring.
  • It's unclear why some files are being deleted, such as src/externals/function.rs and src/wasi.rs. These deletions may have implications elsewhere in the codebase.
  • The patch introduces new files related to asynchronous handling (src/async/import.rs, src/async/mod.rs, src/async/vm.rs). It's unclear how these files are being utilized and how they integrate with the rest of the codebase.
  • There are potential performance implications with the refactoring changes, especially with the large number of insertions and deletions.

Overall, this patch seems to be a significant refactoring effort. It would be helpful to have more context and information about the motivation behind these changes in order to provide a more comprehensive review.

Commit 949c9876187cfc1a7105452b1792cb06acf8a7f9

Key changes in this patch include:

  1. Added a SocketWritable struct with methods writable() and set_writable() to handle socket writability.
  2. Added a SocketWritableFuture struct that implements the Future trait for waiting until a socket is writable.
  3. Added a writable field to the AsyncWasiSocket struct to track socket writability.
  4. Modified the AsyncWasiSocket struct to include new methods readable() and writable() for reading and writing to the socket.
  5. Modified the AsyncWasiSocket constructor to initialize the writable field to a default value.
  6. Updated the AsyncWasiSocket::readable() function to use the inner socket's readable() method.
  7. Updated the AsyncWasiSocket::writable() function to set the writable field and call the inner socket's writable() method.
  8. Updated the wait_fd() function to use the new SocketWritable functionality.

Potential problems:

  • It seems that the writable field is always initialized to true in the AsyncWasiSocket struct. This may lead to unexpected behavior if the socket is not actually writable.
  • The error handling in the handler closure of the wait_fd() function can be improved. Instead of using Errno::from(e), it would be better to handle specific error cases separately for more accurate handling.

Commit 0524f2267559f5dc6517f5329078a1fd1a51f961

Key changes:

  • The patch fixes a bug related to blocking during the TCP connect process in the tokio-wasi library.
  • It adds a line of code to set the socket as writable after establishing a connection.

Potential problems:

  • It's not clear if the blocking issue has been effectively resolved. There is no explanation or context in the patch to understand the problem or the solution. It would be helpful to have more information to ensure the fix addresses the root cause.
  • The patch doesn't provide any tests to verify the fix. It would be beneficial to include some test cases to confirm that the issue is resolved and to prevent future regressions.

Overall, the patch seems to address a specific issue, but additional information, context, and tests are needed to fully evaluate its effectiveness.

Commit d7f841eda90b63b9beab06d3178bd10282f42b3d

Key changes:

  • Added two lines of code in the args_get function to insert a null terminator at the end of the argument buffer.
  • Added two lines of code in the environ_get function to insert a null terminator at the end of the environment buffer.

Potential problems:

  1. The patch does not have any functional or logical changes. It only adds null terminators at the end of some buffers. Without further context, it is unclear why these changes are necessary or what problem they are solving. There should be a clear explanation or justification provided in the pull request.
  2. There are no comments or documentation explaining the purpose of the args_get and environ_get functions. It would be helpful to have comments or documentation describing the expected behavior and usage of these functions.
  3. The commit message does not provide enough information about the purpose of the patch. The title "[Fix] async-wasi get envs & args" is too generic and does not provide specific details about the fix or the issue being addressed. The commit message should be more descriptive and informative.
  4. There is no test coverage mentioned in the patch. It is crucial to have unit tests or integration tests to verify the correctness of the changes. The pull request should include information about any tests that have been added or modified to cover the changes made in this patch.
  5. The code changes could benefit from more descriptive variable names. Variable names like arg_buf, env_buf, and ptr do not provide enough information about their purpose or content. Using more meaningful variable names would improve code readability and maintainability.

Overall, the patch seems to be lacking in terms of documentation, tests, and justification. These aspects should be addressed before merging the changes.

Commit 7408e6278d4172c303bc214adc736b8b0c270d63

Key changes in the patch:

  1. The state field in the AsyncWasiSocket struct has been changed from a direct type (WasiSocketState) to a box pointer (Box<WasiSocketState>).
  2. The seek method and its implementation have been added to the WasiFile struct.
  3. The usage of the Arc<RwLock<ObjectPool<VFD>>> type for the vfs field in the WasiCtx struct has been replaced with the ObjectPool<VFD> type directly.

Potential problems:

  1. The change to the state field in the AsyncWasiSocket struct may introduce additional memory allocations and potentially impact performance. Further analysis is required to determine if this change is necessary and if it provides any benefits.
  2. The addition of the seek method and its implementation in the WasiFile struct should be carefully reviewed to ensure it functions correctly and does not introduce any bugs or issues related to file seeking.
  3. The use of the ObjectPool<VFD> type directly in the WasiCtx struct may have unintended consequences or affect other parts of the code that rely on the previous usage of Arc<RwLock<ObjectPool<VFD>>>. It is important to verify if this change is appropriate and does not introduce any synchronization or concurrency issues.

Commit 882a3b2cd00003615b06105965233fd44a4dd98b

Key Changes:

  • The serialize feature has been temporarily disabled by removing it from the "default" feature list and the "serialize" feature list in the [features] section of the Cargo.toml file for the async-wasi crate.
  • The formatting of the Cargo.toml file has been adjusted for consistency.

Potential Problems:

  • It's unclear why the serialize feature has been temporarily disabled. The reason for this change should be documented for future reference.
  • Removing the serialize feature may impact the functionality or compatibility of the async-wasi crate. It's important to ensure that any necessary changes or adjustments have been made to handle this.

Commit 47a360488a27dcc65f759517938b9556c81b59ad

Key Changes:

  • Added a new struct WasiStdin which implements the Read trait.
  • Added a new struct WasiStdout which implements the Write trait.
  • Modified the existing WasiStdin and WasiStdout structs to use dynamic dispatch instead of concrete types.
  • Implemented various methods for the WasiStdin and WasiStdout structs to handle read/write operations.

Potential Problems:

  • The commit message is not informative. It only mentions "refactor/sdk preview" but doesn't provide any details about the changes made.
  • It seems like the changes being made are related to a virtual file system, but without more context it's difficult to understand the purpose of these changes and how they fit into the overall codebase.
  • There are several unused imports in the code, such as fs and io. These should be removed to clean up the code.
  • It's unclear why the WasiStdin and WasiStdout structs are implementing both Deref and DerefMut traits. It would be helpful to provide comments explaining the reason for this implementation.
  • The method fn systimespec is defined but not used anywhere in the code. It should either be removed or used appropriately.
  • There are a few methods in the WasiStdin and WasiStdout structs that return an error without providing any information about the error. It would be better to return a specific error type or provide more meaningful error messages.
  • In the WasiStdout struct, the fd_read and fd_pread methods are not implemented, but are defined in the trait WasiVirtualFile. This inconsistency should be addressed.

Overall, more information is needed to fully understand the purpose and impact of these changes.

Commit f71e855d8372776f38c758819d9642cdcba18560

The key changes in this patch are:

  1. Update the argument type of run_func_with_timeout and run_func_async_with_timeout functions.
  2. Add an argument async_state to the TimeoutFiberFuture struct and the on_fiber function.
  3. Replace the timeout_sec argument in TimeoutFiberFuture with a deadline argument of type std::time::SystemTime.
  4. Convert the timeout parameter in the call_func_with_timeout function from u64 to std::time::Duration.
  5. Replace the timeout parameter in the on_fiber function with a deadline parameter of type std::time::SystemTime.

Potential problems:

  1. The function descriptions in the code comments are not clear and could be improved.
  2. The code does not handle potential errors when creating timers or setting timer values.
  3. The variable slot is not used and could be removed for clarity.
  4. The code assigns std::mem::zeroed() to the value variable, which could be unsafe if not handled properly.
  5. The code uses C standard library functions (timer_create, timer_settime, timer_delete) which can be error-prone and platform-specific.

Commit 317c7a990f8fc0e781df02334b18fd23b0f91ba8

Key Changes:

  • Removed the registered field from the Validator struct.
  • Removed the condition for calling ffi::WasmEdge_ValidatorDelete in the Drop implementation based on the registered field.

Potential Problems:

  • The removal of the registered field may have unintended consequences. It is unclear if this field was previously used elsewhere in the codebase and how its removal may impact the functionality of the Validator struct.
  • The removal of the condition for calling ffi::WasmEdge_ValidatorDelete in the Drop implementation means that the Validator struct will always call this function when dropped, regardless of the registered state. This may lead to potential memory leaks if the Validator instance was not correctly registered before being dropped.

Commit 89ea9a2713dd3da31b18cfa9fde172e1a7f6d58e

Summary of key changes:

  • The code has been formatted and made clippy-compliant.
  • The conversion impl Into<INode> for WasiStdin has been changed to impl From<WasiStdin> for INode.
  • The conversion impl Into<INode> for WasiStdout has been changed to impl From<WasiStdout> for INode.
  • The conversion impl Into<INode> for WasiStderr has been changed to impl From<WasiStderr> for INode.
  • The #[allow(clippy::needless_pass_by_ref_mut)] attribute has been removed.
  • The function poll_oneoff_impl has been moved into the #[cfg(feature = "serialize")] block.
  • The wrap_future function has been changed to have an additional argument data.
  • The call_func_with_timeout function now takes a mutable reference to the Function object instead of an immutable reference.
  • The returns vector length has been changed from returns_len as usize to returns_len.
  • The returns vector is now initialized with a length of returns_len using returns.set_len(returns_len).

Potential problems:

  • The conversion impl From<WasiStderr> for INode is missing an implementation for the Self return type.

Commit 7cfb54777a586951fa4fe879dfc8639ab94abb35

Key changes:

  • Disabled timeout functionality in the musl libc environment.
  • Removed the use of the TimeoutFiberFuture struct in the executor file.

Potential problems:

  • The code changes seem to be specific to the Linux operating system and not applicable to other platforms.
  • The ifdef statements are hard to read and understand. It would be helpful to include comments explaining the purpose of the conditionals.
  • It is unclear why the timeout functionality is disabled in the musl libc environment. This change should be documented and reviewed for potential impacts on the functionality.
  • There is a mix of cfg and cfg_attr directives, which could be consolidated for better readability.

Overall, it seems like the changes are related to the timeout functionality in the executor module, particularly for Linux-based operating systems. The code changes appear to be fine, but the intent and reasoning behind disabling the timeout in the musl libc environment should be clarified and reviewed.

Commit bf772c0d7b52818c0736e9ecd0a9da5d0701b0c3

Key changes:

  • The run_func_with_timeout function in the Vm struct is modified.
  • The #[cfg(target_os = "linux")] attribute is changed to #[cfg(all(target_os = "linux", not(target_env = "musl")))].

Potential problems:

  • The commit message does not provide sufficient information about the purpose of the changes.
  • It is unclear why the timeout is being disabled specifically for musl libc on Linux.
  • There are no comments explaining the reason behind the condition in the #[cfg] attribute.

Overall, more context is needed to fully understand the purpose and potential implications of these changes. It would be beneficial to have more descriptive commit messages and additional comments explaining the rationale behind the modifications.

Commit 2ffdbb0f96c270faf76d43e94ff2c8b3050c4c81

Key Changes:

  • Updated documentation in frame.rs to clarify the return type of the memory and memory_mut functions.
  • Updated documentation in store.rs to clarify the purpose of the Store struct.

Potential Problems:

  • The patch does not introduce any code changes, only documentation updates. Therefore, there are no potential problems with the code changes.

Overall, the patch seems to be a straightforward documentation update with no new code changes or potential issues.

Commit 9c95c51facbca52b324ce7d973f111cef0ae4a37

Key changes:

  • The patch fixes the conversion from ffi::WasmEdge_String to String.
  • The previous implementation used std::ffi::CStr and to_string_lossy() to convert to String.
  • The patch replaces the previous implementation with a new one that directly converts WasmEdge_String to a slice of raw bytes and then converts it to a String using String::from_utf8().

Potential problems:

  • The patch introduces an unsafe conversion using unsafe { std::slice::from_raw_parts() }. This could lead to memory safety issues if the raw pointer is not valid or if the length is incorrect.
  • The patch uses unwrap_or_default() when converting from Vec<u8> to String. This will silently drop any invalid UTF-8 bytes and replace them with the default value (String::default()). This might not be desired behavior, and it would be better to handle invalid UTF-8 gracefully or return an error.

Overall, the patch introduces some potential problems with memory safety and UTF-8 handling. It would be advisable to carefully review these changes and consider alternative approaches to address these issues.

Commit aa2639d36340822e31aeae59d67df80d5b7b131f

Key changes in the patch:

  • Removed the #[cfg(ignore)] attribute from the mod tests section in the ast_module.rs file, enabling the test code.
  • Removed the #[cfg(ignore)] attribute from the wrap_sync_wasi_fn and wrap_async_wasi_fn functions in the function.rs file, enabling the functions.

Potential problems:

  • The patch removes a significant amount of code in multiple files. It's important to ensure that this code is no longer needed or has been replaced by alternative implementations.
  • The removed code includes test cases and function definitions. Make sure that these deletions are intentional and won't cause any issues with the overall functionality of the software.
  • It is unclear why the #[cfg(ignore)] attribute was added to the test code and function definitions in the first place. It would be worth investigating the reason behind this and confirming that it is no longer necessary.
  • The patch does not include any new code or modifications, only deletions. This means that the overall functionality and behavior of the code should remain the same, but it's important to validate this through thorough testing.

Overall, the key changes in the patch are the removal of code related to test cases and function definitions. The potential problems include ensuring that the removed code is no longer needed and investigating the reason behind the #[cfg(ignore)] attribute.

Commit 4cccc4aef1ccc7277f85a0f802e764de7dbc92c2

Key changes:

  • The WasiModule struct is no longer using #[derive(Clone)].
  • The WasiModule struct now wraps an Instance object instead of an InnerInstance object.
  • Implementations of AsRef<Instance> and AsMut<Instance> have been added for WasiModule.
  • The Drop implementation for WasiModule has been removed.
  • Some function implementations have been modified to use the new Instance object.

Potential problems:

  • The WasiModule struct no longer implements Clone, so any code that relies on cloning WasiModule instances will need to be updated.
  • The Drop implementation for WasiModule has been removed, so the cleanup of resources may no longer be handled correctly.
  • The changes to function implementations may introduce bugs or unexpected behavior, so thorough testing is recommended.

Overall, the changes seem to be a refactor of the WasiModule struct to use the Instance object instead of the InnerInstance object. However, there are potential problems that need to be addressed.

Commit c80ddb47118b29e53146b96f769a318bdeb6ad81

Key changes in this patch:

  • Introduced a new function called WasmEdge_FunctionInstanceGetData in the Function struct.
  • Removed the host_data field from the Function struct.
  • Updated the drop implementation in the Function struct to handle the removal of host_data.
  • Updated test code.
  • Bumped the version of the rust-sdk to 0.13.0.

Potential problems to consider:

  1. The patch involves changes to multiple files, including build files and dependency versions. Ensure that the changes have been tested and do not introduce any build or dependency issues.

  2. The introduction of WasmEdge_FunctionInstanceGetData may impact the functionality of the Function struct. Review the implementation of this new function and verify that it behaves as expected.

  3. The removal of the host_data field from the Function struct could impact any code that relies on this field. Review the codebase for any such dependencies and ensure they have been properly updated to use the new WasmEdge_FunctionInstanceGetData function.

  4. Verify that the updated test code covers all the necessary scenarios and provides appropriate coverage for the changes made.

  5. Review the version bump for the rust-sdk crate and ensure that it is justified and does not introduce any compatibility issues.

  6. Check if the removal of debug code has any impact on the functionality or performance of the code and ensure it was intended.

Commit e475ea792ce2a95eff7f854d5f706c242c9d540b

Key Changes:

  • Added a new API PluginManager::nn_preload for initializing the wasi_nn plugin with preloads.
  • Implemented the FromStr trait for the NNPreload struct.
  • Refactored the argument types for several methods, breaking backwards compatibility.
  • Introduced a new C-API WasmEdge_FunctionInstanceGetData to fix a memory leak issue caused by host data.
  • Added support for macos-13 in the CI build and removed macos-11.

Potential Problems:

  • The refactoring of argument types may break compatibility with existing code that relies on the previous argument types.
  • The introduction of a new C-API may require updates to existing code that interacts with the SDK.
  • The support for macos-13 may introduce compatibility issues with older versions of macOS.

Overall, the changes seem to bring new features, but they may introduce compatibility problems and require updates to existing code. It is also important to verify the impact of the memory leak fix and ensure it does not introduce any regressions.

Commit a27724d24f1acb27ded3188a8b946631168642e0

Key changes in this patch:

  • Update the WASMEDGE_RELEASE_VERSION constant from "0.13.4" to "0.13.5" in the build.rs file in the crates/wasmedge-sys directory.
  • Update the sha256sum values for various platforms in the REMOTE_ARCHIVES map.

Potential problems:

  • There doesn't seem to be any potential problems in this patch. It appears to be a routine update to the build script, updating the version number and sha256sum values for different platforms.

Commit 18073d36d9f48bb515f14c691c1f29c6c55a0700

Key changes in the patch:

  • Added the import statement use crate::AsInstance; in the test_plugin_wasmedge_process and test_plugin_wasi_crypto functions.
  • Replaced the assert!(result.is_some()); statements with assert!(result.is_ok()); statements in the test_plugin_wasi_crypto function.
  • Replaced the assert!(result.is_some()); statements with assert!(result.is_ok()); statements in the mod_instance related assertions in the test_plugin_wasi_crypto function.

Potential problems:

  1. The patch does not include any comments or explanations for the changes made. It would be beneficial to provide some context or reasoning for the modifications.
  2. It is unclear why the import statement use crate::AsInstance; was added in the test_plugin_wasmedge_process and test_plugin_wasi_crypto functions. Further explanation may be needed.
  3. There seems to be inconsistencies in the assertions. The change from result.is_some() to result.is_ok() implies a change in the expected behavior, but without explicit comments, it is difficult to confirm if these changes are intentional.

Overall, the changes appear to be simple modifications to tests related to the wasmedge-sys crate. However, further clarification and explanation would be helpful to fully understand the purpose and impact of the changes.

Commit c99e30df4b00a863083794454b24f1df65c7062e

Key changes:

  • Update the wat dependency version in the Cargo.toml file to "1.0".
  • Update the version of wasmedge-sys crate to "0.17.4" in its Cargo.toml file.
  • Update the version of wasmedge-sdk crate to "0.13.1" in its Cargo.toml file.
  • Update the CHANGELOG.md file to include the new version and a description of the change.
  • Modify several Rust source files: ast_module.rs, compiler.rs, executer.rs, instance.rs, module.rs, and vm.rs.
  • Add new test code to the ast_module.rs file.

Potential problems:

  • The patch does not provide enough information about the reason for updating the wat dependency. It only mentions fixing an issue without specifying the details. It would be helpful to provide more context or reference to the issue.
  • Several test cases in the ast_module.rs file are marked as #[ignore = "need to update import.wat"]. This indicates that the tests are currently failing and need to be updated. It would be good to investigate and fix these tests before merging the changes.
  • The changes in the Rust source files (ast_module.rs, compiler.rs, executer.rs, instance.rs, module.rs, and vm.rs) are extensive and may introduce new bugs or issues. It would be important to thoroughly review these changes.
  • The patch does not provide any information about the impact of these changes on the overall functionality of the project. It would be good to include a summary of the impact or any potential risks associated with these changes.

Commit 6132ffe6db3098e2bc11d6e0bbb6d57ec0e5399b

Key changes:

  • Skipped the test_vmbuilder test by adding the --skip test_vmbuilder option to the cargo test command in the ci-build.yml file.

Potential problems:

  • It is not clear from the patch why the test_vmbuilder test is being skipped. This information should be added in the commit message or pull request description to provide context to reviewers and future maintainers.
  • It is important to ensure that skipping the test_vmbuilder test does not introduce any unexpected issues or break the functionality of the codebase. It may be necessary to provide an explanation for skipping the test or address the underlying problem causing the need to skip it.
  • The patch does not include any information about the reasoning behind the changes. It would be helpful to have more context on why the decision was made to skip the test_vmbuilder test.

Overall, the change seems simple and straightforward, but more information is needed to fully understand the reasoning behind it and ensure it does not introduce any new problems.

Commit b5daacef348582c380d3a96701cff4e3ec17a433

Key changes:

  • Updated the version of wasmedge-sys to "0.17.4" in Cargo.toml.
  • Added "zstd" to the list of dependencies to link in the build.rs file.

Potential problems:

  • There don't appear to be any potential problems with this patch. It seems to be a straightforward update to fix the static build and link against zstd.
  • However, without more context or information about the project, it's difficult to determine the full impact of these changes.

Commit 06e7d2d1db7873fc4454149b1bd2b42504a8daf4

Key changes:

  • Updated the CI workflows.
  • Updated the versioning table for rust-sys and rust-sdk.
  • Fixed a typo in the CI workflow for rust-static-lib.
  • Bumped the version of rust-sdk to 0.13.2.
  • Updated the versioning table in README.md, wasmedge-sys/src/lib.rs, and src/lib.rs.

Potential problems:

  • The changes in the CI workflows may introduce unexpected behavior or break the existing build process. It would be helpful to have more context on why these changes were necessary.
  • The specific changes made to the versioning table and the bump in the rust-sdk version are not mentioned in the PR description. It would be beneficial to provide more information about the rationale behind these changes.
  • The build steps in the rust-static-lib workflow are commented out. This should be clarified if these steps are intentionally commented out or if there was an oversight in removing them.
  • The patch includes changes to multiple files, but it is not clear if these changes are related or independent. It would be helpful to have a more detailed explanation of the changes made to each specific file.

Overall, the pull request lacks sufficient context and explanation of the changes made. It is recommended to provide more details in the PR description to help reviewers understand the purpose and impact of the changes.

Commit e914908b2fc7b1aa9452d99d6e370c28d7fe7524

Key changes:

  • Bumped the version of the wasmedge-sys dependency to 0.17.5 in Cargo.toml and lib.rs.
  • Updated the versioning table in README.md, lib.rs, and src/lib.rs to reflect the new wasmedge-sys version.

Potential problems:

  • There don't appear to be any potential problems in this patch. It is a straightforward update to the wasmedge-sys dependency version and related documentation.

Commit 6d72f15b91103ecf9abc77de8c0617c3fabd4f1a

Key changes:

  • Added a new entry for version 0.13.2 in the CHANGELOG.md file.
  • Included a bug fix related to the static build linking against zstd.

Potential problems:

  • The patch does not include any code changes, only documentation updates, so it would be helpful to review the code changes separately to evaluate the overall impact of the changes.
  • It is unclear why the bug fix is mentioned in the CHANGELOG without any corresponding code changes. It would be beneficial to investigate further to ensure that the bug fix is properly implemented and documented.

Commit 6be2ef9107e05ff46174f27bd50d56cbf208bb18

Key changes in the patch:

  • The workflow file .github/workflows/rust-static-lib.yml has been modified.
  • The Docker image wasmedge/wasmedge:latest has been replaced with wasmedge/wasmedge:ubuntu-build-clang.
  • The libboost-all-dev package has been removed from the list of dependencies.
  • The steps for formatting and linting the Rust code have been removed.
  • The command to build the WasmEdge library has been commented out.
  • The steps for testing the Rust bindings have been modified to test the Rust SDK instead.
  • A new step has been added to test the Rust SDK with the async feature.

Potential problems:

  • The commit message is duplicated multiple times, which may indicate an error in creating the patch.
  • The removed dependencies and commented-out build command may affect the functionality of the SDK.
  • The step for formatting and linting the Rust code has been removed, which may result in code quality issues.
  • The workflow file could benefit from more comments and explanations for the steps being performed.
  • Without more context or information about the specific requirements of the project, it is difficult to identify any other potential problems.

Commit 2d27eabecfb96b840e1fde42aae30b9328139111

Key changes:

  • The implementation of the From<WasmValue> for Val trait has been modified.
  • The code related to FuncRef has been commented out and replaced with a new implementation for FuncRef using ExternRef.

Potential problems:

  • The commented-out code for FuncRef conversion has the potential to cause confusion for developers reviewing the code.
  • The new implementation for FuncRef using ExternRef may introduce compatibility issues if there are dependencies or assumptions based on the previous implementation.

Overall, the key change seems to be the temporary implementation of FuncRef using ExternRef, but it is essential to consider the potential impact on the existing codebase and dependencies.

Commit 17221d00a3a2446bd77431cc2e5ce74d0160a93d

The key changes in this Pull Request include refactoring the compiler.rs, async_run_func.rs, async_run_registered_func.rs, fibonacci.rs, executor_register_active_module.rs, executor_register_named_module.rs, mdbook_example_module_instance.rs, and some other files. The changes mainly involve replacing get_local with local.get and using then instead of return for if conditions in the WebAssembly code.

Based on the patch, potential problems that could be identified are:

  1. The patch contains multiple changes in different files, which makes it harder to review and understand the context of the changes. It would be better to split the changes into separate pull requests or at least group them based on their functionality or purpose.
  2. It's unclear why the changes were made and what problem they are addressing. The pull request description or commit message should provide more information about the motivation behind the refactoring and how it improves the code.
  3. Without the full context of the code and its usage, it's difficult to determine if the changes are correct and don't introduce any regressions. A more detailed explanation or tests would be helpful in ensuring the correctness of the changes.
  4. The patch shows an equal number of insertions and deletions, but it's not clear if the changes are actually improving the code or just changing it for the sake of it. A clearer explanation or justification for the changes would be beneficial.

Overall, the reviewer should request more information on the motivation, expected behavior, and testing of the changes before proceeding with a thorough review.

@L-jasmine L-jasmine force-pushed the refactor/sdk_preview branch 2 times, most recently from 7f750bb to aa2639d Compare November 1, 2023 15:19
L-jasmine and others added 3 commits November 2, 2023 08:46
…data (WasmEdge#84)

* feat(rust-sys): drop host_data in `Function::drop`

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update `drop` of `ImportModule` and `Function`

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sdk): update test code

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update `drop` of `ImportModule` and `Function`

Signed-off-by: Xin Liu <[email protected]>

* refactor(rust-sys): update `ImportModule::drop`

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): remove debug code

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sdk): remove debug code

Signed-off-by: Xin Liu <[email protected]>

* version(rust-sdk): bump to `0.13.0`

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): supress clippy warning

Signed-off-by: Xin Liu <[email protected]>

* ci(ci-build): update rust version

Signed-off-by: Xin Liu <[email protected]>

* ci(standalone): update rust version

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update rustdoc

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sdk): update rustdoc and `README`

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update dependencies

Signed-off-by: Xin Liu <[email protected]>

* ci: update to `macos-13`

Signed-off-by: Xin Liu <[email protected]>

---------

Signed-off-by: Xin Liu <[email protected]>
apepkuss and others added 4 commits November 8, 2023 13:33
* chore(rust-sys): update build script

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update build script

Signed-off-by: Xin Liu <[email protected]>

---------

Signed-off-by: Xin Liu <[email protected]>
L-jasmine and others added 5 commits November 14, 2023 13:45
* chore(rust-sdk): update `wat` dep

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sdk): update test code

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update test code

Signed-off-by: Xin Liu <[email protected]>

* version(rust-sys): bump to `0.17.4`

Signed-off-by: Xin Liu <[email protected]>

* version(rust-sdk): bump to `0.13.1`

Signed-off-by: Xin Liu <[email protected]>

* doc: update `CHANGELOG`

Signed-off-by: Xin Liu <[email protected]>

---------

Signed-off-by: Xin Liu <[email protected]>
@L-jasmine L-jasmine changed the title [WIP] Refactor/sdk preview Refactor/sdk preview Nov 15, 2023
jprendes and others added 4 commits November 15, 2023 18:00
* ci: update workflows

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update versioning table

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sdk): update versioning table

Signed-off-by: Xin Liu <[email protected]>

* ci(rust-static-lib): fix typo

Signed-off-by: Xin Liu <[email protected]>

* ci(rust-static-lib): update workflow

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update rustdoc

Signed-off-by: Xin Liu <[email protected]>

* version(rust-sdk): bump to `0.13.2`

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sdk): update versioning table

Signed-off-by: Xin Liu <[email protected]>

* ci(rust-static-lib): update workflow

Signed-off-by: Xin Liu <[email protected]>

---------

Signed-off-by: Xin Liu <[email protected]>
* version(rust-sys): bump to `0.17.5`

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update versioning table

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update dependency

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sdk): update versioning table

Signed-off-by: Xin Liu <[email protected]>

---------

Signed-off-by: Xin Liu <[email protected]>
@juntao
Copy link
Member

juntao commented Nov 15, 2023

@apepkuss @CaptainVincent Please review. We should aim to merge soon. Thanks.

* ci(rust-static-lib): update workflow

Signed-off-by: Xin Liu <[email protected]>

* ci(rust-static-lib): update workflow

Signed-off-by: Xin Liu <[email protected]>

* ci(rust-static-lib): update workflow

Signed-off-by: Xin Liu <[email protected]>

* ci(rust-static-lib): update workflow

Signed-off-by: Xin Liu <[email protected]>

* ci(rust-static-lib): remove `libboost-all-dev` dep

Signed-off-by: Xin Liu <[email protected]>

---------

Signed-off-by: Xin Liu <[email protected]>
@apepkuss
Copy link
Collaborator

@L-jasmine Please consider to merge both standalone and rust-static-lib workflows into this PR. These two workflows are necessary for quality assurance. For example, both runwasi and Docker teams have dependencies on these two modes.

@apepkuss
Copy link
Collaborator

@DarumaDocker Does this PR satisfy your requirement of using the wasi-nn_ggml plugin? In the current release (v.0.13.2), it's supported by PluginManager::nn_preload API.

apepkuss

This comment was marked as duplicate.

apepkuss

This comment was marked as duplicate.

@apepkuss
Copy link
Collaborator

apepkuss commented Nov 16, 2023

@L-jasmine Please refer to #91 and #90 to update the relevant Cargo.toml files.

/// # Safety
///
/// The lifetime of the returned pointer must not exceed that of the object itself.
pub unsafe fn as_ptr(&self) -> *const ffi::WasmEdge_PluginContext {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to use unsafe here as only the return value is of pointer type.

@@ -38,7 +38,7 @@ impl_wasm_val_type!(i64, ValType::I64);
impl_wasm_val_type!(f32, ValType::F32);
impl_wasm_val_type!(f64, ValType::F64);
impl_wasm_val_type!(i128, ValType::V128);
impl_wasm_val_type!(FuncRef, ValType::FuncRef);
// impl_wasm_val_type!(FuncRef, ValType::FuncRef);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove it if it's not useful. Thanks.

@@ -57,19 +57,19 @@ mod test_wasm_val_type {
assert_eq!(f32::WASM_TYPE, ValType::F32);
assert_eq!(f64::WASM_TYPE, ValType::F64);
assert_eq!(i128::WASM_TYPE, ValType::V128);
assert_eq!(FuncRef::WASM_TYPE, ValType::FuncRef);
// assert_eq!(FuncRef::WASM_TYPE, ValType::FuncRef);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove it if it's not useful. Thanks!

Comment on lines +211 to +225
// assert_eq!(
// <(i32, i64, f32, f64, FuncRef, ExternRef)>::wasm_types(),
// [
// ValType::I32,
// ValType::I64,
// ValType::F32,
// ValType::F64,
// ValType::FuncRef,
// ValType::ExternRef,
// ]
// );
// assert_eq!(
// <(FuncRef, ExternRef)>::wasm_types(),
// [ValType::FuncRef, ValType::ExternRef]
// );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove them if they're not useful. Thanks!

Comment on lines +283 to +287
// impl WasmVal for FuncRef {
// fn to_wasm_value(self) -> WasmValue {
// WasmValue::from_func_ref(self.inner)
// }
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove them if they're not useful. Thanks!

Comment on lines +45 to +46
// Val::FuncRef(Some(func_ref)) => WasmValue::from_func_ref(func_ref.inner),
// Val::FuncRef(None) => WasmValue::from_null_ref(RefType::FuncRef),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove them if they're not useful. Thanks!

src/types.rs Outdated
Comment on lines 61 to 70
if value.is_null_ref() {
Val::FuncRef(None)
// Val::FuncRef(None)
todo!()
} else {
let inner = value
.func_ref()
.expect("[wasmedge] Failed to convert a WasmValue to a FuncRef.");
let ty: FuncType = inner.ty().unwrap().into();
Val::FuncRef(Some(FuncRef { inner, ty }))
// let inner = value
// .func_ref()
// .expect("[wasmedge] Failed to convert a WasmValue to a FuncRef.");
// let ty: FuncType = inner.ty().unwrap().into();
// Val::FuncRef(Some(FuncRef { inner, ty }))
todo!()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove them or implement them if the if-else is still useful. Thanks!

@L-jasmine
Copy link
Collaborator Author

Because FuncRef typically has state, its lifecycle depends on the Module it resides in. Currently, the need to use FuncRef is mostly in the context of wasm-gc, and Rust compilation to wasm doesn't yet support the usage of FuncRef. Therefore, I've commented it out. However, since wasm-gc is nearing completion, we will need to consider how to safely export FuncRef when it's implemented. Hence, I believe there's no need to delete the current comments related to FuncRef.

@L-jasmine L-jasmine force-pushed the refactor/sdk_preview branch from cb14a15 to 17221d0 Compare November 17, 2023 18:24
@L-jasmine L-jasmine requested a review from apepkuss November 17, 2023 18:36
@apepkuss apepkuss merged commit 9211bd7 into WasmEdge:refactor/sdk_preview Nov 28, 2023
37 checks passed
@L-jasmine L-jasmine mentioned this pull request Dec 11, 2023
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.

4 participants