C API: Memory manipulation

[aghinsa]

Addresses #2463

Adds support to use exported malloc/free

[juntao]

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 malloc and free functions for memory allocation and deallocation. This modification affects multiple files: wasmedge runtime, API interface, error message enumerations, and module instantiation. Test coverage for these additions is provided in a new file named APIMallocTest.cpp. Correspondingly, there are modifications to CMakeLists.txt and several GitHub Action workflow scripts to accommodate the new tests and dependencies— notably the WebAssembly Binary Toolkit, 'wabt'.

Potential Issues and Errors:

The changes incorporated in this pull request introduce potential risks. Key concerns include:

1. Memory error handling is not thoroughly covered, particularly for freeing a memory region more than once or freeing an unallocated region, leading to undetermined behavior.
2. No consideration seems to run-time errors or memory leaks arising from the direct memory manipulation, typically caught by dynamic analysis.
3. File path modifications and renaming may lead to system-specific problems due to case sensitivity or missing path validation.
4. Renaming to maintain consistent naming conventions in public header files can cause backward compatibility issues.
5. Introduction of locks for handling multithreading can lead to deadlocks if not implemented properly.
6. Addition and dependency of 'brew' and 'wabt' in multiple GitHub workflows does not clarify the motive behind these dependencies.
7. Scripts assume the presence of the macOS package manager 'brew' in the system environment, which might not always be the case.
8. Increase in build time due to the complex build process of wabt.

Significant Findings:

1. The addition of exported malloc and free functions provides control for memory allocations to the WebAssembly VM, an essential measure for memory-managed environments like WebAssembly.
2. To avoid data races in a multithreaded setting, unique locks for setMallocFunction and setFreeFunction are introduced, indicating a design consideration for multithreaded applications.
3. Continual work on code readability and maintainability is evidenced by various changes - variable renamings for consistency, file path corrections, and dynamically generating test binaries at test time, etc.
4. The introduction of 'wabt' in the build process across GitHub workflows signifies the importance of this toolkit to the project, potentially being a critical component in the build and testing phases.

Details

Commit 3395677101f50733a8a159d4533663f98576ab27

The pull request titled "C API: Memory manipulation" consists of modifications made across multiple files. Notably, an exported malloc function is added which could be used by the WebAssembly VM to allow memory allocations.

Key changes:

1. Implemented the export of a malloc function in "include/api/wasmedge/wasmedge.h" and "lib/api/wasmedge.cpp" along with a new function for memory allocation in module.
2. Added a NoMallocExport and a NoFreeExport error messages in the "include/common/enum.inc".
3. Introduced MallocFunc (the malloc function instance) and FreeFunc (the free function instance) in ModuleInstance class in "include/runtime/instance/module.h", providing the ability to set and get the malloc and free functions respectively.
4. Included a new test case ExportedMalloc for verifying the exported malloc function in the added APIMallocTest.cpp file.

Commit 1aa355a07295c06e7e57ce34305490c909509210

The proposed changes aim to add a Free function to the existing memory management API to free dynamically allocated memory.

Key Changes:

1. The introduction of a new function WasmEdge_Module_Free in wasmedge.h, which is expected to free dynamically allocated memory using an exported free function.

2. Implementation of the Free method in WasmEdge_Module_Free_Internal in wasmedge.cpp.

3. Relevant signature checking for the Free function and its invocation in the helper function WasmEdge_Module_Free_Internal.

4. In export.cpp, the respective free function is set in the ModInst after checking if this function exists with the expected signature in the module exports.

5. The corresponding test for this new function is added in APIMallocTest.cpp.

Potential Problems:

1. No explicit error handling provided if the WasmEdge_Module_Free function tries to free the memory not previously allocated by malloc.

2. WasmEdge_Module_Free might cause unpredictable behaviours if it's called on the same memory segment more than once or if it's called on a NULL pointer.

3. There is no validation if the Free method correctly frees the memory. The returned status is checked, but it may show successful operation incorrectly if there are bugs in the free function.

4. In the test case, the freed memory (Buffer) is not set to NULL following the WasmEdge_Module_Free call. If any further operations are attempted on Buffer, a dangling pointer issue could occur.

