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

[feat] Support creating host functions from closures #20

Merged
merged 38 commits into from
Jul 14, 2023

Conversation

apepkuss
Copy link
Collaborator

In this PR, the APIs for creating host functions are updated. Some relevant types are also refactored for the changes.

Signed-off-by: Xin Liu <sam@secondstate.io>
…ction::create_with_data`

Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
…ync_closure`

Signed-off-by: Xin Liu <sam@secondstate.io>
… in `ImportObjectBuilder`

Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
…siModule`

Signed-off-by: Xin Liu <sam@secondstate.io>
…inModuleBuilder`

Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
…inModuleBuilder`

Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
…_host_function`

Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
…dule`

Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
Signed-off-by: Xin Liu <sam@secondstate.io>
…dule` and `PluginModule`

Signed-off-by: Xin Liu <sam@secondstate.io>
Copy link
Member

juntao commented Jul 13, 2023

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


Overall Summary:

The patch introduces multiple changes to add support for creating host functions from closures in the Function and ImportModule modules. It includes new APIs, wrapper functions, and modifications to existing code and tests. However, there are several potential problems and areas that require attention.

The patch introduces new dependencies that might cause compatibility issues or increase the project's size. The implementation of Function::create_async_from_closure is complex and might be difficult to understand and maintain in the future. The use of static variables for storing and retrieving host functions may introduce thread-safety issues if not handled properly. Additionally, there are concerns about error handling, lack of documentation, missing tests, and potential issues in the code.

It is recommended that further review and improvement is done to address these potential problems and ensure the code is maintainable and robust.

Details

Commit 2304ccbdfd45713975369868e2a5c40aafee6ac3

Key changes:

  • Added a new API Function::create_async_from_closure to create an async host function from a closure.

Potential problems:

  • The patch introduces new dependencies (lazy_static, parking_lot, and rand). These dependencies might cause compatibility issues or increase the size of the project.
  • The Function::create_async_from_closure function implementation is quite long and complex. It might be difficult to understand and maintain in the future.
  • The new function uses static variables (ASYNC_HOST_FUNCS and HOST_FUNC_FOOTPRINTS) to store and retrieve host functions. This might introduce thread-safety issues if not handled properly.
  • The code does not have proper error handling. If any errors occur during the creation of the async host function, the function returns a generic FuncError::Create error without providing any specific details. This can make it difficult to debug issues with the function creation.
  • The code does not have proper documentation. It lacks explanations for the parameters, return values, and potential errors of the Function::create_async_from_closure function.

Overall, the patch introduces a new feature to create async host functions from closures. However, it needs further review and improvement to address potential problems and ensure the code is maintainable and robust.

Commit 58a673809a372cce020dd7ff71d8ddb4bf1aa38d

Key changes:

  • Added new API functions Function::create_from_sync_closure and Function::create_with_data in crates/wasmedge-sys/src/instance/function.rs.
  • Added a wrapper function wrap_fn for thread-safe scenarios.

Potential problems:

  • The code is missing comments explaining the purpose and functionality of the new API functions.
  • The code references fn async_cx in wrap_async_fn_from_closure but doesn't define it, which may cause a compilation error.
  • The code contains commented-out code blocks that should be removed.
  • The naming of the create_from_async_closure function is misleading and should be updated to create_from_async_fn.
  • The test module mod tests is missing tests or has incomplete tests.
  • The code is missing documentation for the new API functions and any changes made to existing functions.
  • The code is missing error handling for the create_from_sync_closure and create_with_data functions.

Commit 5c289bd7aecc08c85f59ab695c8421ac770e444d

Key changes:

  • The patch removes a section of code in the function.rs file in the wasmedge-sys crate. It removes 43 lines of code from the impl Function block.

Potential problems:

  • The patch removes code related to creating a function from an async closure. This functionality might be important for users who want to create asynchronous host functions. Removing this code could limit the capabilities of the library.

Overall, the patch seems to be removing a specific feature from the wasmedge-sys crate. However, without more context or information about the reasoning behind the removal, it is unclear whether this change is necessary or if there are alternative solutions. It would be beneficial to review the discussion or documentation related to this patch to understand the motivation behind the change.

