-
Notifications
You must be signed in to change notification settings - Fork 715
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
C API: Memory manipulation #2620
base: master
Are you sure you want to change the base?
Conversation
Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR. Overall Summary: The "C API: Memory manipulation" pull request introduces a substantial change to the software by exporting Potential Issues and Errors: The changes incorporated in this pull request introduce potential risks. Key concerns include:
Significant Findings:
DetailsCommit 3395677101f50733a8a159d4533663f98576ab27The pull request titled "C API: Memory manipulation" consists of modifications made across multiple files. Notably, an exported Key changes:
Commit 1aa355a07295c06e7e57ce34305490c909509210The proposed changes aim to add a Free function to the existing memory management API to free dynamically allocated memory. Key Changes:
Potential Problems:
In addition to these code-based issues, there may also be runtime memory errors that may not be caught by static code analysis or testing. It's usually a good idea to perform dynamic code analysis or runtime analysis on code that manipulates memory directly. Commit e782810dd73c12e13669d3b631b96dd2573188d6Changes in this Pull Request:
Potential problems:
Commit e3b92bf2f1ca90cc72a4d9c842b2630ddc6c62c1The given patch is primarily a minor code clean-up for a single file:
The changes pertain to a section of code that deals with the allocation of memory. Potential Problems:
To summarize, while this is a simple change from a code perspective, it could have consequences from a usage and compatibility perspective. As with any changes, they should be followed by thorough testing. Commit d9339d178df5dcfad1909cfb9e9767511544b7ffThis patch targets the Key changes:
Potential Problems:
The code reviewer should ensure that these potential issues are addressed before this patch is accepted. The code should be checked to ensure that the file is correctly moved or copied, and that no other parts of the project will be impacted by this change. Commit aa6145a684869c1bc07aec5097fa537104cf3a5eSummary of Changes: The primary changes made in this patch involve a correction in the file path from "apitestData" to "apiTestData". This change affects several areas:
Potential Problems:
Please ensure these potential issues are addressed to ensure your code performs as per the expectations across different environments. Commit 23626f1114d704f79f6657932277b94241d73eacThe patch introduces changes to the build scripts for Fedora, macOS, Ubuntu, and Windows in the GitHub workflows. The primary modification across all these workflows is the addition of the 'wabt' package to the installation process. 'Wabt' is a suite of tools for WebAssembly (Wasm) that serves various functions during the build process. The key modifications in detail are:
Potential Problem(s):
Commit 8b1c8855987992d6852c910d30c9a2250106cd10This pull request changes the build and test workflow scripts for a project. The key changes include:
There seem to be no major potential issues with the proposed changes. However, it's good to remember that:
A safer approach could be to print an error and fail if Homebrew is not installed, instead of installing it automatically. Commit 9ad0887ced684a7788825b5dd7e9eed24f688f06Summary: The developer has made modifications to the package installation requirements for three GitHub workflow scripts named: IWYU_scan.yml, bindings-java.yml, and build-extensions.yml. The common change across these is the addition of 'wabt' (WebAssembly Binary Toolkit) package to the list of packages being installed. In the IWYU_scan.yml script, 'wabt' was appended to 'dnf install' command. In the bindings-java.yml and build-extensions.yml scripts, 'wabt' was appended to 'apt-get install' and 'apt install' commands, respectively. Potential Problems:
These issues could be mitigated by explicitly indicating a specific version to be installed, ensuring compatibility with other packages, increasing test coverage, and clearly justifying the addition of a new package. Commit 0b7c91d41c0534c8ece21ed4707c31d809c05171Summary of Changes: In each case, the change was to prefix this command with sudo (indicating that the command should be run with superuser privileges), resulting in: Potential Problems:
Given the above points, the author should provide more context or reasons in the comments why these changes are necessary and ensure a proper test suite is in place to validate these changes. Commit b3a24cf6d67c08d490a2e84985a3ca24f91c0414Summary: Key Changes:
Potential Issues:
Overall Recommendation: These changes make the assumption that the 'brew' package manager is always available on the macOS systems for this project. If we can confirm that this is the case for all possible systems where the action workflows are used, then these changes can significantly simplify the scripts. Otherwise, this change might lead to scripts failing on systems where 'brew' isn't pre-installed. A more suitable solution may be to refine the installation script for 'brew' but keep the check for 'brew' in the scripts. Commit a5c000068c2c97b439d70672ff41151f815d7c83This patch introduces the WebAssembly Binary Toolkit (wabt) into the Windows build process. It notably features the following key changes:
Potential issues relating to this patch could include:
Commit 93487ab0e06cd32efc12499a908b3394bd464768The pull request shows a single modification in the build configuration file (.github/workflows/reusable-build-on-windows.yml) for the wabt project. The CMake build command has been changed. The key change concerns the removal of an option during the build process. Specifically, a flag for testing ( Potentially problematic is the fact that by not building the tests, possible issues in the code may go unnoticed. If there are automated tests that are meant to validate the functionality and robustness of the code, disabling those tests could camouflage any malfunctions or bugs in the new builds. If this change is not intentional, or if it was made without thoughtful consideration, it may create issues later. We should request confirmation that this change, that turns automated tests off, is indeed necessary and intentional from the author. A justification should be provided, verifying this change won't introduce unexpected or undetected issues in the future due to lack of automated testing. Commit fc84184f0e712124389b2ac16eaaad7b6804886bThis patch makes two key changes:
Potential Problems:
The most significant part of this patch is the handling of git submodules in the 'wabt' repository. Any issues with this could lead to the 'wabt' library not being built correctly, potentially leading to compilation errors in the application being built. Commit e3b337b53044efe2cc490d556a2b9272615269e5Summary of changes: The developer has modified the GitHub Actions workflow file Potential problems:
Commit 317716472fb5b56867e88ebb145c5bd8101eb4c4The pull request titled "C API: Memory manipulation" makes three changes in total across three files:
Potential issues: Secondly, in the Windows workflow, the specific PATH modification removed might cause some dependencies not to be found if they were being installed in the removed path. Also, adding a new path could introduce unforeseen issues if the contents of the new directory conflict with any pre-existing programs or executables. A potential problem might arise if the build for the windows.yml workflow is relying on something within the Further reviewing the software requires running tests to ensure removal/addition of environment paths doesn’t affect other parts relying on them. |
export_malloc/to_export.c
Outdated
#include <stdio.h> | ||
#include <emscripten/emscripten.h> | ||
|
||
void EMSCRIPTEN_KEEPALIVE fillData(int32_t *buffer,size_t size){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will an exported function like this work? Won't the pointer be converted to i32 in wasm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this function mean?
export_malloc/cmemory.c
Outdated
|
||
WasmEdge_StringDelete(FunctionName); | ||
return (void *)&memory_base_ptr[offset]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this function satisfy the current requirements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's not only the malloc
, but new
, etc. for the malloc functions.
You should follow the detection here to search the exported functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can follow the folders in test
to construct your tests.
export_malloc/Makefile
Outdated
@@ -0,0 +1,6 @@ | |||
test_wasmedge: test_wasmedge.c exported.wasm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use cmake
for building.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can follow the folders in test
to construct your tests.
export_malloc/cmemory.c
Outdated
|
||
WasmEdge_StringDelete(FunctionName); | ||
return (void *)&memory_base_ptr[offset]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's not only the malloc
, but new
, etc. for the malloc functions.
You should follow the detection here to search the exported functions.
export_malloc/to_export.c
Outdated
#include <stdio.h> | ||
#include <emscripten/emscripten.h> | ||
|
||
void EMSCRIPTEN_KEEPALIVE fillData(int32_t *buffer,size_t size){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this function mean?
@q82419 Made changes according to previous comments. Will add new,free etc if current approach is ok. |
Hi @aghinsa , Hope you can make these changes first:
Thanks! |
Signed-off-by: Aghin Shah Alin K <aghinsa@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #2620 +/- ##
==========================================
- Coverage 80.94% 80.92% -0.02%
==========================================
Files 150 150
Lines 21624 21703 +79
Branches 4352 4383 +31
==========================================
+ Hits 17503 17563 +60
- Misses 2954 2967 +13
- Partials 1167 1173 +6
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Done |
@q82419 should I proceed with the current impl? |
Signed-off-by: Aghin Shah Alin K <aghinsa@gmail.com>
Could you please check if the |
@hydai I might be missing some context here, but afaik 'new' and 'delete' are operators and not functions. I cound't find a way to export the operators. Exporting 'new,_new,__new' resutls in 'undefined symbol' error. Any ideas? |
I am confused. Your previous question is I think a simple way is to create a C++ example program with |
@hydai Per your suggetion i tried compiling a function which uses
In the wasm code for Now my doubt is how would a user export the
|
IMO, we should disregard the WasmEdge supports multiple SDKs, including Rust, Golang, Python, C++, Java, and C. Nonetheless, most of these SDKs are merely bindings derived from the C SDK. If these SDKs wish to use different terminologies to characterize |
Yes. I agree. |
Please rebase with the latest master commits. Thanks. |
Done. |
Signed-off-by: Aghin Shah Alin K <aghinsa@gmail.com>
Signed-off-by: Aghin Shah Alin K <aghinsa@gmail.com>
Please fix the related CI failure. |
Signed-off-by: Aghin Shah Alin K <aghinsa@gmail.com>
Signed-off-by: Aghin Shah Alin K <aghinsa@gmail.com>
Signed-off-by: Aghin Shah Alin K <aghinsa@gmail.com>
Signed-off-by: Aghin Shah Alin K <aghinsa@gmail.com>
Signed-off-by: Aghin Shah Alin K <aghinsa@gmail.com>
Signed-off-by: Aghin Shah Alin K <aghinsa@gmail.com>
Signed-off-by: Aghin Shah Alin K <aghinsa@gmail.com>
Signed-off-by: Aghin Shah Alin K <aghinsa@gmail.com>
Signed-off-by: Aghin Shah Alin K <aghinsa@gmail.com>
Signed-off-by: Aghin Shah Alin K <aghinsa@gmail.com>
Signed-off-by: Aghin Shah Alin K <aghinsa@gmail.com>
Signed-off-by: Aghin Shah Alin K <aghinsa@gmail.com>
Signed-off-by: Aghin Shah Alin K <aghinsa@gmail.com>
Addresses #2463
Adds support to use exported malloc/free