5. The function WasmEdge_Module_Free returns -1 on any failure, which does not provide detailed error information, making it difficult to trace what caused the error.

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 e782810dd73c12e13669d3b631b96dd2573188d6

Changes in this Pull Request:

1. Added unique locks for setMallocFunction and setFreeFunction in ModuleInstance.
   This might be done to avoid potential data race in multithreaded scenario.
2. Renamed variable P_Native_Addr to NativeAddrPtr and base_ptr to BasePtr in WasmEdge_Module_Malloc_Internal to follow the proper coding style.
3. Replaced P_Native_Addr to NativeAddrPtr in WasmEdge_Module_Malloc.
4. Cleaned up and made consistent the naming conventions of the variables in wasmedge.cpp, APIMallocTest.cpp.
   This will make the code more readable and easier to follow.
5. Deleted the binary WASM test file (cmalloc.wasm) and added a process that builds it at test time.
   This makes sure that the wasmtime test always operates on an up-to-date version of the test file, even if the source was modified after the file was built.
6. Added a custom command in CMakeLists.txt to convert cmalloc.wat to cmalloc.wasm
   This allows to make sure that the test file is always up-to-date with respect to the source.

Potential problems:

1. There is no guard to check if the Mutex is already locked before applying the unique lock. This might lead to potential deadlock if not done properly.
2. It appears to be no error handling in case the binary WASM file cannot be built for some reason. No tests would be run, but the failure would be hidden.
3. The deletion of test/api/apiTestData/cmalloc.wasm might affect other tests or scripts not covered in this pull request.
4. It's not clear if the use of mutex in ModuleInstance methods could lead to potential deadlocks. More context or a broader review may be required.
5. The WasmEdge_Module_Malloc_Internal function assumes that the ModInst->findMemoryExports("memory") always succeed while it may not be the case in all scenarios. This could lead to potential null pointer exceptions.

Commit e3b92bf2f1ca90cc72a4d9c842b2630ddc6c62c1

The given patch is primarily a minor code clean-up for a single file: include/api/wasmedge/wasmedge.h. The key changes made by this patch are mostly naming-related, as follows:

1. P_Native_Addr has been renamed to NativeAddrPtr for semantic consistency and better code readability.

2. Two insertions and two deletions have been made to achieve the renaming operation.

The changes pertain to a section of code that deals with the allocation of memory.

Potential Problems:

1. Without a contextual view and baseline testing, it is hard to evaluate if this variable renaming may lead to issues or whether it will impact other dependent sections of the code.