Commit ee58b39cf8215941cfa7cbfc274a9a5428ec5997

Summary of key changes:

  • Two new APIs Func::wrap_sync_closure and Func::wrap_async_closure have been added to the function.rs file.
  • Func::wrap_sync_closure allows creating a host function by wrapping a native synchronous closure. It takes a closure as an argument and returns a Func instance.
  • Func::wrap_async_closure allows creating a host function by wrapping a native asynchronous closure. It takes an asynchronous closure as an argument and returns a Func instance.
  • Some modifications have been made to the imports and error handling.

Potential problems:

  • In the test_func_wrap_closure and test_func_wrap_async_closure functions, there are println! statements that may not be desirable for a production environment. It is recommended to remove or comment out these statements.
  • It is unclear why the test_func_wrap_async_closure function is conditionally compiled (#[cfg(all(feature = "async", target_os = "linux"))]). This should be documented or revisited.
  • The code includes a tokio::test annotation, but it is not clear why it is specific to Linux. This should be documented or revised if necessary.
  • It is unclear why the tick function is defined inside the test_func_wrap_async_closure function. It should be moved to a more appropriate location.
  • The vm.run_func_async function is called without awaiting the result in the test_func_wrap_async_closure function. This may lead to unexpected behavior and should be corrected.

Overall, the changes look good and provide useful functionality for creating host functions from closures.

Commit b296a1baa3a38b2f5a83fcb2582d4acaa6fac6ae

Key changes:

  • Two new APIs with_sync_closure and with_async_closure have been added to the ImportObjectBuilder struct.
  • The ImportObjectBuilder struct now imports the CallingFrame, HostFuncError, and WasmValue types.
  • The with_sync_closure API adds a host function from a closure that is able to be used in thread-safe scenarios.
  • The with_async_closure API adds an async host function from a closure that is able to be used in thread-safe scenarios.
  • Both APIs take a name and a function as arguments.

Potential problems:

  • None of the potential problems can be determined from the patch file alone.

Commit 2a8ec9096470a3bb0eed28efc5c27439f626640d

Key changes:

  • Renamed the .github/workflows/test.yml file to .github/workflows/ci-build.yml.
  • Added the --test-threads=1 flag to the cargo test commands in multiple jobs.

Potential problems:

  • The test job changes might impact test execution by limiting the number of test threads to 1. This could make the tests run slower, especially if they were designed to run in parallel.
  • The patch does not provide any explanation or justification for the changes made, so it is unclear why these specific modifications were necessary.

Overall, the changes seem to focus on updating the CI workflow for the Rust SDK, possibly to improve stability or ensure consistent test results. However, more context is needed to fully assess the impact of these modifications.

Commit 80d2c44c2225c0352ed38c3c14bc4c651cbf4c9a

Key Changes:

  • Added a new proc-macro attribute sys_host_function_new to create host functions from closures.
  • Implemented the sys_expand_host_func_new function to define the signature of the wrapper and inner functions.
  • Extracted the identities of the first two arguments and the data argument from the function signature.
  • Generated the token stream to define the wrapper and inner functions.

Potential Problems:

  • The code does not handle the case when the item_fn is not an Item::Fn. In this case, the function returns an empty TokenStream without any error message or indication of the issue.
  • The code uses panic! in some places to handle unexpected cases, which can lead to a program crash. It is better to handle these cases gracefully by returning an error if needed.
  • The code assumes that the number of host function arguments will always be either 2 or 3. If the number of arguments is different, it will panic with an "Invalid numbers of host function arguments" message. It might be more appropriate to return an error in this case.

Commit 47a2d9bfe4ba4da976141dcd83e6f27aea9620fa

Key changes:

  • Added new APIs in the Function and ImportModule modules in the wasmedge-sys crate.
  • Added support for creating host functions from closures.
  • Added a new create_from_sync_closure function in the Function module.
  • Added a new wrap_fn_new function as a wrapper for thread-safe scenarios.
  • Added a new wrap_async_fn_new function as a wrapper for thread-safe async scenarios.

Potential problems:

  • The patch adds new APIs and modifies existing code. It is important to review the code changes thoroughly to ensure correctness and prevent any potential issues.
  • The changes involve multiple modules (executor.rs, function.rs, module.rs, lib.rs, plugin.rs, store.rs, common.rs, test_aot.rs), so it is important to review the changes in each module carefully.
  • The patch introduces new functions (create_from_sync_closure, wrap_fn_new, wrap_async_fn_new). It is important to review these functions to ensure they are implemented correctly and do not introduce any errors or performance issues.
  • The changes involve modifications to existing tests (test_executor_register_import, test_executor_run_async_host_func). It is important to review these test changes to ensure they cover all relevant scenarios and produce correct results.
  • The patch introduces several new dependencies and uses features like async and target_os. It is important to ensure that these dependencies are correctly resolved and the code is compatible with the specified features.
  • The patch includes changes to documentation comments. It is important to verify if these changes accurately describe the code changes and provide useful information to users.
  • The patch adds and deletes lines of code. It is important to review these code changes for potential issues such as introducing bugs, removing necessary functionality, or modifying existing behavior.

Commit bb3d302f206854feaadccb69620b1b0c7e871470

Summary:

The patch adds new APIs to the Func and ImportObjectBuilder structs. These APIs allow users to create host functions from closures and native functions, with support for additional data objects and async functionality.

The key changes in the patch include:

  1. Addition of a new new_new function to the Func struct, which creates a host function from a closure or native function along with additional data.
  2. Addition of a new wrap_new function to the Func struct, which creates a host function by wrapping a native function.
  3. Addition of a new wrap_async_new function to the Func struct, which creates an asynchronous host function by wrapping a native function.
  4. Code changes in the Function and Func structs to support the new APIs.
  5. Code changes in the ImportObjectBuilder struct to support the new APIs.

Potential Problems:

  1. The patch introduces multiple new functions and changes to existing code, which increases the complexity of the codebase.
  2. The functions in the Func struct have generic types (T) and multiple type constraints (Send, Sync, 'static), which may make the code harder to understand and maintain.
  3. The code includes multiple conditional compilation attributes (#[cfg]), which may make the code less portable and harder to test across different platforms.
  4. The patch modifies multiple files and includes a significant number of changes, which may introduce bugs and regressions in the codebase.
  5. There is a potential issue with the use of raw pointers (*mut std::os::raw::c_void) in the real_add function, which may lead to unsafe behavior and memory-related issues.

Commit e05df22235701d63271c4415280b6edde74699d9

The key changes in this pull request are:

  1. Refactored the Function, ImportModule, and AsyncWasiModule structs and their related methods.
  2. Updated the usage of the Function::create method to the new Function::create_sync_func and Function::create_async_new methods.

Potential problems:

  1. The refactored code in Function, ImportModule, and AsyncWasiModule should be carefully reviewed to ensure that the changes do not introduce any bugs or regressions.
  2. The new Function::create_sync_func and Function::create_async_new methods should be reviewed to ensure they correctly handle the creation of host functions from closures.
  3. The refactored code in executor.rs, function.rs, module.rs, table.rs, store.rs, and common.rs should be reviewed to ensure that the changes do not introduce any bugs or regressions.
  4. The unit tests in test_aot.rs should be reviewed to ensure they still pass after the refactoring.

Commit 6da86980ca563391d3b3e9a4e6ab3d31036e63ec

Key changes in this pull request:

  • The Func struct has been refactored.
  • The Func::new function has been removed.
  • The Func::wrap function has been renamed to Func::wrap_new.
  • The Func::wrap_sync_closure function has been removed.

Potential problems:

  • The Func struct has been extensively modified, which may introduce bugs or regressions. It would be beneficial to thoroughly test the changes to ensure the functionality has not been impacted.
  • The removal of Func::new and Func::wrap_sync_closure may break existing code that relies on these functions. It would be helpful to flag down these breaking changes and document alternative solutions or migration paths.
  • It would be good to evaluate the impact of the refactoring on code readability and maintainability. Are the changes improving the codebase? Is there potential risk or complexity introduced?

Commit 20e2abf1d19009bb20c6f5c944fe6da6a8aeda61

The key change in this patch is the refactoring of the Function and ImportModule implementations in the wasmedge-sys crate. Specifically, the create_async_new method in the Function implementation has been renamed to create_async_func. Additionally, the create_from_async_closure method has been removed.

A potential problem with this change is that the removal of the create_from_async_closure method may break existing code that relies on it.

Other than that, the patch mainly consists of code removals and some minor changes to example code and tests.

Commit 230cc19f516e98dea20b3e8ff5e6f79a2a37b6a9

Key changes:

  1. The Func struct has been refactored to have separate methods for creating synchronous and asynchronous host functions.
  2. The ImportObjectBuilder and PluginModuleBuilder structs have been refactored to use the new Func methods.

Potential problems:

  1. The wrap_async_closure method has been removed from the Func struct, but it is still being used in the ImportObjectBuilder struct. This will cause a compilation error. The method has been replaced with the wrap_async_func method.

Recommendation:

  1. Update the usage of the wrap_async_closure method in the ImportObjectBuilder struct to use the wrap_async_func method instead.

Commit c29f49b3a95374aa9b1e7b184644597c034be38d

Key Changes:

  • Fixed clippy issues in the codebase.

Potential Problems:

  • The patch appears to be a chore fix, so there should be no direct impact on functionality or behavior.

Commit c3893816d78de43df8a6538b1e6e0a44888ace68

Key changes:

  • The patch modifies three files: src/externals/function.rs, src/import.rs, and src/plugin.rs.
  • In src/externals/function.rs, the line use crate::r#async::{AsyncHostFn, AsyncState}; is removed. This is likely due to a change in the codebase's organization or structure.
  • In src/import.rs, the line #[cfg(all(feature = "async", target_os = "linux"))] and the associated import use crate::r#async::AsyncHostFn; are removed. Similar to the previous change, this is likely due to a change in the codebase's organization or structure.
  • In src/plugin.rs, the line use crate::r#async::AsyncHostFn; is removed. Again, this change is likely due to a change in the codebase's organization or structure.

Potential problems:

  • It's unclear why the imports for AsyncHostFn and AsyncState are being removed in multiple files. If this functionality is still required, it may be necessary to investigate why these changes were made and whether they will have any impact on the code's behavior.
  • It's also important to consider whether the removal of these imports may cause any compatibility issues or issues with other parts of the codebase that relied on these imports. It would be beneficial to review the codebase's history and discussions to better understand the context of these changes.

Overall, the patch appears to mostly involve removing unused imports, but the removal of AsyncHostFn and AsyncState imports raises some questions that should be addressed.

Commit 6e19df79b561da3b9f4bfc378f1f1af57567c6e1

Key Changes:

  • Added compilation configuration in chore(rust-sys): add compilation config
  • Added support for creating host functions from closures
  • Added new wrapper functions for thread-safe scenarios

Potential Problems:

  • The patch does not provide any details or explanations for the changes made.
  • The patch adds new functions without documenting their purpose or usage.
  • It is unclear why the compilation configuration was added and what it entails.
  • The patch includes functions that are conditionally compiled based on the async feature and target OS linux, but it is unclear why these conditions are necessary.

Overall, more documentation and context are needed to understand the purpose and implications of these changes.

Commit 5efde4dd0012667d230385f0b78a793ec38627c7

Key changes:

  • Two lines of code were removed from the executer.rs file.

Potential problems:

  • It is not clear why these lines were removed without any explanation or comments.
  • The commit message does not provide much information about the purpose of the change or the reasoning behind it.
  • There is no reference to any issue or discussion related to this change.

Overall, the removal of code without clear justification or context raises concerns about the impact on the functionality of the codebase. It would be helpful to have more information and context regarding this change.

Commit eee34b0e3035df9b3478c1cd68edf4c4708ecc97

Key changes:

  • Fix clippy issues in the function.rs file.
  • Import the ValType type from the wasmedge_types module.
  • Remove the import of the HostFn type from the function module.

Potential problems:

  • The commit message mentions fixing clippy issues, but it doesn't provide any details about what specific issues were fixed. It would be helpful to have more information about the changes made.
  • It is unclear why the HostFn type was removed from the import list in the lib.rs file. This removal may cause compilation errors if the HostFn type is used elsewhere in the codebase.

Commit 0f25848127b45abdff2a816b5458aeefba2936c9

Key changes in the patch:

  • The patch modifies the definition of the HostFn type.
  • The HostFn type is restricted to wasmedge_sys::instance::function::HostFn<T> when the async feature is enabled and the target operating system is Linux.

Potential problems:

  • This patch seems to address a clippy issue, but the specific issue is not mentioned in the commit message or code comments. It would be helpful to have more information about the clippy issue being fixed.
  • The condition #[cfg(all(feature = "async", target_os = "linux"))] might be too restrictive. If the intent is to only apply this change when both the async feature is enabled and the target OS is Linux, it might be worth considering whether this is the desired behavior. If this is correct, it should be documented or communicated clearly to users and contributors.

Commit 4e419d65a33ba239b18fa23a10de7d5f43d55ebd

The key changes in this patch include:

  1. Importing the NeverType and ValType types in crates/wasmedge-sys/src/instance/function.rs. This suggests that the patch may add support for these types in certain parts of the code.

  2. Importing the NeverType, ValType, Mutex, and Arc types in crates/wasmedge-sys/src/instance/module.rs. This patch may introduce new features or behavior related to these types in the module.

Potential problems:

  1. There are no obvious potential problems in this patch.

Overall, this patch seems to introduce some changes related to types and imports in the codebase. It would be helpful to have more context or additional patches to fully understand the impact and purpose of these changes.

Commit f3b461a8d6bf6e3c176796e575004c34bf2ae841

Summary of key changes:

  • In the vm.rs file, the code has been updated.
  • The function with_func in the ImportObjectBuilder struct has been modified.
  • The real_add function has been modified.

Potential problems:

  1. In the with_func method call, the third parameter None has been added, but its purpose is not clear from the provided code. It might be necessary to review the documentation or code logic to understand its significance.

  2. The real_add function has changed its signature by replacing the Option<&mut T> parameter with a *mut std::os::raw::c_void parameter. It is important to review the context in which this function is used to ensure that the change is appropriate and doesn't introduce any unintended side effects.

Please note that a more comprehensive review might require additional context and information about the purpose and usage of the modified code.

Commit 0924d979d06742f383c1006de3da84cbab1edaec

Key changes:

  • Updated the host_function proc-macro in the wasmedge-macro crate.
  • Reorganized the code and removed unused imports.
  • Changed the signature of the generated wrapper function.
  • Implemented support for creating host functions from closures.

Potential problems:

  • The code makes use of unwrap() and panic!() in several places, which could result in runtime errors or panics if the input is not in the expected format.
  • It is not clear why the wrapper_fn_inputs are modified in a specific way. Further explanation and documentation might be necessary to understand the intention behind these modifications.
  • The use of unsafe code when casting data to #ty_ptr could introduce potential memory safety issues. Proper validation and handling of the input should be implemented, such as proper error handling or using a safe alternative.
  • The code does not handle cases where the number of function arguments is less than or greater than 2 or 3. Proper error handling and validation could be added to handle these cases more gracefully.

Commit a4715b19a1dfdbf02a9b7b0ff8770aa1e3a8d17f

Key changes:

  • Refactored code in multiple files including ast_module.rs, compiler.rs, config.rs, executor.rs, function.rs, and others.
  • Removed unused imports and organized the imports.
  • Changes related to HOST_FUNCS and HOST_FUNC_FOOTPRINTS variables.

Potential problems:

  • It's not clear why the code was refactored and what specific changes were made in each file.
  • The changes related to HOST_FUNCS and HOST_FUNC_FOOTPRINTS are unclear. It's not clear why the variables were changed and how they are used.
  • In some places, the code is accessing variables in a multi-threaded context (HOST_FUNCS and HOST_FUNC_FOOTPRINTS). Potential issues related to concurrency and thread safety should be carefully reviewed.
  • The patch doesn't include any comments or explanations for the changes. It would be helpful to have more context and explanations for the changes made.

Commit 6e69bd51acb834ddce067d23dbe65cc5526bbc4a

Key Changes:

  • Added support for creating host functions from closures in the wasmedge-macro crate.
  • Added new macros: sys_wasi_host_function, sys_async_wasi_host_function, sys_expand_wasi_host_func, sys_expand_async_wasi_host_func_with_two_args, sys_expand_async_wasi_host_func_with_three_args.

Potential Problems:

  • The code does not handle cases where the number of host function arguments is not 3. This could lead to unexpected behavior or crashes if the user tries to create a host function with a different number of arguments.
  • The code uses the panic! macro in several places. Instead of panicking, it would be better to return a syn::Result with an appropriate error message.
  • The code relies on hard-coded types (*mut std::os::raw::c_void, Vec<WasmValue>, HostFuncError) in several places. It would be better to make these types configurable or more generic to support different use cases.
  • The code does not have proper error handling for the async functions (sys_expand_async_wasi_host_func and sys_expand_async_wasi_host_func_with_two_args). If there is an error during expansion, it currently panics. It would be better to return a syn::Result with an appropriate error message.
  • The code does not have proper error handling for the async functions (sys_expand_async_wasi_host_func and sys_expand_async_wasi_host_func_with_two_args). If there is an error during expansion, it currently panics. It would be better to return a syn::Result with an appropriate error message.
  • The code does not provide any examples or documentation on how to use the new macros. It would be helpful to provide some usage examples and additional documentation to guide users on how to use the macros correctly.
  • The code does not include any tests to verify the correctness of the new macros. It would be beneficial to include test cases to ensure that the macros work as expected in different scenarios.

Commit 5f0ae871f18edadb29e249f163f04c9ff6c3f105

Key changes:

  • Updated code in various files related to host functions.
  • Added new host functions in the async_wasi.rs file.

Potential problems:

  • It is unclear what the purpose of the changes is without additional context.
  • There could be potential issues with the new host functions added in async_wasi.rs. It would be helpful to have more information about the changes and their intended functionality.
  • It would also be important to ensure that the updated code in other files is functioning as expected and does not introduce any regressions or compatibility issues.

Overall, further review and clarification are needed to fully assess the changes and potential problems.

Commit 97e6dd4f45976074bec1be02dae3895611e5a445

Key changes:

  • Added a new parameter _data of type *mut std::os::raw::c_void to the following functions: async_hello in src/executor.rs, real_func in src/externals/function.rs, and real_func in src/import.rs and src/plugin.rs.

Potential problems:

  • The purpose and usage of the new _data parameter are not clear from the provided code. It would be helpful to have comments explaining its purpose and how it should be used.
  • There is no documentation or explanation for why the _data parameter is added. It's important to provide context and rationale for such changes.
  • The code changes look fine, but without the full context and understanding of the project, it's difficult to identify any potential problems or implications of these changes.

Overall, the addition of the _data parameter needs more explanation and documentation to ensure it is used correctly and meets the project's requirements.

Commit 3229b3fc6556ebf3f1d1a852e0d414384949ecc6

Key changes in the patch:

  • Formatting changes in ast_module.rs, compiler.rs, and instance/table.rs files.
  • Import statements reordering in ast_module.rs, compiler.rs, and instance/table.rs files.

The most important finding is that the patch mainly consists of formatting changes and reordering import statements. These changes do not affect the functionality of the code.

Commit 8f8381f736c78767809681c1928a2342a5231529

Key changes:

  • Renamed the function with_func_new to with_func in the PluginModuleBuilder implementation in the plugin.rs file.

Potential problems:

  • It's not clear why the function name was changed. The pull request description or commit message does not provide any explanation. The reviewer should ask for clarification or check if there was any related discussion or issue before approving the change.
  • The commit message does not provide enough context about the purpose or motivation behind this change. The reviewer should ask for more information to understand the reasoning behind the rename.
  • This change seems to be a straightforward renaming without any functional or behavioral modifications. However, without additional context, it's hard to determine if there are any potential issues or unintended consequences related to this change. The reviewer should carefully review the surrounding code and dependencies to ensure that the renaming did not introduce any issues.

Commit 3c11037de6c64b3b3f21fbcbbaff42c164bfc8ac

Key changes:

  • Added a new field funcs to the PluginModule struct.
  • Modified the add_func function to store the Function in the funcs vector before calling the ffi function to add the function to the module instance.

Potential problems:

  • In the Drop implementation, the funcs vector is being drained but it is not clear why. It would be helpful to have some comments explaining the purpose of this code.
  • The add_func function is no longer marking the Function as registered. This might be an intentional change, but it should be clarified in the code or with comments.

Overall, the changes seem to be introducing support for creating host functions from closures in the PluginModule struct. The new funcs vector is used to store the functions before adding them to the module instance. There are a couple of areas that could benefit from additional documentation or comments to clarify the intent and reasoning behind the changes.

Commit 78b37b0447206feb19ba17d69d123509f1f3052c

Key changes:

  • Added dependencies on parking_lot crate for Mutex usage.
  • Replaced several instances of std::sync::Arc::new with Arc::new(Mutex::new(...)) in various structs.
  • Replaced uses of inner struct pointers with self.inner.lock().0 to acquire mutable access to the inner instance.

Potential problems:

  • The changes introduce the use of Mutex in several structs, likely to enable synchronization of shared data between threads. This dependency should be evaluated to ensure it is necessary and appropriate for the codebase.
  • The patch includes changes to Global and Memory structs. It is unclear if these changes were necessary and sufficient.

Additional observations:

  • There are several instances of dbg! statements that seem to be for debugging purposes. These should be removed once the code is ready for merging.
  • There are some instances of Arc::strong_count being used in the Drop trait implementation to determine when to delete a resource. This may not be the best way to handle resource cleanup and should be reviewed for correctness.
  • There are also statements in Drop trait implementations assigning std::ptr::null_mut() to resources. The purpose of these statements is not clear and should be reviewed to ensure they are correct and necessary.
  • There seem to be missing parts of the patch as there is no context provided on what these changes are introducing or why they are necessary.

Commit 550b3b9dba8eb1fc30a2de1bad0419aaa5981831

Key changes:

  • Added a new field memories to the PluginModule struct.
  • Added a new method add_memory that adds a memory to the PluginModule.
  • Modified the add_table method to set the inner field of the Table struct to nullptr.
  • Modified the add_global method to set the inner field of the Global struct to nullptr.

Potential problems:

  • The drop method of the PluginModule struct is not thread-safe as it does not use synchronization primitives when accessing and modifying the funcs and memories vectors.
  • The add_memory method does not set the registered field of the Memory struct, which may cause issues if this field is used elsewhere.
  • The add_table method sets the inner field of the Table struct to nullptr, which may break other code that depends on this field.
  • The add_global method sets the inner field of the Global struct to nullptr, which may break other code that depends on this field.

Overall, the code changes seem to introduce potential problems, mainly related to concurrency issues and breaking existing code. These issues should be addressed before merging the pull request.

Commit 07431b75265c964ae64fff5ce27e0b8522be54d0

Key changes:

  • The finalizer function type has been removed from the plugin.rs file.
  • The create method of the PluginModule struct has been modified, removing the finalizer parameter and replacing it with the host_data_finalizer function.

Potential problems:

  • The removal of the finalizer parameter might have implications for the overall functionality of the code. It would be important to review the code to ensure that the removal of this parameter does not introduce any issues or unwanted behavior.
  • The implementation of the host_data_finalizer function should be reviewed to ensure that it correctly handles the freeing of the host data and avoids any potential memory leaks or other issues.

Commit 318c9e1bcd5f4c2a28c4ca9fd5781b7c38b4a01e

Key Changes:

  • The Finalizer type has been removed.
  • The host_data field in PluginModuleBuilder no longer has an associated Finalizer.
  • The with_host_data method in PluginModuleBuilder no longer takes a Finalizer argument.
  • The PluginModule::create method in the build function no longer takes a Finalizer argument.

Potential Problems:

  • It appears that the functionality related to the Finalizer type has been removed. It's unclear if this was intentional or accidental. It would be important to verify with the author why this change was made and if it aligns with the intended behavior of the code.
  • If the removal of Finalizer was intentional, then any code that relies on the Finalizer functionality would need to be updated accordingly.

Commit ec946dd83da80f3c147e6e3c8be8c5f0db580ef6

Key Changes:

  • Debugging code has been removed from multiple files (executor.rs, function.rs, global.rs, memory.rs, module.rs). The dbg!() macro calls and debug print statements have been removed.

Potential Problems:

  • It is unclear why the debugging code has been removed. Debugging code is often useful for troubleshooting and finding issues. Removing it might make it harder to debug problems in the future.
  • The comments and debug print statements say that certain objects are being dropped or freed, but it is not clear if this is the expected behavior or if there are any implications. It would be helpful to have better comments explaining the rationale for these changes.

Overall, the main change is the removal of debugging code. This should not affect the functionality of the code, but it might make troubleshooting and debugging more difficult in the future. It would be helpful to have more context and explanations in the comments to understand the reasoning behind these changes.

Commit f0477077c26190c888a60e45e3a9acd0b13f1693

Key Changes:

  • The version of the wasmedge-macro crate is bumped from 0.4.0 to 0.5.0.

Potential Problems:

  • No potential problems found in this patch.

Commit 4d9616c214b970337b29824d68faae46d2558cd6

Key changes:

  • The version of a crate called rust-sys has been bumped to 0.15.0.
  • The version of two other crates, wasmedge-macro and wasmedge-types, have been bumped to 0.5 and 0.4 respectively.

Potential problems:

  • It is unclear why the version of rust-sys has been bumped. The Pull Request does not provide any context or explanation for this change. It would be helpful to have more information about why this bump is necessary.
  • Similarly, the reasons for bumping the versions of wasmedge-macro and wasmedge-types are not mentioned in the Pull Request. It would be beneficial to understand the motivations behind these changes.
  • Without more context, it is difficult to assess whether these version bumps are necessary or could potentially introduce compatibility issues with other dependencies.

Overall, more information and context should be provided to better understand the motivations and implications of these version bumps.

Commit 1614292b8b1b13128dc60fb224f1cb64f7608ecc

Key changes:

  • The version of the project has been bumped from "0.9.0" to "0.10.0-dev" in the Cargo.toml file.
  • The versions of the wasmedge-macro and wasmedge-sys dependencies have also been bumped.

Potential problems:

  • The new version number "0.10.0-dev" suggests that this is a development version. It is important to ensure that this update does not introduce any breaking changes or regressions.
  • It is unclear from the provided patch what other changes have been made in this version update. It would be helpful to review the commit history or additional patches to get a complete understanding of the changes.

Overall, it is necessary to carefully review the changes made in this version update and ensure that they are properly tested and do not introduce any issues.

Commit 66fee7d359cc1e484ff0511f0f62763cf4754e14

Key changes:

  • Refactored the ImportModule, WasiModule, AsyncWasiModule, and PluginModule structs.
  • Removed the memories field from these structs.
  • Modified the Drop implementation for ImportModule, WasiModule, AsyncWasiModule, and PluginModule to remove the registered host functions instead of dropping the memories.

Potential problems:

  • The refactored code removed the handling of memories in ImportModule, WasiModule, AsyncWasiModule, and PluginModule. It is unclear if this change is intentional or accidental. This should be clarified and confirmed if the removal of memory handling is intended or if it needs to be added back.

Overall, the changes seem to be related to removing memory handling from the module structs and modifying the drop behavior for registered host functions. The impact of these changes should be reviewed and validated to ensure they do not introduce any issues.

@apepkuss
Copy link
Collaborator Author

@L-jasmine Could you please help review this PR? Thanks a lot!

Copy link
Collaborator

@L-jasmine L-jasmine left a comment

Choose a reason for hiding this comment

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

Because this modification is intended for backward compatibility with the old version of the API, but the old version of the API has some hidden issues, this PR will also introduce some code that can pass the compilation check but shouldn't have passed. It is necessary to fix these issues in subsequent PRs as soon as possible.

@apepkuss
Copy link
Collaborator Author

@L-jasmine Thanks for the review!

@apepkuss apepkuss merged commit 68f6b13 into WasmEdge:main Jul 14, 2023
@apepkuss apepkuss deleted the feat/support-closure-new branch July 14, 2023 04:49
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.

3 participants