[refactor] Introduce WasmEdge_FunctionInstanceGetData to drop host data

[apepkuss]

In this PR, the new C-API WasmEdge_FunctionInstanceGetData is introduced into the drop methods of ImportModule and Function structs to free host data.

In addition, the following minor changes are involved:

• Bump wasmedge-sdk to 0.13.0
• Update dependencies: fiber-for-wasmedge v14.04, bindgen v0.69
• Update CI workflows: Rust to 1.73, macOS to macos-13

[juntao]

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

---

Overall summary:

The overall summary of the pull request titled "[refactor] Introduce WasmEdge_FunctionInstanceGetData to drop host data" is as follows:

The pull request introduces changes related to the dropping of host data in the Function and ImportModule structs. These changes involve modifications to the drop implementations and the addition of a new function called WasmEdge_FunctionInstanceGetData.

There are several potential issues and errors identified in the pull request. These include potential problems with memory management, dangling references, and possible double free errors. It is important to carefully review the changes and ensure that they are correct and do not introduce any unexpected behavior.

In terms of important findings, the addition of the WasmEdge_FunctionInstanceGetData function and the modifications to the drop implementations are the main changes of significance. The handling of host data and the ownership of host functions and import modules should be carefully reviewed and validated.

Overall, the changes in this pull request require thorough review and testing to ensure they align with the intended functionality and do not introduce any problems or errors.

Details

Commit dc114658897f4879a3b314283e7d2a87788eacfd

Key changes:

• The drop implementation of the Function struct has been modified to drop the host_data associated with the function instance.

Potential problems:

• The host_data is being dropped using Box::from_raw on a raw pointer obtained from a C function call. It is important to ensure that the host_data was originally allocated using Box::into_raw. If it wasn't, this could result in undefined behavior.
• It's unclear if the host_data is expected to be dropped for all function instances or only when they are being deleted. The intention should be clarified in the documentation or code comments.

Other observations:

• No other significant problems or areas of concern were identified.

Commit ac89b2a574b42c16d9d2907ae369ba0188cf2cce

The key changes in this pull request are as follows:

1. Added a new function WasmEdge_FunctionInstanceGetData to get host data.
2. Removed the code that set the inner pointer to null when dropping a function.

Potential problems:

1. The drop implementation for Function no longer sets the inner pointer to null, which may lead to dangling references if the Function is used after being dropped.
2. The removal of the code that deletes the function instance may introduce memory leaks, as the memory allocated for the function is not freed.
3. In the test function test_func_drop_v2, the delete function is not called on the FunctionInstance, which may also cause memory leaks.

Overall, the changes in this pull request introduce potential problems related to memory management and dangling references.

Commit 150b0bbe62c9147c50db494203927a5a2887f873

Key changes in this patch include:

• The removal of the use of Box::from_raw to drop host data in the function.rs file. The comment explains the reason for this change.

• The addition of an ignore attribute before the test_vmbuilder function in the vm.rs file. There is also a comment indicating that the test requires the installation of the wasi_crypto plugin.

Potential problems:

• The use of Box::from_raw to drop host data in the function.rs file has been changed without any explanation. This change should be reviewed carefully to ensure it doesn't introduce any issues or unexpected behavior.

• It's not clear why the test_vmbuilder function in the vm.rs file is being ignored. The reason for this should be clarified or the ignore attribute should be removed if it's not necessary.

Overall, it seems like the changes in this patch are relatively minor. However, it's important to review the modified code and the reasons behind the changes to ensure they are correct and don't introduce any problems.

Commit 68a8abdaf3e020f8bd9f34973bdbdf5cf757c166

Key changes:

• The Function struct has a new field data_owner which indicates whether the host context data is owned by the host function.
• The Function struct has a new method create_with_data that takes an additional data_owner parameter.
• The Function struct has a new implementation of the Drop trait that drops the host data if it is owned by the host function.
• The ImportModule struct has a new field data_owner which indicates whether the import module data is owned by the host function.
• The ImportModule struct has a new implementation of the Drop trait that drops the host functions and the module instance.

Potential problems:

• In the Drop implementation of Function, there is a check to ensure that the host function is not registered before removing it from the HOST_FUNCS container. However, if the check fails, there is a missing error handling code and a panic is raised instead. It should be handled in a more graceful way.
• In the Drop implementation of Function, the host data is dropped even if the host function is still registered. This could lead to a double free error if the host data is borrowed by the host function after it has been dropped.
• In the Drop implementation of Function, there is a missing check for the host data owner flag before dropping the host data. This could lead to a double free error if the host data is not owned by the host function and is dropped twice.
• In the Drop implementation of ImportModule, there is a missing check to ensure that the import module is not registered before removing the host functions from the funcs container. It should be handled in a similar way as in the Drop implementation of Function.
• In the Drop implementation of ImportModule, there is a missing check for the host data owner flag before dropping the host functions. This could lead to a double free error if the host data is not owned by the host function and is dropped twice.

Commit 1078cdbcf57cc74a7352a43a63a9521a0da79c3a

Key changes in this patch:

1. Introduced a new function WasmEdge_FunctionInstanceGetData to drop host data.
2. Modified the drop implementation for ImportModule to use the new function to drop host data before dropping the module instance.

Potential problems:

1. In the drop implementation for ImportModule, the condition if !self.registered && Arc::strong_count(&self.inner) == 1 && !self.inner.0.is_null() is used to determine if the module instance should be dropped. This condition assumes that no other references to self.inner exist, which may not always be true.
2. The unsafe block that drops the host data in the drop implementation may not be handling all cases correctly. It would be helpful to have more context on what the WasmEdge_FunctionInstanceGetData function does and how it should be used.
3. There are several calls to dbg! and println! throughout the code, indicating possible debugging statements. These should be replaced or removed before the pull request is merged.
4. In the drop implementation, the comment // remove the real_func from HOST_FUNCS is misleading since the host function is not actually removed from the HOST_FUNCS container.