2. As this is a change in the external API (the library's header file), it could have backward compatibility issues if other developers have used this function with the older parameter name.

3. Despite being a change in semantics and seemingly simple, it is paramount to ensure that relevant documentation is updated to reflect these changes. If not, it might lead to confusion for developers using this API.

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 d9339d178df5dcfad1909cfb9e9767511544b7ff

This patch targets the CMakeLists.txt file in the test/api directory. It updates the path to the cmalloc.wat file.

Key changes:

1. The command to convert cmalloc.wat to cmalloc.wasm has been changed. It previously used the source file from ${CMAKE_CURRENT_SOURCE_DIR}/apitestData/cmalloc.wat but now uses ${CMAKE_CURRENT_BINARY_DIR}/apitestData/cmalloc.wat.

2. The DEPENDS line in the add_custom_command build rule has also switched its dependency from ${CMAKE_CURRENT_SOURCE_DIR}/apitestData/cmalloc.wat to ${CMAKE_CURRENT_BINARY_DIR}/apitestData/cmalloc.wat.

Potential Problems:

1. If the cmalloc.wat is not properly copied or moved to ${CMAKE_CURRENT_BINARY_DIR}/apitestData, then this command will fail as it will not be able to find the necessary .wat file to convert to .wasm.

2. If other parts of the project are dependent on the cmalloc.wat file being in its original location in the source directory, those parts may fail.

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 aa6145a684869c1bc07aec5097fa537104cf3a5e

Summary 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:

• The OUTPUT path in the add_custom_command.
• The COMMAND path in the add_custom_command where the WASM file is being created.
• The DEPENDS path in the add_custom_command that specifies the dependency for the output file.
• The path in the add_custom_target where the target depends on the generated cmalloc.wasm file.

Potential Problems:

1. Case Sensitivity: A common problem with file paths is case-sensitivity. On certain file systems (like Linux), filename cases are treated as distinct, while others (like Windows) are case-insensitive. If the project must operate across various operating systems, this could cause confusion.

2. Existing References: The patch doesn't indicate whether any other areas refer to the old apitestData directory. If they do and are not updated, this could lead to errors. Other required changes might also have been missed in this patch.

3. Path validation: This script doesn't include any path validation. This might cause problems if it's run on another machine or if the directory doesn't exist.

Please ensure these potential issues are addressed to ensure your code performs as per the expectations across different environments.

Commit 23626f1114d704f79f6657932277b94241d73eac

The 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:

1. In reusable-build-on-fedora.yml, 'wabt' has been appended to the list of packages to be installed through dnf.
2. In reusable-build-on-macos.yml, the 'wabt' package is being installed through the brew package manager. A check has been added to ensure brew is installed, and if not, an installation is attempted.
3. In reusable-build-on-ubuntu.yml, 'wabt' is installed through the apt package manager. This change is made both for generic build and explicitly for the build with coverage.
4. In reusable-build-on-windows.yml, the 'wabt' package is being added to the list of packages to be installed using Chocolatey.

Potential Problem(s):

• These changes introduce the dependency on 'wabt' without any mention or justification. It is good practice in software engineering to articulate why the dependency is necessary in the commit message.
• For the macOS build, if Homebrew is not installed, the script attempts to install it. While it likely won't be an issue in a controlled CI environment, this could have unexpected consequences outside of it.
• Similarly, if the 'wabt' installation fails on any platform due to unforeseen issues, it may halt the entire build process.
• The difference in 'wabt' version across different platforms could introduce inconsistencies in the build process or the behavior of the compiled software.

Commit 8b1c8855987992d6852c910d30c9a2250106cd10

This pull request changes the build and test workflow scripts for a project. The key changes include:

• Checking if the Homebrew package manager is installed before trying to use it. If it isn't, the script installs it using a one-liner bash and curl command.
• Adding 'wabt' as a new dependency that's installed via Homebrew.
• Setting an environment variable CI=1 to run Homebrew in a non-interactive mode, which is useful for CI/CD environments to prevent interference with user prompts.

There seem to be no major potential issues with the proposed changes. However, it's good to remember that:

• On uncontrolled environment like local dev systems, installing a package manager like Homebrew in case it's missing could slightly be invasive. But it seems appropriate in this context as the scripts appear to be meant for a CI/CD system.
• There aren't any new compatibility issues introduced by 'wabt' without versioning being specified. It could potentially lead to problems if there were older versions of 'wabt' incompatible with the current script.
• The CI=1 environment variable could potentially conflict with other scripts that use the same variable for different purposes. But since these scripts are isolated in GitHub Actions jobs, it's unlikely to cause any issues.

A safer approach could be to print an error and fail if Homebrew is not installed, instead of installing it automatically.

Commit 9ad0887ced684a7788825b5dd7e9eed24f688f06

Summary:

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:

1. Version Specification: The script does not explicitly mention the version of 'wabt' to be installed. Without clear specification, different machines can end up with different versions, potentially leading to inconsistent workflow behavior.

2. Compatibility: Without explicitly knowing the function of 'wabt', one can only assume that it's needed for certain operations within the workflow. If 'wabt' is incompatible with rest of the components, it could lead to failed workflow runs.

3. Test Coverage: There are no tests in this pull request to verify whether the addition of 'wabt' package to each of the workflow actually works as expected and doesn't break the workflows.

4. Dependency Cleanliness: The added package dependency could unnecessarily bloat the system if it's not commonly used by other components within the workflows.

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 0b7c91d41c0534c8ece21ed4707c31d809c05171

Summary of Changes:
The changes made in this pull request are quite straightforward. The author modifies a single line across five different GitHub actions. The line that is modified is a command to install Homebrew, a software package management system, if it doesn't exist already: CI=1 /bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)"

In each case, the change was to prefix this command with sudo (indicating that the command should be run with superuser privileges), resulting in:
sudo CI=1 /bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)"

Potential Problems:

1. Security Risks: Using sudo in the command unnecessarily increases the privilege level which may be a security risk if the code being executed is malicious or contains a bug. An attacker could potentially gain root access to the system.

2. Permissions: While adding sudo might help in local machine testing, adding such a command in a CI script can be problematic. CI servers might restrict sudo usage, and using sudo without a proper configuration can cause the CI to fail.

3. Compatibility: Since sudo is not available on all types of Unix-like operating systems, there may be compatibility issues. If the production environment doesn't support sudo, it might make more sense to fix the permission problem in the environment setup than forcing the use of sudo in the script.

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 b3a24cf6d67c08d490a2e84985a3ca24f91c0414

Summary:
The changes in this pull request are mainly about removing conditional checks for the availability of the 'brew' command in various GitHub Action workflows scripts. 'brew' is the package manager for macOS systems. This change indicates that the author now assumes that 'brew' is always installed on the system where these scripts are being run.

Key Changes:

• The check for the existence of 'brew' command and consequent installation script has been removed from the following GitHub Action workflows: IWYU_scan.yml, bindings-java.yml, build-extensions.yml, release.yml, reusable-build-on-macos.yml, and wasi-testsuite.yml.
• This check was removed from both the 'run' sections and the 'env' sections of these scripts.
• Overall, a total of 33 lines of code have been deleted and 1 line has been added.

Potential Issues:

1. If 'brew' is not pre-installed on the macOS system where these scripts are run, these workflows will fail because the ‘brew install’ command won’t be recognized. We can't always assume that 'brew' is present.
2. The removed lines also included the part where the CI environment variable was set for running Homebrew in non-interactive mode. This could potentially cause interactivity 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 a5c000068c2c97b439d70672ff41151f815d7c83

This patch introduces the WebAssembly Binary Toolkit (wabt) into the Windows build process. It notably features the following key changes:

1. The overall structure of dependencies installed on Windows is refined by separating the installation of wabt from other packages like cmake, ninja, and vswhere.
2. A new step is added to install the wabt toolkit. The new sequence of actions does the following:
   a. Installs visualstudio2019buildtools package component for supporting Visual C++ x86 and x64 tools.
   b. Clones the wabt repository using git clone.
   c. Enters the wabt directory, creates a build directory, and navigates into it.
   d. Runs cmake with arguments setting up a RELEASE build type and a Visual Studio 2019 generator.
   e. Finally, it invokes cmake to build and install wabt.

Potential issues relating to this patch could include:

1. Failure in any of the installation steps could halt the entire workflow. Such failures could arise from network issues, changes to package parameters, or changes in wabt repository structure.
2. Given the complexity of the build process, debugging errors might prove difficult if they originate from within the wabt build.
3. The installation and setup of wabt take a considerable amount of time, increasing the overall build duration.
4. There is no cleanup step after the build. This could impact subsequent builds that are based on the current state of the build machine.
5. Lastly, there might be compatibility issues, because the patch uses a specific version of visualstudio2019buildtools. It might fail if this version is not available, or there might be issues when inter-operating with other versions.

Commit 93487ab0e06cd32efc12499a908b3394bd464768

The 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 (-DBUILD_TESTS=OFF) has been added to the build command. With this flag, the build procedure should not include the compilation of test cases.

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 fc84184f0e712124389b2ac16eaaad7b6804886b

This patch makes two key changes:

1. In the MacOS workflow file, the patch adds a new line eval $(/opt/homebrew/bin/brew shellenv). This line is used to setup Homebrew shell environment, which could be necessary for some parts of the build process to use Homebrew installations correctly.

2. In the Windows workflow file, the patch adds the line git submodule update --init within a section dealing with the cloning and building of a wabt repository. This new line is added to ensure that any submodules for the 'wabt' repository are initialized and updated, which is important if the 'wabt' repository uses git submodules, otherwise, it might not build correctly.