Overall, the patch brings in a new function to handle dropping host data and updates the drop implementation accordingly. However, there are potential issues and the code could benefit from further explanation and cleanup.

Commit 56f3e286bd9b5cd709aa49415cfd93da50592895

Key Changes:

• Removed debug code from the impl Drop for Function implementation in function.rs.
• Changed the visibility of HOST_FUNCS and HOST_FUNC_FOOTPRINTS to pub(crate).

Potential Problems:

• No potential problems identified. The changes seem to be straightforward and aimed at cleaning up the codebase.

Commit c41e2abb33c0076434c44b39aefb843bb12d81cf

Key changes:

• The dbg! statements that were used for debugging purposes have been removed.

Potential problems:

• Removing the dbg! statements may make it harder to debug the code in the future if similar issues arise.
• It is unclear why the named_func instance is being dropped after the function execution. This should be documented or discussed with the author to ensure it is intentional and does not introduce any unintended consequences.
• The commit message does not provide much context or explanation for the changes. Adding more details to the commit message would be beneficial for understanding the purpose of the changes.

Commit f503c4d306598a18f4cd7b619bbbe8bf110c2b9b

Key changes:

• The version of the rust-sdk in the Cargo.toml file is being bumped from "0.12.3-dev" to "0.13.0".

Potential problems:

• No potential problems are evident from the provided patch. However, it would be helpful to review the entire pull request to ensure that this version update does not introduce any breaking changes or compatibility issues with other dependencies or modules.

Overall, this patch appears to be a straightforward version update for the rust-sdk.

Commit 59395caae5f37579f651dc87be111bc433fd8972

Key Changes:

• Added #[allow(clippy::from_raw_with_void_ptr)] attribute to Drop implementation in Function and ImportModule structs.

Potential Problems:

• No potential problems were identified based on the provided patch. The changes seem to address Clippy warnings and are not likely to introduce any issues.

Commit f240c8f0de31bf6ecc1917258ec86898cc5d984f

The key change in this patch is the update of the Rust version used in the CI build. The list of supported Rust versions has been updated to include 1.73, 1.72, and 1.71, and the previous version 1.70.0 has been removed.

Potential problems:

1. There is no justification provided for updating the Rust version. It's important to ensure that the updated version is compatible with the codebase and doesn't introduce any issues.
2. The patch does not include any additional information or context about the change.

Overall, the patch seems to be a routine update to the CI build configuration. It would be helpful if the patch description provided more details about the motivation behind the update and any potential risks or considerations. Additionally, it's important to verify that the updated Rust versions are compatible with the codebase and don't introduce any new issues.

Commit 8b711ede19415ec73318a085f6695dfd3d5f034f

Key changes:

• The patch updates the Rust version in the .github/workflows/standalone.yml file.
• The supported Rust versions are updated from [1.72, 1.71, 1.70.0] to [1.73, 1.72, 1.71] for all the specified jobs.

Potential problems:

• It is not clear why the Rust version is being updated. The pull request description or commit message should provide more context on why this change is necessary.
• It would be helpful to include information on any known issues or incompatibilities with the updated Rust version.
• The patch only updates the Rust versions in CI jobs. If there are other parts of the code that require changes or updates for compatibility with the new Rust version, those should be addressed as well.

Overall, the change seems straightforward, but more information and context are needed to fully understand the reasons behind the update and to assess any potential problems.

Commit 0a2153c544894ea11070660b560118759d301b92

Summary of changes:

• The patch updates the required Rust version for wasmedge-sys from v1.70 to v1.71.
• A table is added to the documentation, listing the versions of various dependencies.

Potential problems:

• There don't appear to be any potential problems with this patch. It's a straightforward update to the Rust version requirement and an addition of a table to the documentation.

Commit 73c88dbfcc4e3bbcc49be69de4c3daf71b21ec0e

Key changes:

• In the README.md file, the minimum supported Rust version was updated from 1.70 to 1.71.
• In the lib.rs file, the minimum supported Rust version was updated from 1.70 to 1.71.

Potential problems:

• There are no apparent problems with these changes. They simply update the minimum supported Rust version in the documentation to reflect the actual requirement.

Commit 60d5003883eef42ce3fee9a4aadb4ba78db0832d

Key changes:

• The version of the fiber-for-wasmedge dependency has been updated from 8.0.1 to 14.0.4.
• The version of the bindgen dependency has been updated from 0.65 to 0.69.

Potential problems:

• It's unclear from the provided patch whether the updated dependencies have been thoroughly tested and are compatible with the existing codebase. It would be ideal to include information about the changes and any relevant testing that has been performed.
• If the updated dependencies introduce breaking changes or require additional modifications to the codebase, it would be helpful to include those changes in the patch or as additional context.

Overall, the patch seems to be a straightforward update of dependency versions. However, further information and testing details would be beneficial to ensure the changes are properly integrated.

Commit dcb751f923618fde6c342a37219255f721788b61

Key Changes:

• Updated the CI builds to use macOS 13 instead of macOS 11 and macOS 12.

Potential Problems:

• No potential problems are apparent from this patch. The changes seem to be straightforward and solely focused on updating the CI configuration.

[apepkuss]

@hydai Could you please help review this PR? Thanks a lot!

[apepkuss]

@hydai Thanks for the review!