Potential Problems:

1. For the MacOS workflow, if the Homebrew installation directory is not '/opt/homebrew/' (i.e., it has been installed in a non-standard location) or is not installed at all, the added line may cause an error.

2. For the Windows workflow, if the 'wabt' repository does not contain any git submodules, the added line git submodule update --init will be redundant. However, if it does contain submodules and those submodules require specific versions of software or specific configuration, these are not being handled by this patch.

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 e3b337b53044efe2cc490d556a2b9272615269e5

Summary of changes:

The developer has modified the GitHub Actions workflow file reusable-build-on-windows.yml. The primary change is the addition of a line that appends the bin subdirectory of the current location to the PATH environment variable in a Windows environment. This modification is done in the section of the workflow that performs a cmake build.

Potential problems:

1. Hardcoding paths: While this change is generally safe, hardcoding paths can sometimes pose issues. If the bin directory doesn't exist at that location or the user has non-standard configurations, this script could fail. It may be better to use a more dynamic means to get the necessary paths or ensure they exist before attempting to add them to the PATH.

2. Platform compatibility: This change assumes that the workflow is always being run on a Windows platform. If the workflow has to support multiple platforms (e.g., Linux, MacOS), this line of code might fail on platforms that do not use the same syntax to set environment variables or to refer to file paths.

3. Debugging path issues: PATH is a critical environment variable, and manipulating it may cause confusion or errors down the line if there are problems and if errors are not explicitly handled here. For instance, if an error happens later in the script, it might be difficult to troubleshoot whether it was caused by a faulty PATH.

4. Redundancy: This new line is executed regardless of whether the cmake commands above it succeed or not. If the cmake commands fail, adding the bin directory to PATH might still happen, which could be unnecessary or could even cause issues if any builds were partially successful.

5. Missing error handling: The line doesn't handle any possible failures. It would be a good practice to check the return status of the command and handle any error situations, rather than assuming that it will always execute successfully.

6. Clarity and documentation: The purpose of this change is not immediately clear from the context. It would be beneficial to include a brief comment explaining why this step is necessary. This could save time for future developers who have to understand or debug the script.

Commit 317716472fb5b56867e88ebb145c5bd8101eb4c4

The pull request titled "C API: Memory manipulation" makes three changes in total across three files:

1. A change in .github/workflows/reusable-build-on-macos.yml where the shell environment of the /opt/homebrew/bin/brew is evaluated. This means Homebrew's shell environment will be used during the build, ensuring that any dependencies managed by Homebrew are correctly set up in the build environment.

2. A change in .github/workflows/reusable-build-on-windows.yml where a PATH environment variable addition has been removed and another one has been added. The new addition appends the wabt build bin directory to the PATH, allowing executables in that directory to be accessed more easily.

3. A space has been added in test/api/CMakeLists.txt, it does not affect the functionality.

Potential issues:
Firstly, the brew shellenv command in the macOS workflow might cause a problem if the Homebrew setup is not correct or if the command is not recognized due to OS version or setup differences.

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 (Get-Location).Path)/bin that's now no longer included in PATH.

Further reviewing the software requires running tests to ensure removal/addition of environment paths doesn’t affect other parts relying on them.

[aghinsa]

will an exported function like this work? Won't the pointer be converted to i32 in wasm?

[q82419]

What does this function mean?

[aghinsa]

Does this function satisfy the current requirements?

[q82419]

There's not only the malloc, but new, etc. for the malloc functions.
You should follow the detection here to search the exported functions.

[q82419]

You can follow the folders in test to construct your tests.

[q82419]

Please use cmake for building.

[q82419]

You can follow the folders in test to construct your tests.

[q82419]

There's not only the malloc, but new, etc. for the malloc functions.
You should follow the detection here to search the exported functions.

[q82419]

What does this function mean?

[aghinsa]

@q82419 Made changes according to previous comments. Will add new,free etc if current approach is ok.

[q82419]

Hi @aghinsa ,

Hope you can make these changes first:

1. Apply the DCO (sign-off your commits).
2. Lint your code changes with the clang-format.

Thanks!

[codecov]

Codecov Report

Merging #2620 (2d1c2a0) into master (88d4922) will decrease coverage by 0.02%.
Report is 1 commits behind head on master.
The diff coverage is 72.72%.

@@            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     

Files Changed	Coverage Δ
lib/api/wasmedge.cpp	92.21% <56.52%> (-1.14%)	⬇️
lib/executor/instantiate/export.cpp	88.88% <93.33%> (+3.17%)	⬆️
include/runtime/instance/module.h	90.65% <100.00%> (+0.65%)	⬆️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[aghinsa]

Hi @aghinsa ,

Hope you can make these changes first:

1. Apply the DCO (sign-off your commits).
2. Lint your code changes with the clang-format.

Thanks!

Done

[aghinsa]

@q82419 should I proceed with the current impl?

[hydai]

@q82419 should I proceed with the current impl?

Hi @aghinsa
Yes. That's the way that is described in the issue. Please go ahead, feel free to let us know if you are finished.

[aghinsa]

@q82419 should I proceed with the current impl?

Hi @aghinsa Yes. That's the way that is described in the issue. Please go ahead, feel free to let us know if you are finished.

@hydai Done. Have not added check for 'new' as a malloc function as the api is for c. Please do let me know your comments.

[hydai]

@hydai Done. Have not added check for 'new' as a malloc function as the api is for c. Please do let me know your comments.

Could you please check if the new / delete will generate different signatures? If not, then we can just check the malloc and free only.

[aghinsa]

@hydai Done. Have not added check for 'new' as a malloc function as the api is for c. Please do let me know your comments.

Could you please check if the new / delete will generate different signatures? If not, then we can just check the malloc and free only.

@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?

[hydai]

@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 Have not added check for 'new' as a malloc function as the api is for c. That's why I am asking what's the signature of new.

I think a simple way is to create a C++ example program with new and delete, then compile it with emcc to see what's the generated wasm code to figure it out.

[aghinsa]

#2463

@hydai
Sorry for the confusion. Let me try to clarify.
Initially I didn't add check for new/delete as they were cpp operators. Later I tried exporting 'new' and 'delete', using
emcc to_export.c -s EXPORTED_FUNCTIONS=["_new,_delete"] -o exported.wasm, this didn't work as new and delete are operators and not functions.

Per your suggetion i tried compiling a function which uses new and delete.

// cpp_export.cpp
#include <new>

using namespace std;

extern "C" void testing_new() {
  int *p = new int;
  *p = 9;
  delete p;
}

int main(int argc, char *argv[])
{
  testing_new();
  return 0;
}

emcc cpp_export.cpp -s EXPORTED_FUNCTIONS=["_testing_new"] -o exported.wasm

In the wasm code for testing_new, functions with same signature as malloc and free are called, which makes sense.

Now my doubt is how would a user export the new and delete operators (Since we are planning to add check for new and delete). The only way i could think of is to export something like.

extern "C" void* int_new() {
  int *p = new int;
  return p;
}

[hydai]

IMO, we should disregard the new/delete operators at the C SDK level. This is primarily because these operators will inherently be generated alongside malloc/free. Furthermore, developers choosing to call malloc/free at the WebAssembly level generally have a clear understanding of their intentions. Therefore, it isn't necessary for us to export new/delete functions.

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 malloc/free behavior, they are equipped to manage it independently on their side.

[aghinsa]

IMO, we should disregard the new/delete operators at the C SDK level. This is primarily because these operators will inherently be generated alongside malloc/free. Furthermore, developers choosing to call malloc/free at the WebAssembly level generally have a clear understanding of their intentions. Therefore, it isn't necessary for us to export new/delete functions.

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 malloc/free behavior, they are equipped to manage it independently on their side.

Yes. I agree.
Do you require anything further from my side for a merge?

[hydai]

Please rebase with the latest master commits. Thanks.

[aghinsa]

Please rebase with the latest master commits. Thanks.

Done.

[hydai]

Please fix the related CI failure.