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

[Plugin] zlib [LFX 2023 Term 2] #2562

Merged
merged 122 commits into from
Sep 15, 2023
Merged

[Plugin] zlib [LFX 2023 Term 2] #2562

merged 122 commits into from
Sep 15, 2023

Conversation

NeelDigonto
Copy link
Contributor

@NeelDigonto NeelDigonto commented Jun 5, 2023

#2546

Project Overview: #2244 (comment)
Improved version of PR #2395

CMake

  • CMakeLists.txt
    • option(WASMEDGE_PLUGIN_ZLIB "Enable WasmEdge zlib plugin." OFF)
  • plugins/CMakeLists.txt
    • if(WASMEDGE_PLUGIN_ZLIB) add_subdirectory(wasmedge_zlib) endif()
  • plugins/wasmedge_zlib/CMakeLists.txt
    • Add zlib-ng through FetchContent
    • Add sources & build library

Workflows

  • .github/workflows/build-extensions.yml
    • build_ubuntu
    • build_manylinux
  • .github/workflows/release.yml

Documentation

  • docs/book/en/src/contribute/build_from_src/plugin_wasmedge_zlib.md

Tests

  • Basic Function presence check.
  • Full Deflate Inflate Cycle.

Implementation

  • 74 Functions (ZStream + GZFile) Covered
  • deflateSetHeader | inflateGetHeader (This PR)
  • zlibVersion | gzerror | zError | get_crc_table (New PR)
  • gzprintf | gzvprintf (New PR)
  • inflateBack (New PR)

Ready for Review

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Copy link
Member

juntao commented Jun 5, 2023

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


The pull request "[Plugin] zlib [LFX 2023 Term 2]" mainly focuses on adding zlib plugin and its functionalities - a data compression library to the WasmEdge project. Various patches are included taking care of multiple aspects like adding the plugin to CMake build, configuring GitHub Actions workflows, adding environment for the zlib plugin, implementation of different zlib functions and adding zlib module implementation, however, several potential issues need to be addressed:

  1. The patches assume that the zlib library is installed on the building system and the wasmedge_zlib directory exists and contains valid source code; this could lead to build process failures.
  2. The CI/CD workflow patches don't consider potential build failures on missing or non-executable compilers, and multiple core availability is assumed without explicit verification. There is a lack of tests, and identical output paths for different jobs may lead to confusion or conflicts.
  3. In code level changes, the thread safety and error handling conventions are not clear or missing, leaving potential risks for multidata corruption, failed allocations, and other unexpected runtime behavior.
  4. Another significant concern is that all patches add or modify code without corresponding unit tests, potentially leading to undiscovered bugs in the new functionality.

The introduction of a new library dependency, zlib-ng, could have a project-wide impact, raising further potential issues:

  1. If zlib-ng fetching fails or if an updated version is not manually handled, it may lead to build and runtime failures.
  2. If specific features of zlib-ng are used, having ZLIB_COMPAT set to ON might cause a failure.
  3. ZLIBNG_ENABLE_TESTS is set to OFF, which might lead to potential undetected functionality issues.

In conclusion, the patches bring significant additions and improvements to the project but also introduce potential risks and problems that should be addressed to ensure robustness and reliability of the solution. Regardless of apparent syntax correctness, it's crucial to add unit tests, handle exceptions meticulously, and manage memory and resources effectively to guarantee reliable and efficient operation in a multi-threaded environment. Furthermore, comprehensive documentation and meaningful descriptions would help understand the changes better and maintain code quality over iterations.

Details

Commit fcc2423258cf20e9b8d0472ee6907be69fc5ee59

This update is to the plugins/CMakeLists.txt file, which seems to handle the build process for the plugins of a certain application. The key change is the addition of a 4-line code snippet to include the wasmedge_zlib in the CMake build process.

If the flag WASMEDGE_PLUGIN_ZLIB is set, then the wasmedge_zlib directory will be added as a subdirectory for the build process, assumingly to build a zlib plugin.

Given that this is a short patch, there are no overt problems. However, there are implicit assumptions that we can discuss:

  1. wasmedge_zlib directory exists and contains valid source code. If the directory doesn't exist or doesn't contain a proper CMake setup, then the build process might fail.

  2. The flag WASMEDGE_PLUGIN_ZLIB is expected to be defined somewhere. If it's not set perhaps no zlib plugin is built. If it should always be built, this might be a problem.

  3. This patch assumes that the zlib library is installed on the building system in case that library is needed while compiling wasmedge_zlib. If zlib is missing, this might lead to compilation errors.

Lastly, this is patch 1/98, it is possible that following patches may depend on this one. If they are applied pre-emptively, it might create issues.

Commit 2d866db038b8dd958bf723e421443ac868b19625

Summary of Changes:
The author, Saikat Dey, has added an enable/disable option for a new WasmEdge plugin named 'zlib' in the 'CMakeLists.txt' file. This is done using the 'option()' function in CMake, which allows users to specify if they want to build this new plugin while configuring the build process.

Potential Problems:
This update, by itself, does not seem to have any glaring issues. However, it's worth noting that a few potential problems might come up down the line, especially if the implementation of the plugin hasn't been done carefully:

  1. If the zlib plugin code isn't ready or stable, enabling it might cause the entire build to fail. This could be mitigated by keeping the plugin disabled by default (as it's now) until its code is ready and stable.

  2. If there are unmet dependencies for zlib, the build might fail or throw warnings, requiring the users to first install those dependencies before they can successfully build the software.

  3. Compatibility issues might arise if the new plugin 'zlib' isn't compatible with other components, plugins, or architectures of the system.

It's crucial to test the software thoroughly after adding a new plugin, ensuring that it works as intended and doesn't interfere with the functioning of other components.

Commit b4d0cc4630cc77d0247fe4a932e716126fde654b

The patch is introducing a new dependency to the project - zlib-ng, a modern, optimized version of zlib. This is a major change, depending on the size of the project. The changes are made in the CMakeLists.txt file of the plugin wasmedge_zlib, meaning it's specifically for building the zipped up version of wasmedge (which seems to be a software development kit). Key changes:

  • An Apache-2.0 license is set for the software.
  • System zlib is no longer being relied on.
  • ZLIB_COMPAT is set to ON.
  • Tests for zlib-ng are switched off (ZLIBNG_ENABLE_TESTS is set to OFF).
  • zlib-ng is fetched from its GIT repository (https://github.com/zlib-ng/zlib-ng.git) at tag 2.0.7.

Potential issues:

  1. Dependency Issue: If zlib-ng fails to fetch, the build may fail. While not a coding problem, it entails risk. Proper exception handling needs to be ensured.
  2. Versioning Issue: The patch is fetching a fixed version of zlib-ng (2.0.7). If there are critical updates in zlib-ng later on, the current code might not benefit from those unless manually updated.
  3. Compatibility Issue: Since ZLIB_COMPAT is set to ON, the code is intended to work with the zlib compatible interface. If some features specific to zlib-ng are used in future, they can fail.
  4. Test Deactivation: This patch deactivates the tests for the zlib-ng (ZLIBNG_ENABLE_TESTS is set to OFF). This could cause potential undetected issues regarding zlib-ng's functionality.
  5. Deactivation of System zlib: Dependency on system zlib is removed which implies all utilities are expected to be provided by zlib-ng. If some features were specific to system zlib, it might cause issues.

Finally, if this is a large project, introducing a new dependency like this could have a significant impact, so it should be suitably reviewed and discussed by the team.

Commit da6a3023eaeebcf7a4e21d52b6c2eb7c756a9149

The pull request titled "[Plugin] zlib [LFX 2023 Term 2]" primarily focuses on adding a new build target for the zlib plugin for the 'build_ubuntu' job in the GitHub Actions workflow, as mentioned in 'build-extensions.yml'.

Key Changes:

  1. The new build target 'Zlib' is added to the matrix under the job 'build_ubuntu'. Both g++ and clang++ compilers are expected to be used for this build, and the build output is expected to be in the Release mode.

  2. The target details include the tar_name, compiler, docker_tag for the Ubuntu build, output_path for the binaries, output_bin for the built binary, test_path for the test binaries, and test_bin for the test binary.

  3. Two new job steps have been added for building and testing the WasmEdge Zlib plugin. There's an 'if' condition for the build step that checks if the matrix name contains 'Zlib'.

Potential Problems:

  1. The zlib build and test steps are directly depending on the 'Zlib' matrix condition. If this plugin is absent in the matrix, the steps will be skipped.

  2. If the build or test steps encounter any issues, the detailed error messages or debug information messages might not be available because they seem to be not set up.

  3. The script is set to compile the plugin with both g++ and clang++, but it doesn't verify if these compilers exist or are executable on the system before running the build. This might cause build errors on systems that don't have these properly installed.

  4. Code duplication: The same 'Zlib' build target is added twice with the same binary output path and test binary but different compilers. If future changes are expected in these settings, they have to be done in both places which could lead to human errors.

  5. We're assuming that the build machine has multiple cores with the make -j flag, which might not be always true and could possibly break the build.

Commit cc0dd7cbe8c1ec08c05f259254522b66d501b53c

Summary of Main Changes:
This GitHub pull request entails an author named Saikat Dey to add zlib build target for the job 'build_manylinux' in the 'build-extensions.yml' configuration file towards the WasmEdge plugin that exists in the repo. This code is part of a Continuous Integration/Continuous Deployment (CI/CD) workflow.

The patch adds two build jobs into the YAML CI script with system platforms, manylinux2014_x86_64 (64-bit x86 architecture) and manylinux2014_aarch64 (64-bit ARM architecture). This inclusion is meant to create a build for the zlib plugin of WasmEdge on those platforms, creating a shared library output named 'libwasmedgePluginWasmEdgeZlib.so'. The cmake option '-DWASMEDGE_PLUGIN_ZLIB=ON' seems to indicate that the zlib plugin will be included at compile-time.

Potential Problems:

  1. Reuse of the Same Job Name: The same job name "Zlib" is used for both - manylinux2014_x86_64 and manylinux2014_aarch64, which may lead to confusion when debugging failures in CI/CD process.
  2. Lack of Tests: Each of the job configurations has 'test: false' which suggests that no tests are being run after the build. This might make it difficult to detect if the addition of the plugin is causing any errors or unexpected behavior.
  3. Platform Support: The code seems to introduce the build of the plugin on Linux platforms only (manylinux2014 for AMD 64-bit and ARM 64-bit) with no evidence of support for other platforms.
  4. Build Failures: Build workflow will fail if the zlib plugin is not available or not compatible with the specified systems and architectures.
  5. Identical Output Paths: Both jobs output to 'plugins/wasmedge_zlib'. It's assumed that this will not cause any conflicts, but it may be less confusing to have different paths.
  6. Documentation: There's no update or addition to documentation or comments to help understand why this change has been made or how it is to be used.

Overall, these additions should be validated to confirm that they don't negatively impact the project's build processes.

Commit 618c2e84549d2444568f7beac0101b67e79da568

The invitee, Saikat Dey, has introduced an initial environment for a new zlib library plugin for the WasmEdge environment, as shown by 3 new files with 100 lines of insertions. The zlib library is usually used for data compression.

In the zlibbase.h file, a new template class called WasmEdgeZlib has been created which inherits from Runtime::HostFunction<T>. An instance of WasmEdgeZlibEnvironment is protected within it.

The zlibenv.h file defines the Wasm_z_stream struct which is a simple mapping to a wasm 32bit z_stream object along with a new class WasmEdgeZlibEnvironment that contains a std::unordered_map named ZStreamMap mapping uint32_t to unique pointers of z_stream objects.

The zlibmodule.h file introduces a class WasmEdgeZlibModule that is a derived class of Runtime::Instance::ModuleInstance. It includes a method for retrieving an instance of WasmEdgeZlibEnvironment.

There are, however, a few potential problems that may arise:

  1. Error handling: There is no error checking of, or exception handling for, potential failures. For example, what happens if initializing the zlib environment fails or if the allocation for unique pointers of z_stream objects fails.

  2. Thread-Safety: The current design does not seem to take multi-threading scenarios into account. Particularly, the unordered_map ZStreamMap within WasmEdgeZlibEnvironment is not protected against concurrent access, which could lead to race conditions in a multi-threaded environment.

  3. Resource cleaning: unique_ptr is used to track and release z_stream objects automatically. However, we don't see any code for handling zlib's way of managing resources, which usually includes a deflateEnd or inflateEnd call to free any internal decompression state in z_stream objects. This might cause resource leaks if not handled correctly.

  4. Code review on zlib integration: The use of the zlib library is currently unknown. This raises concerns of potential inconsistencies or errors when integrating the zlib library. It would be necessary to see the consequent patches where zlib is actually used.

  5. Test cases: There are no associated test case changes in this patch, so it's not clear how thoroughly the new code has been tested. It would be necessary to add related tests to ensure the proper function of these classes and structures.

Commit c167c297a01082cb978b610b39687a4de8dbb6c1

This patch introduces a new header file (zlibfunc.h) in the wasmedge_zlib directory of the project. The primary change is the creation of several classes, each of which represents a different function provided by the zlib library, a software library used for data compression. The classes are:

  1. WasmEdgeZlibDeflateInit_: handles the zlib deflate initialization process.
  2. WasmEdgeZlibInflateInit_: handles the zlib inflate initialization process.
  3. WasmEdgeZlibDeflate: implements the zlib deflate process.
  4. WasmEdgeZlibInflate: implements the zlib inflate process.
  5. WasmEdgeZlibDeflateEnd: handles the zlib deflate end process.
  6. WasmEdgeZlibInflateEnd: handles the zlib inflate end process.

All the classes are inheriting from a generic template class WasmEdgeZlib and are implementing a method called body.

Main findings:

  1. Given this is a new file, there should not be any breaking changes, assuming the new functionality does not interfere with existing behavior of the application.

Potential issues to consider:

  1. While these changes don't immediately produce any glaring issues, it's important to ensure each class has been correctly implemented. It is hard to determine this with the information provided.

  2. One potential problem could arise depending on how threading is used in WasmEdge, as zlib is not thread-safe by default. If concurrent execution occurs within these functions, it could result in unexpected difficulties or data corruption.

  3. The patch does not show any unit tests accompanying these new classes. Generally, it is a good practice to include unit tests for any new code to verify its correctness, and this helps to catch potential bugs or issues early.

  4. Also, the error handling mechanism is not clear from this patch. Ideally, any errors thrown during the execution of zlib functions should be appropriately caught and handled.

In conclusion, while there seem to be no immediate issues with the patch, a review of the full implementation and corresponding tests is necessary for a complete understanding.

Commit 9b9e14de7ddccec003de1fc96937b01527fae9ce

The change presented in this patch involves the addition of a new file named zlibenv.cpp to the wasmedge_zlib plugin in a codebase presumably related to the WasmEdge runtime. The new file contains the implementation of a plugin descriptor and an environment for Zlib, a compression library.

Here are the key changes:

  • A new file named zlibenv.cpp is added.
  • Within the zlibenv.cpp file, a new module instance WasmEdgeZlibModule is created in the function create.
  • A struct Descriptor of the type Plugin::Plugin::PluginDescriptor is defined, which describes the plugin's name, API version, version numbers, the number of modules, and a pointer to the module details.
  • A plugin registration function Register is called with the Descriptor.

Potential issues:

  1. Descriptions for the plugin and the module are empty. It is good practice to add meaningful descriptions to help others understand what these components are for.
  2. There is no error handling or null-checking when new WasmEdgeZlibModule is called. If the allocation fails, the program may experience undefined behavior.
  3. As the Create function is defined within an unnamed namespace, it is only visible within this file. If it is required in other parts of the plugin, it would be inaccessible.
  4. Memory management isn't well defined. There's a potential for memory leaks because new is used to allocate the WasmEdgeZlibModule, but it is not clear who is responsible for delete to free the memory.
  5. Full testing or documentation is not part of this request, it would be necessary to ensure that the added code is working as expected.

Commit fcaa19904597c16d793e680329194b8211b5ba99

This pull request, by Saikat Dey, is focused on adding a new zlib module implementation to the plugin 'wasmedge_zlib'. A new file named 'zlibmodule.cpp' is created and 27 lines of code are added in it. Here are the key changes:

  • A namespace corresponding to 'WasmEdge' and within it, another namespace 'Host' is defined.
  • The 'WasmEdgeZlibModule' is initialized with the name 'wasmedge_zlib'.
  • The following six host functions are being added in the 'WasmEdgeZlibModule':
  1. wasmedge_zlib_deflateInit_
  2. wasmedge_zlib_inflateInit_
  3. wasmedge_zlib_deflate
  4. wasmedge_zlib_inflate
  5. wasmedge_zlib_deflateEnd
  6. wasmedge_zlib_inflateEnd

Each of these functions are initialized with a 'WasmEdge XYZ' function where XYZ corresponds to the respective function name.

Given the information in the provided patch commit, there doesn't appear to be any critical issues in terms of syntax or missing mandatory parts. However, there are a couple of points that need to be considered for revealing potential problems:

  • It's not clear if the file 'zlibfunc.h', which is included in 'zlibmodule.cpp', is available and correctly implemented.
  • Moreover, it appears that these functions are created using the 'std::make_unique' function, but there is no clarification regarding the contents of these 'WasmEdge XYZ' functions. They must be set up correctly, contain no errors, and must be compatible with the rest of the system.
  • The 'Env' object which is passed into the 'std::make_unique' function is not visible in this patch, it should be ensured that it is defined and valid.
  • Lastly, unit tests for this new functionality are not included in this patch, which makes it impossible to automatically validate these changes.

Commit bf6adcdb00a2859bcedddcfa1c82e6a4ae961bfb

The developer, Saikat Dey, has added a file containing an implementation for a plugin, named WasmEdgeZlibDeflateInit_, in the WasmEdge project. The implementation is in a file, zlibfunc.cpp, inside the directory plugins/wasmedge_zlib.

Key Changes:

  1. A new file "zlibfunc.cpp" was created in the "plugins/wasmedge_zlib" directory.
  2. It declares the function/procedure for WasmEdgeZlibDeflateInit_ using C++. The function has a return type of "Expect<int32_t>" and takes as parameters a calling frame &Frame, a 32-bit unsigned integer ZStreamPtr, a 32-bit integer Level, and 32-bit unsigned integers VersionPtr and StreamSize.
  3. A memory instance "MemInst" is acquired from the passed "Frame" and is checked for nullity. If it's found null, the host function error is returned.
  4. Checks if the stream size is non-standard, if it's not equal to 56 it returns HostFuncError.
  5. Fetches the WasmZlibVersion and checks it against the Host's Major Zlib version. If it doesn't match, it triggers an error and returns HostFuncError.
  6. Initializes a new z_stream object (HostZStream) with values of Z_NULL for zalloc, zfree and opaque fields.
  7. deflateInit_ function is invoked with parameters HostZStream, Level, ZLIB_VERSION, and size of z_stream; the result is stored in z_res.
  8. The constructed HostZStream object is inserted in the Env.ZStreamMap by making a pair with the key as ZStreamPtr.
  9. Returns integer value of z_res as output.

Potential Problems:

  1. No nullity check done when getting the WasmZlibVersion via the getPointer function. This may cause a null pointer dereferencing error.
  2. There is no check for deflateInit_ errors, as it could also return an error code.
  3. No error handling if MemInst->getPointer fails for any reason.
  4. Lastly, ZStreamPtr is used to index the ZStreamMap, if this is user-supplied data, there may be a risk of collision or overwriting data.

Commit 9615d7b6a44ea11cea9986e913fc5a77b3fb3350

The pull request involves adding a new implementation for the plugin "zlib". This implementation pertains to the zlib inflation initialization. Below are the key changes:

  1. A new function WasmEdgeZlibInflateInit_::body has been defined. This function initializes the inflation process with zlib.
  2. The function checks if the memory instance "MemInst" is null and if the stream size is not equal to 56.
  3. Then, it retrieves the zlib version from memory.
  4. The function checks if the zlib version on the host matches the first character of the version in the Web Assembly (Wasm).
  5. Then, it initializes a z_stream object with Z_NULL for the zalloc, zfree, and opaque members.
  6. The inflateInit_ function is called with the z_stream object, the zlib version, and the size of z_stream.
  7. Finally, the z_stream object is added to the ZStreamMap in the environment with ZStreamPtr as the key.

Potential problems:

  1. Error handling: It would be a good idea to consider more robust error handling. The current implementation only checks if the memory instance is null or if the stream size differs from 56. There might be other cases where exceptions or errors might occur.

  2. Magic Number: The use of magic numbers (like 56) can make maintenance more difficult in the future as they don't inherently describe what they represent. It would be better to create a constant that describes what the 56 stands for in this context.

  3. Version Checking: The implementation only checks the first character of the zlib version for matching with 'ZLIB_VERSION[0]`. If there is a need for precise version checking then this approach may not be sufficient.

  4. Memory Leak: The unique pointer HostZStream used in initializing the inflation is moved into Env.ZStreamMap. If there's no associated cleanup of the memory stored in the map, this can lead to a memory leak.

  5. Thread-safety: If this code is accessed by multiple threads, using std::map::emplace to add mapped elements to 'Env.ZStreamMap' might lead to race conditions. It's not specified if concurrency is an issue in this context, but it's worth noting.

Commit 73e1dcb76f079805fe9e5c38170d94cdbadccb0e

Summary of Changes:

The developer Saikat Dey has made changes in the file plugins/wasmedge_zlib/zlibfunc.cpp. A new method called WasmEdgeZlibDeflate::body has been added to the WasmEdgeZlibDeflate class.

In the added function, the logic includes looking up a ZStreamPtr in the ZStreamMap. If it's not found, an exception is returned.
Otherwise, it retrieves the Pointer to a Wasm_z_stream struct from memory and retrieves memory from the start. Using this memory, it updates the next_in and next_out pointers and avail_in and avail_out values on the host stream HostZStream by accessing the similar fields from WasmZStream. Next, it calls the deflate function with HostZStream and Flush as arguments. After the deflate call, it updates the WasmZStream struct with the new values of avail_in, avail_out, next_in, and next_out from HostZStream. The function finally returns the result of deflate call that was typecast to the int32_t type.

Potential issues:

  1. Error handling: The added function handles the situation where ZStreamPtr is not found in the ZStreamMap by returning an unexpected host function error. However, it doesn't seem to handle any potential failures or exceptions that might occur during the deflate call.

  2. Memory management: There is no explicit memory deallocation code present. If the HostZStream object or the WasmMemStart are populating large amounts of memory, there could be a potential memory leak.

  3. Double naming: WasmEdgeZlibDeflate has been mentioned twice in the function signature. This might cause confusion or it might even be a typo.

  4. Testing: Without seeing any associated tests, we can't confirm whether the newly implemented function operates as expected.

  5. Code clarity: The purpose of certain operations, like WasmMemStart + WasmZStream->next_in/out, could be made clearer with added comments.

Commit 0e61ff487c0cd25a3ea59dde3fdb40d0843803e9

The key changes in this pull request are the implementation of a zlib inflate functionality within the existing WasmEdgeZlibInflate class in the zlibfunc.cpp file.

Summary of changes:

  1. Addition of the method body within class WasmEdgeZlibInflate. This method accepts parameters CallingFrame &Frame, uint32_t ZStreamPtr, and int32_t Flush.
  2. Within the method body, first, it verifies that ZStream exists in the ZStreamMap. If not, it returns with HostFuncError.
  3. Then it fetches a pointer to the ZStream within the memory setting up the available space and the pointers to point towards the memory locations for the input and the output.
  4. Afterwards, it calls the inflate function with the prepared ZStream and the flush method passed in as arguments.
  5. Once the inflate is complete, the method updates the WasmZStream values and returns the result.

Potential problems:

  1. Error handling: The function seems to return just Unexpect(ErrCode::Value::HostFuncError) if the provided ZStreamPtr is not found, which might give very non-specific error information to users.
  2. Missing validation: There is no apparent error handling for possible issues with other functions used, such as memory allocation failures (MemInst->getPointer) or inflate errors.
  3. Unclear memory management: It's unclear how memory is handled here. There might be memory leaks if memory isn't freed properly.
  4. Multithreading problems: If this code is executed in a multithreaded context without proper synchronization, it could lead to race conditions due to the shared use of Env.ZStreamMap.
  5. The code does not check if the Frame has memory associated with it. An invalid or absent memory could cause errors.
  6. If the data's size exceeds the available space in WasmMemStart (or the range calculated by next_in and next_out), it might cause buffer overflow issues.

Commit 9f692f00b9b7ef50acf6b7562dd6629745048210

The pull request titled "[Plugin] zlib [LFX 2023 Term 2]" contains changes primarily in the file zlibfunc.cpp inside the directory plugins/wasmedge_zlib/.

Summary of Key Changes:

  • 19 lines of code have been added to the existing source code.
  • An implementation for the function WasmEdgeZlibDeflateEnd::body() has been added. This function promises to return an int32_t result. This function appears to be vital for the disassembly of compressed data with zlib's deflate algorithm. First, it retrieves the ZStreamPtr from the ZStreamMap in Env. If it is not found, it returns an ErrCode::Value::HostFuncError.
  • It further calls deflateEnd(HostZStream) function using the HostZStream pointer, stores the result in ZRes, and erases ZStreamPtr from ZStreamMap.
  • Finally, it returns the result of the deflateEnd(HostZStream) call.

Potential Problems:

  • There is no explicit error-checking for the deflateEnd function, which might fail for several reasons. This may subsequently return misleading or incorrect errors.
  • Erasing the ZStreamPtr from the ZStreamMap prior to checking for a successful deflate operation could lead to problematic behavior if the deflate operation fails.
  • Commented-out code/todos indicate that this pull request may not be complete or fully functional yet. The code suggests that message synchronization in inflateEnd() and deflateEnd() methods needs to be implemented, but it's not done here.
  • Addition of tests to validate this new implementation would be helpful (though not visible in this patch). It's important to make sure that the new code behaves as expected in all scenarios, including edge cases. Absence of such tests might lead to potential unidentified bugs in production.

Commit face6ab6df477905441777c8c375542b0465b793

This patch represents the implementation of the WasmEdgeZlibInflateEnd function within the WasmEdge zlib plugin.

Key Changes:

  1. A function named WasmEdgeZlibInflateEnd::body has been added with parameters const Runtime::CallingFrame & and uint32_t ZStreamPtr.
  2. Within the WasmEdgeZlibInflateEnd::body function, the code is finding ZStreamPtr from Env.ZStreamMap. If ZStreamPtr is not found, it's returning an Unexpected error with HostFuncError code.
  3. If ZStreamPtr is found in Env.ZStreamMap, it invokes the inflateEnd function with the pointer found in the map.
  4. Then it proceeds to remove ZStreamPtr from the Env.ZStreamMap.
  5. At the end, it returns the result of the inflateEnd function as a 32 bit integer.

Potential Issues:

  1. An error handling mechanism when the inflateEnd function returns an error is not in place. For instance, inflateEnd may return an error message when freeing memory isn't successful but the current implementation won't surface this, leading to potential silent failures.
  2. There is no verification if ZStreamPtr is valid before using it. An invalid pointer can cause undefined behavior leading to crashes or bugs.
  3. The ZStreamMap.erase(ZStreamPtr) operation isn't protected with error checks or exceptions. If this erasure operation fails for some reason, the code does not handle such a scenario. It might cause memory leaks if the delete operation is not successful, the pointer will not be deleted but it will be removed from the map.

Commit a0cc2d24acd2f2c27ba6f939fd6c9d2748edbdc9

This patch populates the 'CMakeLists.txt' file to help build and link the zlib plugin source.

Key Changes:

  1. A new library called 'wasmedgePluginWasmEdgeZlib' is defined using 'wasmedge_add_library' function with 'zlibenv.cpp', 'zlibfunc.cpp', and 'zlibmodule.cpp' as sources.
  2. A compiler flag '-DWASMEDGE_PLUGIN' has been provided to be used when compiling this new library.
  3. The include directories for the new library are specified with '$<TARGET_PROPERTY:wasmedgePlugin,INCLUDE_DIRECTORIES>' and the current source directory.
  4. Depending on the value of 'WASMEDGE_LINK_PLUGINS_STATIC', either 'wasmedgeCAPI' or 'wasmedge_shared' library is linked.
  5. The compiled library 'wasmedgePluginWasmEdgeZlib' is set to be installed in the '${CMAKE_INSTALL_LIBDIR}/wasmedge' directory.

Potential Problems:

  1. There's an assumption that the sources 'zlibenv.cpp', 'zlibfunc.cpp', and 'zlibmodule.cpp' exist. If they do not, the compilation will fail.
  2. If the variable 'WASMEDGE_LINK_PLUGINS_STATIC' is not set anywhere in the whole cmake project, it could lead to a potential configuration error or unintended linking.
  3. The target property 'INCLUDE_DIRECTORIES' fetched from the 'wasmedgePlugin' might not contain all necessary include files, resulting in compilation errors.
  4. If there is any issue with the 'wasmedgeCAPI' or 'wasmedge_shared' libraries that are being linked, like if they are not found or not built correctly, the build will fail.

Commit a98bca1ff6fcc073fc675c1d10ee272435fbcb86

The change made to the codebase in this pull request are:

  1. ZLIB has been added as a link dependency to the wasmedge_zlib plugin in its CMakeLists.txt file. This addition is necessary for the plugin's successful build since it likely utilizes zlib's functionalities. The change was applied for both cases where the plugin is linked statically (WASMEDGE_LINK_PLUGINS_STATIC is True) and dynamically (WASMEDGE_LINK_PLUGINS_STATIC is False). ZLIB is rightly linked as a private dependency, meaning that it is not exposed to other targets that link to this one.

Though the commit message and code changes are clear, the following potential problem could occur:

  1. The build could fail if ZLIB is not found in the system or project directory structure since it is added as a link dependency. This failure could occur if ZLIB is not installed or if its path is not correctly referenced in the cmake file. To fix this problem, a check should be added to make sure that ZLIB exists before trying to link it. If it is not found, an informative error message should be displayed.

Apart from this potential issue, everything else about this pull request seems to be in order. The developer has followed good practice by signing off the commit.

Commit 2b8668580ec73cdc0a3b737107385735c13b389d

The author of this patch, Saikat Dey, has added a basic test case for the zlib plugin in the WasmEdge project. This is represented by the new file 'wasmedge_zlib.cpp' in the 'test/plugins/wasmedge_zlib' directory.

Key Changes:

  1. New test file - wasmedge_zlib.cpp: This new file contains a single test, WasmEdgeZlibTest, Module, which tests several functionalities of the zlib module.
  2. Test setup and tear down - createModule(): A standalone function that attempts to create and load a WasmEdge zlib module from a location relative to the current path. If this fails, it returns null.
  3. Test case - WasmEdgeZlibTest, Module: The test confirms the validity of the loaded module, checks if its ZStreamMap is empty, and validates the presence of several function exports like deflateInit_, inflateInit_, deflate, inflate, deflateEnd and inflateEnd in the module. Then it deletes the loaded module.

Potential Problems:

  1. Hardcoded paths - The path to the plugin is hardcoded, which means the test could fail in any environment where the plugin location is different or if run from a different directory.
  2. Memory management - The createModule() function uses release() on a unique pointer, which can cause memory leaks. It's not clear who owns the returned pointer and when it should be deleted. The test case attempts to delete the module, but the function itself might also be used elsewhere, leading to confusion about responsibility for deleting the module.
  3. Test Coverage - Currently, there's only a single test case and it only checks very basic functionality (module creation and function existence in the module). This may not be adequate to fully test the robustness of the zlib plugin.
  4. Dependence on Dynamic Casting - The test dynamically casts the return value of createModule() to WasmEdge::Host::WasmEdgeZlibModule * type. If this fails due to a type mismatch, it could result in undefined behavior.
  5. Null checking - The function createModule() can return null, but it's not checked for in main(). This could lead to undefined behavior.

Commit bc857cce1c8e1299470bc85537d9ffb8d95cea0f

The commit titled "Added test/plugins/wasmedge_zlib/CMakeLists.txt to build and create zlib test executable." adds a new file, CMakeLists.txt, to the wasmedge_zlib plugin test directory.

Key Changes:

  1. An executable named wasmedgeZlibTests is added using the source file wasmedge_zlib.cpp.
  2. The newly created executable wasmedgeZlibTests is dependent on wasmedgePluginWasmEdgeZlib.
  3. Public include directories for the target wasmedgeZlibTests are added from wasmedgePlugin and wasmedgePluginWasmEdgeZlib.
  4. Google Test libraries are linked privately to the target.
  5. Depending on whether WASMEDGE_LINK_PLUGINS_STATIC is true, the target is linked privately to either wasmedgeCAPI or wasmedge_shared.
  6. A test named wasmedgeZlibTests is added, which runs the wasmedgeZlibTests executable.

Potential Problems:

  1. If dependencies such as wasmedgePluginWasmEdgeZlib, Google Test libraries, wasmedgeCAPI or wasmedge_shared are not properly set up or missing, the build may fail.
  2. If the source file wasmedge_zlib.cpp does not exist or has errors, the compilation for the new executable will fail.
  3. The test expectations aren't clearly defined in the commit. If the test fails, troubleshooting might take longer since it's not clear what the expected behavior should be. Detailed comments or documentation should be included to understand more about the test's expectations.
  4. If the WASMEDGE_LINK_PLUGINS_STATIC flag is not defined properly, it may lead to build errors or incorrect linking.

Commit 7b0ed641b3777d704520bfec70e9ebaee42543ca

Summary of changes:

This patch modifies the CMakeLists.txt file in the test/plugins directory. The modified file now includes an additional check for the -DWASMEDGE_PLUGIN_ZLIB flag which is likely used to compile the WasmEdge runtime with Zlib plugin support. If found, it adds a new subdirectory wasmedge_zlib to the list of directories to be processed by CMake.

Potential problems:

  1. This change assumes that the wasmedge_zlib directory already exists and contains a valid CMakeLists.txt file. If this directory or file doesn't exist, the build process as it currently stands could fail.

  2. There's no error checking for the presence or absence of the zlib library itself. If the user tries to enable this plugin, but doesn't have the zlib development files installed, the build will likely fail.

  3. This change is inserted unconditionally. While it's not necessarily a problem, the style of the file might require that all plugins are added in a certain order or grouped together in a certain way.

  4. The WASMEDGE_PLUGIN_ZLIB flag might not be documented, making it difficult for users to understand how to enable the zlib plugin. A README update might be necessary to address this.

Commit d6d969f87da1603f7926d9764e85c42d3557d5c9

Summary of Changes:

  • Saikat Dey has made modifications to two C++ source files, plugins/wasmedge_zlib/zlibmodule.cpp and test/plugins/wasmedge_zlib/wasmedge_zlib.cpp.
  • The major change lies in the refactoring of naming convention where the namespace prefix from the function names was removed. This action has resulted in reducing the complexity of function names within the WasmEdgeZlibModule::WasmEdgeZlibModule().
  • 6 host functions previously prefixed with "wasmedge_zlib_" such as "wasmedge_zlib_deflateInit_", are now simply referred to as "deflateInit_".
  • Corresponding changes were also made in the test file test/plugins/wasmedge_zlib/wasmedge_zlib.cpp to reflect the updated function names while checking for function exports.

Potential Issues:

  • This change could potentially affect places where these functions are being used or called. If there are other parts of code relying on these function names, they would face a function undefined error.
  • The use of prefix in function names is often a useful way to avoid conflicts between different modules or libraries. Removing this might increase the potential for name collisions.
  • Although not a problem, but it's worth to notify that these changes haven't altered the logic of the software but only its readability and potentially, maintainability. It's crucial to test thoroughly to ensure compatibility.

Commit 2027eacfb574416c9b0ec9f8c0c969a4b72e964f

Changes:

The developer has made corrections to the case of the library identifier 'ZLIB' in a CMakeLists.txt file. It seems like the ZLIB library reference was case-sensitive, and the file has been updated to reflect this, changing it from 'ZLIB' to 'zlib'.

This correction has been made in 4 different places in the same file:

  • In the FetchContent_Declare function where the zlib git repository is declared.
  • In the FetchContent_MakeAvailable function which is used to build the zlib library.
  • In the target_link_libraries function used twice in two different scopes to link the zlib library with wasmedgePluginWasmEdgeZlib.

Potential problems:

  1. If all references to ZLIB throughout the project aren't consistently corrected to 'zlib', this could cause build errors.
  2. There needs to be confirmation that the library is indeed supposed to be lowercase 'zlib'. If the original 'ZLIB' was correct, these changes could potentially cause issues.
  3. This patch doesn't appear to include any tests case updates, so if there were any tests that specifically check the referenced library 'ZLIB', they will fail.

The reviewer should check for these potential issues.

Commit 28cc2cf6f9c8ba90a5d53ed6b429a1afb75973e2

The Pull Request is primarily changing the build pipeline for a number of plugins in build-extensions.yml. It appears to be attempting to streamline and generalize the build process across multiple plugins.

Major Changes:

  1. The pipeline has been significantly refactored. Several build jobs have been consolidated into larger ones, significantly reducing the amount of redundancy in the pipeline configuration.
  2. In addition to reducing redundancy, the pipeline configuration has been simplified by using matrixes and loops to remove most identical steps across jobs. For example, the build_ubuntu process splits plugin-related operations into two sections: one specifically for wasi_nn and the other for the remaining plugins. These parallelization changes can help improve the efficiency of the overall process.
  3. Variable and configuration names have been standardized and simplified, leading to cleaner and more readable code.
  4. The environments in which the pipeline runs have been updated to use the latest ubuntu images.
  5. The process now explicitly defines the steps for dependency installation, plugin building, testing, and artifact uploading. This improves the visibility of each step in the process.

Potential Problems:

  1. This is a large change that affects multiple components of the entire build pipeline of plugins. Detailed testing should be done to ensure that the entire process still works as expected.
  2. The PR doesn't seem to include any new unit tests or modification of existing ones. Since this is a refactoring operation, existing unit tests, if any, must pass. Jenkins or other CI results need to be checked.
  3. Some parts of the pipeline depend on external scripts and libraries. These external dependencies should be tracked and tested to ensure that they do not break any functions.
  4. The changes are touching multiple scripts and dependencies. It is recommended to check whether all the used dependencies are well-maintained and up-to-date.
  5. The refactoring might cause an issue when troubleshooting. For instance, deciphering error messages can become more complex due to loops in the build process.

Commit 0d8c9b1873c1da8f5269f02e8be25cfa69ec1bc8

The key changes in this Pull Request are about correcting cmake build options in the build-extensions.yml file. Specifically, the "-DWASMEDGE_PLUGIN_WASI_ZLIB=ON" option was incorrectly named and has been corrected to "-DWASMEDGE_PLUGIN_ZLIB=ON". This change is made in two places in the build-extensions.yml file.

As these are string changes for build options, there seems to be no direct potential for causing problems. However, potential issues may arise if the updated flag "-DWASMEDGE_PLUGIN_ZLIB=ON" is not setup correctly in the cmake environment or if this flag has dependencies on other parts of the code.

Since the same change is applied to two different parts of the file, this might imply that the same environment is being setup twice. This suggests a possible area of improvement in the code, which might be to avoid this duplicated setup.

It would help if the author tested the build with the new flag, to ensure there's no further dependencies issue during build time. Also, providing a brief explanation for the change in the commit message might add clarity for future reference.

Commit d5d4f36480e2f3f65f0fc3115eca91cb4066f677

The provided patch concerns the CPP files for a plugin named "wasmedge_zlib". The key changes are mainly type changes made to better reflect the Zlib API. These are minor changes that involve altering the data types of the parameters for certain methods.

  1. Two functions within the file "zlibfunc.cpp", namely the "WasmEdgeZlibDeflateInit_::body" and the "WasmEdgeZlibInflateInit_::body" methods, have their "StreamSize" parameter type changed from "uint32_t" to "int32_t". These methods seem to be initializing some operations for deflate and inflate functionality.

  2. In the "zlibfunc.h" header file, the same parameter type changes for "StreamSize" are also reflected in the function definitions.

Due to the API type naming conventions, the change might be necessary for consistency purposes.

Potential problems:

  1. As the type of "StreamSize" has been changed from unsigned (uint32_t) to signed integer (int32_t), it could potentially allow for negative values. However, as the variable name suggests "StreamSize", which usually can not be negative, there might be a risk of undesirable behavior if negative values are passed.
  2. There is no apparent error checking after this change to account for negative inputs, which could lead to unexpected issues at runtime.
  3. The scope of changes is limited without changes to other associated code or documentation, which may lead to discrepancies or confusion.
  4. It is difficult to assess the full impact without knowledge of the wider code base or the specifics of how the Zlib API in the plugin uses these methods.
  5. If functions are invoked using old data types in other parts of the project, compilation errors may occur.

Commit a9e1bc206b6aa6f8ae23cb5e6d0136402f8f1e33

The patch adds a new unit test for the zlib plugin in the Wasmedge runtime - a test for deflate and inflate operations, named DeflateInflateCycle. Here are the key changes:

  1. A header file runtime/instance/module.h is added to the global #include list.
  2. A new liner #include <cstdio> is added to make use of the snprintf function.
  3. The fillMemContent() function is added to simplify memory population for tests.
  4. A DATA_SIZE constant is added, defining the sample data size for deflate/inflate operations.
  5. The helper randChar function is added to generate random characters for the test data.
  6. In the new DeflateInflateCycle test function, the zlib's deflate and inflate modules are checked for correctness and their exported functions are directly called.
  7. Next, some buffer space in memory is filled with random data, which is then deflated and inflated.
  8. Several tests are run, checking the correct response of the deflate and inflate functions, as well as the correct transformation of data.

Potential problems:

  1. The test generates a lot of data (1MB). It's unclear if this might lead to memory overflow or performance issues on certain systems.
  2. The many inclusion of EXPECT in the test might make debugging harder in case of a failure. It might be useful to refactor the test to smaller units.
  3. The presence and order of tests seem unusual and repeated. For example, WASM z_stream size mismatch and version mismatch tests are repeated twice. This repetition could be avoided by restructuring or refactoring.
  4. The test assumes that the deflate and inflate modules are correctly instantiated and findable. If changes occur in the module instantiating code in the future, this test might break.
  5. The test might fail in a non-deterministic way as the input data is generated randomly via the randChar function. If a failure occurs, it might not be reproducible. Consider seeding the random number generator with a fixed value for more deterministic tests.

Commit ba8bff1813d9d9d8aab23ddb1f34627a2bcfd94a

The key changes made in the pull request are related to the wasmedge_zlib.cpp file, where the author has removed two lines of code. Specifically, the initializer list where zalloc, zfree, and opaque properties of strm object are set to Z_NULL has been removed. The changes appear in a test function called DeflateInflateCycle.

The main implication of these changes is that the zalloc, zfree, and opaque properties of the strm object no longer get explicitly nullified in the code.

Review on Potential Problems:

  1. The author stated that the unnecessary nulling was removed since the 'host ignores them'. There is a chance that this 'ignorance' may be environment or situation dependent which the author might not have taken into account. Hence, further verification is needed.
  2. It also depends on how the strm object is defined and instantiated. If the constructor of the strm class does not initialize zalloc, zfree, and opaque to Z_NULL, and other parts of the program assume they are Z_NULL at the creation of the object, then bugs can occur.
  3. These changes make the code less explicit, and a future reader might not be clear why these properties are not nullified as they often are with zlib when the host system isn't providing a custom memory management schema.
  4. Finally, the removal of these lines might affect other tests or applications that are dependent on this state of the strm object in the 'DeflateInflateCycle' test. The impact of the change on other system components needs to be assessed.

Commit c081bf7acba97f54db57e1471dd13e6a39a8a9f8

This is a very specific patch that only modifies the plugins/wasmedge_zlib/zlibenv.h file. The key changes in this patch are:

  1. Addition of 2 lines.
  2. Introduction of a static_assert statement for the Wasm_z_stream struct, checking its size to be 56 bytes.

The purpose of this change appears to be ensuring that the size of the Wasm_z_stream structure remains consistent at 56 bytes. If the size changes unexpectedly, the code will not compile due to the static_assert statement, warning the developer of the structure size inconsistency. This might be significant in wasm environments where the size of C++ structures must align with the wasm host environment for proper interaction.

Potential problems:

  1. If future modifications change the size of the Wasm_z_stream structure, they would likely run into this assertion which would cause compilation to fail. Developers should be aware of this assertion when making changes to the structure.
  2. There is no explanation in the code as to why the size of the Wasm_z_stream structure must be exactly 56 bytes. Including such explanation in form of comments would improve the readability and maintainability of the code.
  3. Other structures within the file or related files which have size-dependent behavior might also require similar safeguards, this has to be checked and addressed appropriately.

Commit 8a0ab936de0d08ff13a27329ff220de9a04763fd

This patch from Saikat Dey is applied to the file 'wasmedge_zlib.cpp' and appears to change how memory is allocated in the WASM (WebAssembly) heap.

The key change in this patch is that the initial address from which writing to the WASM heap begins has been shifted from address 0 to address 1. This is done to avoid a clash with the semantic meaning of address/offset 0 as a null pointer (nullptr), which is a potential source of bugs and undefined behavior.

While the change itself seems straightforward, there are a few potential implications and issues:

  1. There don't appear to be any immediate validations or additional changes in response to this modification, therefore it's not clear if creating any offset in the heap would have downstream effects.

  2. It's not clear that avoiding writing at address zero totally prevents nullptr issues as other sections of the code might still assume the usage of zero.

  3. This change assumes that the offset is always valid at the location '1'. However, there aren't any checks or validations for potential edge cases where this might not be applicable.

Overall, without more context or information, while safe-per-se this change could potentially lead to unforeseen side effects. Especially when considering that memory management in WebAssembly is a critical aspect of ensuring the safe and efficient performance of WASM applications.

Further testing and validation would be recommended to ensure that this does not introduce any bugs or undesired side-effects.

Commit 20cf963e9d9ed67cfff06e180f920bb0f38e39e1

The proposed patch primarily improves and refactors the zlib plugin for Wasmedge. The critical changes introduced are:

  1. The individual checks for WasmZlib version mismatch and stream size mismatch in the WasmEdgeZlibDeflateInit_ and WasmEdgeZlibInflateInit_ have been refactored out into a new function CheckVersionSize(). This function returns a boolean value after checking the input version and stream size.
  2. The bulky bilateral error logging and exception throwing for version and stream size mismatches have been replaced with an invocation of CheckVersionSize(). Now, if the check fails, a Z_VERSION_ERROR is subsequently returned.
  3. The test code has been updated to reflect these changes. Now, instead of expecting a false return value for deflateInit_ and inflateInit_ on mismatch (indicating an error), a true return value is expected along with the Z_VERSION_ERROR value.

From a reviewer's perspective, these changes seem to significantly reduce code redundancy and improve modularity, which are highly desirable qualities. However, there are still a few potential issues that should be addressed:

  1. Removing error logging: The new code does not log errors when the version or stream size doesn't match the expected values. This might make debugging more difficult when these errors occur.

  2. Changing test expectations without new test cases: The tests have been updated to match the new behavior. However, it is not confirmed if any new tests were added to check for this specific error condition. If not, they should be added to ensure the new functionality is validated properly.

  3. The new function CheckVersionSize() uses hard-coded values for verification. Maintaining these values might become problematic in the future if changes are necessary. Any future changes would require updating these hard-coded values in the code, which might not be efficient or error-prone.

To address the potential issues, maintain error logs for visibility into code execution paths. Consider adding more granular tests to validate the new changes, and think about refactoring hard-coded values in the code for ease of future updates.

Commit 43def5948309900e09cc219df1bb0091b000d35d

The commit involves the changes to the plugins/wasmedge_zlib/zlibfunc.cpp file. Here, the developer Saikat Dey has added a comment block in the CheckVersionSize() function to explain why the function only compares the first character of zlib version string rather than the entire string.

The key changes in the pull request are:

  1. Added a section of comments in the code to explain why only the first character of the Zlib version string is compared while checking for Zlib version within the CheckVersionSize function. This is done presumably to make the code easy to understand and to clarify the logical decision behind version checking.
  2. The comment section includes two external links referencing the zlib-ng and madler/zlib repository blobs that provide further context and rationale behind this approach.

Potential Problems:

  1. While the code doesn't have any functionality changes, maintainability might be an issue if the repositories mentioned in the links change their structure or if the files at these paths are moved or deleted. These links may then become obsolete, leading to confusion.
  2. The comment does not explain the rationale behind the version check strategy directly. It assumes that the readers will visit the provided links to understand the reasoning, which may not be convenient for all users.
  3. As a reviewer, one would also need to check these external links for appropriateness of the reference and to ensure that they provide sufficient context and justification for the approach.
  4. This change relies on the fact that zlib versioning strategy will not change in the future. Changes in zlib versioning policy might require updates to this function.

Commit cce004ac0a792a9ead6dfc4708885912dc6adb5f

The key changes in this patch revolve around improving the safety of pointer usage in the areas of memory handling for Deflate and Inflate operations in the WasmEdgeZlib plugin.

In detail:

  1. The commit removes the global, start-of-memory pointer *WasmMemStart, which might be considered unsafe because it was referenced throughout the code.
  2. It introduces offset-based pointer calculation. The next_in and next_out pointers are now calculated directly from the WasmZStream’s next_in and next_out areas.
  3. These pointers now start their life as offsets, and only get converted to actual pointers when they are used by the deflate and inflate functions.
  4. The patch also introduces PreComputeNextIn and PreComputeNextOut variables which store the starting points of the input and output pointers for the deflate/inflate operations.
  5. Following the deflate/inflate operations, next_in and next_out are updated based on the difference between their value post-operation and their initial, pre-operation value.

Potential Problems:

  1. The use of getPointer might risk null pointer dereferencing if the MemInst object does not hold the right memory or if the WasmZStream does not have initialized areas for next_in and next_out.
  2. Any change in the underlying memory between the pre-compute and post-compute steps could potentially yield invalid/inconsistent results. If this is multithreaded code or if there are any asynchronous callbacks that can modify the base memory, it could cause issues.
  3. Direct pointer arithmetic like HostZStream->next_in - PreComputeNextIn could potentially lead to undefined behavior if for some reason HostZStream->next_in ends up being less than PreComputeNextIn.
  4. This refactor does not include explicit handling for such edge cases, and there doesn't seem to be any error checking after operations like deflate or inflate. If these operations were to fail for some reason, the results could propagate inappropriately through the system.

Commit c74505d1cc0785cb78c6eba3afc346c05be913d3

This pull request introduces in-code documentation to every field in the Wasm_z_stream struct in the file plugins/wasmedge_zlib/zlibenv.h. It seems that 18 lines were added which include comments explaining each field while two lines were deleted which likely were comments.

Here is an overview of what these fields do based on the added comments:

  • next_in is the [Wasm Offset] next input byte.
  • avail_in is the number of bytes available at next_in
  • total_in is the total number of input bytes read so far
  • next_out is the [Wasm Offset] next output byte will go here
  • avail_out is the remaining free space at next_out
  • total_out is the total number of bytes output so far
  • msg is the [Wasm Offset] last error message, NULL if no error
  • state is the [Wasm Offset] not visible by applications
  • zalloc is used to allocate the internal state
  • zfree is used to free the internal state
  • opaque is the [Wasm Offset] private data object passed to zalloc and zfree
  • data_type is best guess about the data type: binary or text for deflate, or the decoding state for inflate
  • adler is the Adler-32 or CRC-32 value of the uncompressed data
  • reserved is reserved for future use

Potential problems could be:

  • If comments are inaccurate, it may confuse future developers.
  • If this is not the standard way of adding comments in this project, it could break consistency.
  • Comments do not affect functionality, so no functional issues should arise from this change.

The most significant finding is the improved documentation which could assist in comprehensibility & maintainability of the code for the team or other contributors. However, it is necessary for the comments to be accurate and consistent with the rest of project's coding practices.

Commit 89004feaa93b434d164561a5dc6c563f78162833

The pull request modifies three files: plugins/wasmedge_zlib/zlibenv.h, plugins/wasmedge_zlib/zlibfunc.cpp, and test/plugins/wasmedge_zlib/wasmedge_zlib.cpp.

Key Changes:

  1. The struct name 'Wasm_z_stream' is changed to 'WasmZStream' to follow the LLVM naming conventions. Every occurrence of 'Wasm_z_stream' has been replaced by 'WasmZStream'. This renaming operation seems to follow through on all files as it should.

Potential Problems:

  1. The code apparently does not add any new functionality or remove any existing one, so it shouldn't break any existing test. However, the renaming might impact code that is not included in this request, if there are references to the older struct name 'Wasm_z_stream' in other parts of the code base.

  2. No new test is added to test the change. Although it's a straightforward renaming operation, it might still be good to test the code again with the changed struct name to ensure nothing breaks.

  3. There is no update in the documentation related to this code change. If any parts of the documentation or comments mention the changed struct name, those should be updated to reflect the change.

Overall, there seems to be very little potential for errors, assuming all references have been updated correctly. The renaming operation could have been automated using a refactoring tool, which would reduce the risk of missing any occurrences. The only area to pay attention to would be ensuring that all references in the codebase are updated accordingly.

Commit 705dea5cf2787c0eb96753501436a273b425a810

In the pull request, the author made changes to update a struct name for the compliance with LLVM's naming convention. The struct was previously named 'Wasm_z_stream' which was changed to 'WasmZStream'. This change has been done in the zlibfunc.cpp file inside the wasmedge_zlib plugin.

Key Changes:

  • The name of the struct has been changed from 'Wasm_z_stream' to 'WasmZStream' in the all the lines where it was referenced.
  • The two-line code that assigned the pointer of 'WasmZStream' to 'ModuleZStream' has been condensed to one line.

Potential Problems:

  • Other files or contributors using the old 'Wasm_z_stream' structure name will encounter issues if they haven't also updated the struct name, and this could result in compilation errors.
  • Since this change is only implemented within this file, if this struct is used in other files in the codebase without these changes being implemented, it might lead to errors.
  • The patch is a part of larger chain (one of 98 PATCHes), so skipping or rejection of this patch might cause inconsistency in style or even in functionality downstream.

It is recommended to cross-verify in the entire project for the usage of this struct and update it accordingly to prevent inconsistency and future errors. Update all associated documentation or communication to keep every contributor on the same page.

Commit c7b4de2296111970e0b9a0c08a31f30cdfcf398c

The pull request involves a change in a single line of code in one file (zlibfunc.cpp) in the wasmedge_zlib plugin.

The main modification is that a C-style cast ((int32_t)sizeof(WasmZStream)) has been replaced with a C++ static_cast<int32_t>(sizeof(WasmZStream)). C++ static_cast is generally safer and more readable than the old C-style casting.

Key change:

  • (int32_t)sizeof(WasmZStream) changed to static_cast<int32_t>(sizeof(WasmZStream)).

Potential issues:

  • No logical or functional change is apparent, so unless the codebase is being uniformly updated to use C++ style casting this change might not be necessary.
  • If this change is meant to enforce a standard of using C++ style casting in the codebase, similar changes should be observed in the rest of the codebase. If not, there may be a lack of uniformity in code style.
  • It's important that the submitter tests this change rigorously despite its minor nature, as type casting can have unexpected outcomes if not carefully handled.
  • As this is patch 36 out of 98, it is recommended to review the entire set of changes for a comprehensive understand of the modifications and their potential impact.

Commit 7465495da118be34e2cc25d5830838ff1a3852c8

Summary of Changes:

The developer has made changes to the code in wasmedge_zlib.cpp. They modified some variable names to make them follow LLVM (Low Level Virtual Machine) standards.

This is a part of the test case for the WasmEdge Zlib Plugin.

Below are the changes:

  1. Changed variable name randChar to RandChar.
  2. Changed variable name charset to Charset.
  3. Changed variable name max_index to MaxIndex.

The call to randChar was also updated to RandChar in the statement std::generate_n(MemInst.getPointer<char *>(wasm_hp), DATA_SIZE, RandChar);

Potential issues:

  • Compilation Issues: If any of the modified variable names are referenced elsewhere in the project, this could result in compilation errors. The reviewer needs to ensure that all references to these variables were updated in this commit as well.
  • Randomness: The function RandChar, which is used to generate random characters in the test case, relies on std::rand(). This may have implications for the quality of randomness, especially in a multithreaded context. Consider using a thread-safe and better pseudo-random number generator if necessary.
  • Testing: Since these changes are within test scripts, it's important to rerun tests to ensure they all pass with the updated variable names.

Apart from the potential issues mentioned above, these changes are simple refactorings that improve code consistency and are unlikely to introduce new bugs themselves.

Commit 4805d6daba4c3530a1f168649d0d443ef050e7b4

This patch does not introduce any functionality changes but rather modifies the naming convention of some variables to align with LLVM standards. The key change here is that the z_res variable has been renamed to ZRes in multiple locations in the zlibfunc.cpp file.

Concerning this change, the following functions were affected:

  • WasmEdgeZlibDeflateInit_::body(const Runtime::CallingFrame &Frame,
  • WasmEdgeZlibInflateInit_::body(const Runtime::CallingFrame &Frame,
  • WasmEdgeZlibDeflate::WasmEdgeZlibDeflate::body(
  • WasmEdgeZlibInflate::body(const Runtime::CallingFrame &Frame,

Each function has a line where the z_res variable was used to capture the result of a function call (either deflateInit_, inflateInit_, deflate or inflate). Following that, the z_res variable was then cast to an int32_t and returned. Now, instead of z_res, the name ZRes is used in identical fashion.

Potential problems might arise if this naming scheme deviates from other parts of the application, but in general the change seems to follow LLVM's guidelines for variable naming and should be fine. It's also important to ensure that these changes are reflected in any parts of the codebase, such as documentation or tests, that may reference these variable names.

Commit 731d55a5c084e8381b680487f2daa57f26d808dd

This pull request titled "[Plugin] zlib [LFX 2023 Term 2]" by Saikat Dey includes changes for improved synchronization and wrapping of zlib calls in the wasmedge_zlib plugin:

  1. A comprehensive Host and Wasm Stream Synchronization Function, namely SyncRun, has been added which takes in a callback function. The SyncRun function handles proper synchronization between the host and the wasm stream, including updates on in/out streams and bound attributes. All zlib calls have been wrapped in this new function, improving code maintainability and reducing redundancy.

  2. The zlib calls that have been wrapped with the new synchronization function include deflateInit_, inflateInit_, deflate, inflate, deflateEnd and inflateEnd.

  3. The Pull Request removes a lot of redundancy where certain operations were earlier repeated for each of the zlib calls. These redundant operations are now performed inside the SyncRun function.

Key potential issues:

  1. While the inline lambda functions wrapped in SyncRun improve readability, they could introduce potential misbehaviors or bugs, if not tested thoroughly.

  2. Error handling for the zlib library calls is still reliant on returning the zlib result directly with very little context. Robust error handling needs to be implemented and examined.

  3. Thread safety is unclear from this diff. Changes to the shared state involving HostZStream and ModuleZStream, as well as manipulations in SyncRun, need to be examined for potential data races especially when concurrent threads are run.

  4. It would be good to have comments or description for SyncRun and the wrapped methods explaining their working.

Commit 31517720ef0fb52338bd1f287b4693f78b82a3de

The developer has made changes to comments in the file zlibfunc.cpp of the plugin wasmedge_zlib.

Key changes include:

  1. The original comment // ignore msg, state was split into two parts // TODO: ignore msg for now and // ignore state.
  2. The above change was made twice in the file, once for the function auto SyncRun and another in the same function defined some lines below.
    3.The TODO comment at the end of the file, which contained a note to sync *msg in [inflate|deflate]End(), was removed.

Potential problems:

  1. The developer has marked "ignore msg" as a TODO now, which signifies that he plans to address this in the future. If forgetting to address this ignored message might lead to bugs or inefficiencies, it might be worth adding more context or tracking it in an issue for better visibility.

  2. The comment removal at the end might indicate an intention to not act upon syncing *msg in [inflate|deflate]End(). If this syncing was crucial to the code functionality or performance, it could be a point of concern.

  3. The changes in this pull request are mainly comments which might not have the potential to break functionality. However, they provide insights about the future development plan and current decision of the developer about ignoring certain aspects of the appllication.

Please note that review comments are based purely on the comments and isolated view of this patch. To fully assess the impact or potential problems, it might be useful to understand the whole context in its totality i.e., the purpose of ignored properties and whether the removal of the last comment was just a cleanup or it indicates an actual change of plan.

Commit dbb9f7aac80179a320b96ebeaabfd1c3f0308557

The following key changes have been made in the 'zlibfunc.cpp' file:

  1. Conditions have been introduced in 'WasmEdgeZlibDeflateInit_' and 'WasmEdgeZlibInflateInit_' functions to register z_stream only if the initialization was successful (i.e., if ZRes == Z_OK) . This is to prevent any potential issues caused by an unsuccessful initialization.

Potential issues identified:

  1. This conditional implementation doesn't cater to the case when ZRes != Z_OK (error cases). It might be necessary to consider handling these scenarios, perhaps with an appropriate error log or user prompt. Currently, the functions will just finish after unsuccessful initialization, which can potentially lead to silence failure.

  2. If 'ZStreamPtr' points to invalid memory location, or if allocation was not successful, the making of pair and emplacement in 'ZStreamMap' would cause a crash. This patch assumes that 'ZStreamPtr' is always a valid pointer without verification.

  3. There's no apparent functionality to remove the pair from the 'ZStreamMap' when it's no longer needed. This could potentially lead to memory leaks over time.

  4. The commit does not come with unit tests. It is hard to determine its scope and effectiveness.

Minor Note: The patch looks simple and small, and hence does not introduce a major change. The author's future contribution in the PR might further alter this functionality, make sure to consider this context while reviewing.

Commit a23011d2748b85503a7845149b532f1cc1db1cf2

Summary of Key Changes:
The author, Saikat Dey, has modified zlibfunc.cpp file within the wasmedge_zlib plugin. He has:

  1. Removed the existing CheckVersionSize function which was previously used to verify the zlib version and stream size.
  2. A new function CheckSize has been created to just verify the stream size.
  3. CheckVersionSize invocations have been replaced with CheckSize in WasmEdgeZlibDeflateInit_::body and WasmEdgeZlibInflateInit_::body.
  4. Inflator and deflator initialization now directly uses WasmZlibVersion instead of the hardcoded ZLIB_VERSION string.

Potential Problems:

  1. Conventionally, version checks are important to ensure compatibility. Here, the version check has been removed. This can lead to compatibility issues if the current implementation is not compatible with all versions of the zlib library.
  2. Version information is not being verified now. Thus, if an incorrect version is passed, the function may behave unpredictably.
  3. Future changes in the zlib library that may render the current code nonfunctional won't be catched during version checking. This could lead to runtime errors which might be more difficult to trace.

Suggested Course of Action:
While the change may streamline the code and remove an apparently unnecessary check, it is crucial to evaluate the potential implications of removing the version check. Consequently, it would be necessary to add a line in the function documentation on its compatibility with zlib versions, ensuring users are aware of which versions it will work with.

Commit 0bafab8428d763bdc81da762add6fbac4eb5a3e8

The key changes in this Pull Request focus on adding new functionality to the zlib plugin within the WasmEdge project. The developer has introduced two new methods, WasmEdgeZlibDeflateSetDictionary and WasmEdgeZlibDeflateGetDictionary to compress and decompress data using a provided dictionary. These changes involve 63 insertions and 2 deletions across two files (zlibfunc.cpp and zlibfunc.h).

Summarizing the changes in this patch,

In zlibfunc.cpp:

  1. In the functions WasmEdgeZlibDeflateInit_::body and WasmEdgeZlibInflateInit_::body, the pointer type was replaced from const char * to const auto *, which is beneficial as it reduces ambiguities. This is considered an improvement.

  2. The WasmEdgeZlibDeflateSetDictionary::body and WasmEdgeZlibDeflateGetDictionary::body functions are added. These new functionalities allow setting and getting a compression dictionary within the zlib library.

In zlibfunc.h:

  1. The developer declares two new classes WasmEdgeZlibDeflateSetDictionary and WasmEdgeZlibDeflateGetDictionary. The definition of these classes are spread across the zlibfunc.cpp file.

Potential Problems:

  1. There are no null checks for pointers Dictionary and DictLength obtained from Frame.getMemoryByIndex(0). This could lead to null pointer dereference or segmentation faults if the pointer values aren't valid.
  2. There's no check for the validity of ZStreamPtr before its use. If this value isn't valid, the resulting HostZStreamIt could end up invalid, leading to errors further along in the execution of the function.
  3. Error handling for deflateSetDictionary and deflateGetDictionary methods is not clear from the patch. If these methods return errors, it is necessary to manage them appropriately.
  4. The absence of any tests provided with these changes is concerning. Robust tests could ensure these functions work as expected.

Commit e5c147e2969e6c5e35ee960cfe9a69f72406dab8

The key changes in this patch are:

  1. The addition of the WasmEdgeZlibDeflateCopy class which seems to implement the deflateCopy() function. Existing in both the zlibfunc.cpp and zlibfunc.h files, this function copies the compression state information from a source to a destination.

  2. The body method of WasmEdgeZlibDeflateCopy attempts to locate the source in the ZStreamMap. If it doesn't find it there, it returns an ErrCode::Value::HostFuncError.

  3. If the source is found, body creates a new z_stream structure called DestZStream, where it intends to copy the source information.

  4. Then it executes SyncRun() on the source and then on the newly created destination. If the copying operation is successful, it adds the DestZStream to the ZStreamMap and returns the result ZRes.

Potential problems:

  1. There's a lack of error checking/handling. While there is an error check for SourceZStreamIt not being found in ZStreamMap, I don't see any similar checks for DestPtr before using it, which could lead to uncertainty around the state of DestZStream.

  2. When ZRes doesn't equal Z_OK, the function returns ZRes immediately, which could include many different states that aren't handled explicitly.

  3. There is also no check for whether DestPtr already exists in ZStreamMap. This could cause a memory leak as we silently overwrite the old z_stream with the new one, without deleting the old one.

  4. We're missing a destructor or proper cleanup after usage of the new unique_ptr.

  5. Comments are used to provide function descriptions instead of using function documentation, which would be more useful to developers trying to understand the codebase.

  6. There may be a missing corner cover for the SyncRun function.

Commit 076cd31dd377024b8da05ceebcb6063bb67efbe7

Summary of Key Changes:
The patch introduces a new functionality in the form of a method called deflateReset within WasmEdgeZlibDeflateReset class in the WasmEdge namespace. This function essentially resets the deflation of a ZStream using the zlib library.

  • In zlibfunc.cpp, the new function WasmEdgeZlibDeflateReset::body() is defined which expects a runtime frame and a ZStream pointer as arguments and returns an integer representing the zlib status code. Inside the function, it checks if the ZStreamPtr is present in the map ZStreamMap and then does a deflateReset on it using the zlib library function.

  • In zlibfunc.h, a new class WasmEdgeZlibDeflateReset has been declared which extends from WasmEdgeZlib; this new class includes the constructor declaration and the new method body declaration.

Potential Problems:

  1. Error Handling: There's a bit of error handling present here if the provided ZStreamPtr is not found in the ZStreamMap. However, we're throwing a generic HostFuncError. It might be more user-friendly to throw a more specific error related to ZStream not found, which would make debugging easier for developers.

  2. Correctness of Process: As for the deflateReset function itself, it's critical that it only be called when it's safe and appropriate to do so as per respective zlib documentation. It's the responsibility of the developer to ensure this.

  3. Absence of Test Cases: Notably, this patch does not include any new test cases. Any new feature or method introduction should ideally be accompanied by relevant unit tests to ensure the newly introduced logic is working as expected.

  4. Thread Safety: The implemented method doesn't explicitly handle thread safety. If the ZStream objects can be accessed concurrently by multiple threads, there should be some mechanism to ensure that resources are safely accessed or modified.

  5. Memory Management: The code assumes that it’s okay to simply reset the deflation process and reuse a ZStream. However, if there are any unfinished operations or unhandled memory, simply applying deflateReset without proper cleanup could potentially cause memory leaks or other issues. The developer should be aware of the state of the ZStream before resetting it.

Commit 6242f27efe63f14a5072615cb88da5e3766b5310

Summary of changes:

  1. A new delegate function WasmEdgeZlibDeflateParams has been added to the zlib plug-in, which presumably changes the parameters of the deflate algorithm for a given stream.
  2. The newly introduced function WasmEdgeZlibDeflateParams::body method takes ZStreamPtr pointing to the stream, Level which might be the compression level, and Strategy as parameters.
  3. The body method of WasmEdgeZlibDeflateParams attempts to find the specified stream, and if it is not found, it returns HostFuncError.
  4. If the specified stream is found, the parameters of the deflate function are changed using deflateParams.

Potential Problems:

  1. There are no checks for the validity of the Level and Strategy parameters. The deflateParams function may fail or behave unexpectedly if invalid parameters are given. This can lead to runtime errors or unreliable results.
  2. There doesn't seem to be any exception handling mechanism in case the deflateParams function encounters any errors.
  3. If ZStreamPtr doesn't exist in ZStreamMap, the function directly returns an error code. Not having a backup plan or recovery mechanism may leave the system in an inconsistent state.
  4. There are no unit tests provided in the pull request to validate that the new changes work correctly. This makes it hard to assert the correctness of the new feature.
  5. Lastly but importantly, the pull request is missing a proper description about what it does. A detailed explanation about the changes and their purpose would be beneficial for reviewers and future maintainers.

Commit 57df5c780ee956fb32e69b49984601d46f469d88

This pull request by Saikat Dey contains two significant additions to the wasmedge_zlib plugin: deflateTune and deflateBound.

  1. deflateTune: This new function is added to the WasmEdgeZlibDeflateTune class. It seems to be intended for tuning deflate's internal compression parameters, goodLength, maxLazy, niceLength, and maxChain. The function first check if the provided ZStreamPtr exists in the ZStreamMap. If it doesn't exist, it returns a HostFuncError. Otherwise, it executes a SyncRun on deflateTune.

  2. deflateBound: Added to the WasmEdgeZlibDeflateBound class, this function seems designed to give an upper bound on compressed output given the input length, sourceLen. The function's behavior if it cannot find the provided ZStreamPtr in the ZStreamMap is similar to deflateTune. If ZStreamPtr exists, it executes a SyncRun on deflateBound.

From the initial review of the changes, here are some potential issues:

  • Error Handling: Errors for these new functions are only thrown when the z-stream cannot be located. It's unclear how the program handles other potential sources of failure within the deflateTune and deflateBound//operations//.
  • Unclear Comments: There's a link in the comments for WasmEdgeZlibDeflateBound that points to an issue on GitHub, but no explanation of how it's related.
  • Thread Safety: The new functions use SyncRun. Whether this is thread safe or not is dependent on the implementation of SyncRun which is not shown in this diff. If it isn't thread-safe, potential issues might arise when these operations are used concurrently.

Commit 84a3e9cb571ce203ee31d3712500d3261d521c1b

The developer, Saikat Dey, is adding functionality to the WasmEdge Zlib plugin.

Changes:

  1. In 'zlibfunc.cpp', two new functions are defined - 'WasmEdgeZlibDeflatePending' and 'WasmEdgeZlibDeflatePrime'. Both these functions retrieve a Zlib stream from the 'ZStreamMap' based on a provided key. If the key is not present in the map, the functions return a 'HostFuncError' error code.

    • 'WasmEdgeZlibDeflatePending': This function accepts 4 parameters - a Runtime Frame, a Zlib Stream Pointer, and pointers to an integer referring to the number of pending output bytes and another integer for number of bits. It then runs 'deflatePending' on the retrieved Zlib Stream object, which modifies the number of pending output bytes and the number of bits that are ready to be output.

    • 'WasmEdgeZlibDeflatePrime': This function takes 4 parameters - a Runtime Frame, a Zlib Stream Pointer, and integers representing the number of bits and their value. It runs 'deflatePrime' on the retrieved Zlib Stream, which inserts bits into the deflate output stream.

  2. In 'zlibfunc.h', two new classes 'WasmEdgeZlibDeflatePending' and 'WasmEdgeZlibDeflatePrime' are declared. These classes inherit from a template base class 'WasmEdgeZlib'. The classes declare a constructor and the respective 'body' function mentioned in the 'zlibfunc.cpp' changes.

Potential issues:

  1. There are no null checks performed when the pointers 'Pending', 'Bits', or 'Value' are retrieved from memory in their respective functions. This can lead to issues if null pointers are passed as arguments.

  2. If the 'deflatePending' or 'deflatePrime' methods fail for some reason, these failures are not handled in 'WasmEdgeZlibDeflatePending::body' and 'WasmEdgeZlibDeflatePrime::body'.

  3. There are no comments explaining the purpose and usage of these new additions. This can make maintenance and future developments harder.

In light of these issues, I would recommend adding null pointer check. Also, it would be beneficial to add error handling after calling the 'deflatePending' and 'deflatePrime' methods. Lastly, adding documentation or comments explaining the usage of these new features is recommended for better maintainability.

Commit 0a9329c0faf65b2f8188d66de848f01416b3fd5d

The key changes in this pull request titled "[Plugin] zlib [LFX 2023 Term 2]" are focused on the file named "zlibenv.h" within the "wasmedge_zlib" plugin.

Changes include:

  1. The author, Saikat Dey, defined a new structure 'WasmGZHeader' that represents the gzip header information. This structure was not in the original code.

The WasmGZHeader structure includes fields such as:

  • text: A boolean indicating whether the compressed data is believed to be text
  • time: The timestamp when the file was modified
  • xflags: Extra flags
  • os: The operating system
  • extra: Pointer to any extra field
  • extra_len: Length of the extra field
  • extra_max: Space at the extra field
  • name: Pointer to the zero-terminated file name
  • name_max: Space at the name field
  • comment: Pointer to the zero-terminated comment
  • comm_max: Space at the comment field
  • hcrc: A boolean indicating whether a header CRC is or will be present
  • done: A boolean indicating whether reading the gzip header is complete
  1. The code also includes a static assertion to ensure that the size of the 'WasmGZHeader' struct equals 52 bytes.

Potential issues:

  1. The lack of comments in the struct definition may lead to difficulty in understanding the purpose of each field especially for fields like 'hcrc', 'done', 'xflags', 'extra', etc.

  2. Some of the fields in the 'WasmGZHeader' struct are declared as pointers (such as 'extra', 'name', 'comment'). There are no null checks for these pointers, so they could potentially cause null pointer dereferencing issues if not handled properly.

  3. It's not clear from the patch if the structure is being properly initialized somewhere in the code. Without correct initialization, the variables can contain garbage values.

  4. The type for time is specified as 'uint32_t' which typically represents an unsigned 32-bit integer. This may lead to problems if the time is expected to be in a format that can't be correctly represented by a uint32_t.

Commit ff5154dfcb68efc9c78526207795fd58f65189b4

The changes in this pull request are focused on the plugins/wasmedge_zlib files and include the implementation of several zlib related functions.

In the 'zlibfunc.cpp' file, a total of nine functions were added. These functions include 'WasmEdgeZlibInflateSetDictionary', 'WasmEdgeZlibInflateGetDictionary', 'WasmEdgeZlibInflateSync', 'WasmEdgeZlibInflateCopy', 'WasmEdgeZlibInflateReset', 'WasmEdgeZlibInflateReset2', 'WasmEdgeZlibInflatePrime', 'WasmEdgeZlibInflateMark', and 'WasmEdgeZlibInflateBackEnd'. Each function follows a similar pattern to find a specific pointer in the 'Env.ZStreamMap', perform a specific operation using that pointer and then return a result. However, specific operations performed by each function vary.

In the 'zlibfunc.h' file, the same nine functions have been declared, each in their own class which inherits from the 'WasmEdgeZlib' base class.

Potential problems:

  1. The same 'WasmEdgeZlibInflateSetDictionary' function implementation code seems to be duplicated three times, this is likely a mistake.
  2. There is little to no error handling in these functions, besides checking if the required pointers exist in the 'Env.ZStreamMap'. This could lead to crashes or unwanted behavior if something goes wrong.
  3. Commenting is limited - while the functions are decently self-descriptive, this could still pose a coherent understanding problem for other developers looking at the code.
  4. If any of these functions are called with invalid parameters, the behavior is likely to be unexpected as there are no checks on the input arguments.
  5. Memory management: memory allocated with calls like 'getMemoryByIndex' or 'getPointer' is not freed explicitly, which might lead to memory leaks.
  6. There is also a lack of test coverage for these new functions. Without unit tests, we cannot confidently say that these functions behave as expected.

Commit 10125fd541230e547ee361e2308603e83282f23c

This commit adds non-gz functions to a Zlib plugin for the WasmEdge runtime. Major changes include:

  1. Several Non-GZ related functions have been added to the zlib plugin including WasmEdgeZlibDeflateSetDictionary, WasmEdgeZlibDeflateGetDictionary, WasmEdgeZlibDeflatePending, and WasmEdgeZlibInflateSetDictionary.

  2. New validation checks have been added to ensure that a valid MemoryInstance is provided before executing a function. If the MemoryInstance is null, it returns an error of type HostFuncError.

  3. The commit also introduces functions for compression and decompression such as WasmEdgeZlibCompress, WasmEdgeZlibCompress2, WasmEdgeZlibCompressBound, WasmEdgeZlibUncompress and WasmEdgeZlibUncompress2.

  4. It also introduces a function called WasmEdgeZlibZlibCompilerFlags which gets the compression settings.

The potential problems and observations are:

  1. The newly added functions have not been documented, adding comments to these methods would make it easier to understand the intention of functions.

  2. There is potential undefined behavior in some new functions like WasmEdgeZlibCompress::body(). The pointer HostDestLen is not initialized before it is dereferenced (the code *HostDestLen = *DestLen; is called before HostDestLen has been assigned a valid address).

  3. Code repetition: Error handling for null MemoryInstance has been repeated across all newly added methods. To improve the code's maintainability, this could be refactored into a helper method.

  4. There are no tests included that cover these new added functions which might potentially induce regression.

  5. Review if correct memory management procedures are used since it deals with raw pointers. It'll be beneficial to monitor issues related to memory leaks, buffer overflow, or segmentation faults.

Backward compatibility doesn't seem to be affected as this Pull Request only adds more functionalities and does not modify existing ones. Though it'll be crucial to ensure the new functionalities perform as expected.

Commit 9102cbb0db678867f88a089f4c996ac8be3fd821

The primary changes in the patch revolve around the addition of four new functionalities, namely adler32, adler32_z, crc32, and crc32_z to the WasmEdgeZlib plugin. The mentioned changes can be broken down as follows:

  1. A modification is applied to the WasmEdgeZlibUncompress function in zlibfunc.cpp, being renamed to WasmEdgeZlibUncompress2. The function seems to be the core routine for the decompression process.

  2. Four new functions are added to zlibfunc.cpp namely WasmEdgeZlibAdler32, WasmEdgeZlibAdler32_z, WasmEdgeZlibCRC32, and WasmEdgeZlibCRC32_z. These functions enable Adler-32 and CRC-32 checksum calculations for input data. Each function retrieves a memory instance, checks if it's valid, computes the checksum using it, and returns the result.

  3. Corresponding class declarations for WasmEdgeZlibAdler32, WasmEdgeZlibAdler32_z, WasmEdgeZlibCRC32, and WasmEdgeZlibCRC32_z are added in zlibfunc.h.

Potential Issues:

  1. Error Handling: Each of the new methods assumes that the memory fetch (Frame.getMemoryByIndex(0)) is always successful. If not, the attempt to access the memory instance could cause a null pointer exception. This might need additional error checking and handling.

  2. Function Renaming: The pull request changes the WasmEdgeZlibUncompress function to WasmEdgeZlibUncompress2, but it is not clear why this modification is necessary or whether it could potentially break existing references to the function. This could create issues in other parts of the program that utilize this function.

  3. Absence of Test Cases: The pull request does not include any test cases or examples to illustrate the correct behavior of the newly added functions. Adding proper test cases would be a good practice to confirm the correctness of the code.

Overall the changes seem sound in terms of extending the functionality of the plugin, but they could be improved by addressing these potential issues.

Commit 28bf85d99fdc87c69b6d9192ce105e6dd5720246

The key change in this patch is the renaming of version variables in two jobs within a GitHub Action workflow. Specifically, the "_v2" postfix has been removed from "get_version_v2" in two instances. This change affects the naming of the uploads in the "Upload artifact - wasmedge_zlib" steps of the build-extensions.yml GitHub Action file.

The important consideration associated with this change is ensuring that "needs.get_version.outputs.version" is a valid, existing reference. If the get_version action output does not exist or has not been properly initialized in the workflow, it could lead to failures in the job where the variable is used. This would result in failed attempts to name and upload the artifact correctly, potentially causing the entire workflow to fail.

Another issue could be if other parts of the codebase rely on the old "_v2" postfix. If such references are not updated, this renaming could lead to further errors.

It would be recommended to test this change thoroughly, as well as verify that no other jobs depend on the changed output variable name. Additionally, ensure that the versioning scheme conforms to semantic versioning standards to avoid any confusion.

Commit 1e31c2d57a7110e85ab85a784c3f4698c9488826

This patch from developer Saikat Dey concerns the file 'zlibfunc.cpp' in the 'wasmedge_zlib' plugin.

Key changes:

  • The most notable change is the removal of a function named 'WasmEdgeZlibInflateSetDictionary', containing 26 lines of C++ code. This function takes a 'CallingFrame', 'ZStreamPtr', 'DictionaryPtr', and 'DictLength' as parameters and returns an expected 'int32_t' result.
  • A 1-line insertion has been made where the original function had been placed, changing the function 'WasmEdgeZlibInflateSetDictionary::body' to 'WasmEdgeZlibInflateGetDictionary::body'.

Potential issues:

  • Without the context provided by surrounding code, one cannot be certain about certain issues. However, if 'WasmEdgeZlibInflateSetDictionary::body' is being called somewhere else in the project, removing it could potentially cause errors or unexpected behavior.
  • The new inserted line suggests the 'WasmEdgeZlibInflateGetDictionary::body' function will take the exact same parameters as the deleted 'WasmEdgeZlibInflateSetDictionary::body'. If this is not the case, there could be parameter mismatches. Furthermore, if the behavior of these two functions is different, it could lead to unexpected results or software bugs.
  • The change could potentially affect backward compatibility if previous versions of the software rely on the removed function.
  • It is also worth noting that removing this implementation might impact the overall functionality of the software, especially if 'WasmEdgeZlibInflateSetDictionary' is essential in the original source code.

Commit c607f08608ae34aa0bea1c967883555ec315b415

Key Changes:

  1. The developer removed the 'Frame' parameter from two functions, WasmEdgeZlibZlibCompilerFlags::body and WasmEdgeZlibCompressBound::body. From the diff, we can see that 'Frame' was not used within the functions, which is likely why it was removed.

Potential Problems:

  1. If the WasmEdgeZlibZlibCompilerFlags::body and WasmEdgeZlibCompressBound::body functions were designed to have a consistent interface with other similar functions in this system, this change could break that consistency.

  2. If other code is calling these functions, this change could break that code. The developer will have to make sure they find all usages of these functions and update the calling code. Otherwise, it could lead to compile errors.

  3. Interested parties, like plugin users or maintainers, should be notified about the function signature change, as it may affect them. If a warning is not issued, it may surprise external users with interface changes.

Please inspect the entire codebase for the usage of these functions to confirm that no other areas are affected by this change. If tests were written for these functions, they should be updated as well to reflect the changes.

Another thing is the change is quite specific with no general application or framework modification, the purpose of the change should be clearly released in the version's log and documentation to prevent unnecessary confusion.

Commit bea8b35d152fcbd8af573f3a8a9f597ec6b5c574

The code changes made in this patch involve the handling of HostDestLen, and in the final function, HostSourceLen, in four functions: WasmEdgeZlibCompress::body, WasmEdgeZlibCompress2::body, WasmEdgeZlibUncompress::body and WasmEdgeZlibUncompress2::body.

In the initial code, HostDestLen and HostSourceLen were handled as pointers, which could be problematic due to potential instability or mishandling of memory. The updated code modifies HostDestLen and HostSourceLen to be unsigned long variables and passes their addresses to the compress, compress2 and uncompress functions, which may take care of some memory handling issues. After function execution, the values are assigned back to *DestLen and in the WasmEdgeZlibUncompress2::body function additionally to *SourceLen.

Key issues to verify would include:

  1. Correctness: The change of variable handling from pointer to non-pointer type should be validated for its appropriateness in the broader context of the software.
  2. Compatibility: Make sure these changes are tested across different platforms and scenarios to ensure there are no unforeseen impacts.
  3. Exception Handling: Error checks or exception handling around the compression and uncompression functions seem to be missing. It would be a good practice to handle and interpret any error code returned by these functions for robustness.

No major issues seem to be introduced by this pull request. However, extensive testing is recommended due to changes in memory handling.

Commit 7ee7a324269d8cee42601c5c5e11432678c29487

The pull request's title suggests adding zlib support to a WasmEdge plugin, most likely in a project related to WebAssembly. There are three main files that are modified: zlibenv.h, zlibfunc.cpp, and zlibfunc.h.

Key Changes:

  1. zlibenv.h: A new map, GZFileMap, that holds unique pointers to gzFile has been introduced. This map contains gzFile pointers that are sorted in decreasing order of their associated uint32_t keys.

  2. zlibfunc.cpp: This file has multiple additions. A new constant, WasmGZFileStart, is defined as the size of a gzFile. A new function, WasmEdgeZlibGZDOpen::body, is implemented to open a gzip file and store it in GZFileMap.

  3. zlibfunc.h: New classes, WasmEdgeZlibGZDOpen and WasmEdgeZlibGZBuffer, are declared. These classes are set to provide functionality related to opening and buffering gzip files.

Potential problems:

  1. Handling Errors: When using the method gzdopen() to open gzip files, it's essential to check for failure. If this function fails for any reason (like the file does not exist or permission issues), it can return NULL. The current code does not seem to handle these errors.

  2. Memory Leaks: While handling pointers, especially new and delete, care must be taken not to leak memory. It is not clear from the patch how possible memory leaks are being handled when using the std::unique_ptr<gzFile>.

  3. Thread Safety: The map GZFileMap might be accessed from different threads, depending on how these classes are used. If so, there's a need for handling concurrent access to prevent potential race conditions.

  4. Exception Handling: As with any code, there should be good exception handling in place to ensure that any unforeseen issues do not crash the program. From the given code snippet, we cannot ascertain the level of exception handling in place.

The above issues should be handled before merging this pull request.

Commit b54d1269ff185632a99d8c467f49a3b614899b45

Summary:

  1. Added a type alias for gzFile to WasmEdgeZlibEnvironment class, in zlibenv.h. This alias is used in the map GZFileMap.

  2. The file zlibfunc.cpp has been modified to implement gzbuffer, gzsetparams and gzread functions in WasmEdgeZlibGZBuffer, WasmEdgeZlibGZSetParams and WasmEdgeZlibGZRead classes respectively.

  3. The function calls all check the existence of GZFile in GZFileMap and then call the respective zlib functions on the obtained gzFile object.

  4. For WasmEdgeZlibGZRead, it additionally fetches the memory instance and checks it for null.

  5. Added the declarations of these three classes in zlibfunc.h.

Potential Problems:

  1. Error Checking: The patch involves some error checking (for instance, verifying GZFile's presence in GZFileMap). However, it may not account for all potential error scenarios (like the validity of arguments being passed to zlib functions).

  2. Manual Memory Management: Care needs to be taken here due to the use of raw pointers and std::unique_ptr. If not handled correctly, this could lead to memory leaks or crashes. However, the creator of the patch here seems to be familiar with memory management concepts like unique_ptr, implying they've likely handled these concerns appropriately.

  3. Computational Complexity: The patch uses a std::map for GZFileMap, where lookups (like find) are log(n). If the GZFileMap gets large and lookups are frequent, this could be a performance concern, since log(n) is slower than the constant time lookups of std::unordered_map.

  4. Thread Safety: Since multiple threads may access and modify GZFileMap, concurrency issues might arise. But without additional context, it's unclear whether the application is multi-threaded or not.

  5. Exception Handling: The functions implemented here return an Expect<T> object, and it's unclear how the user of these functions handles the error cases (corresponding to Unexpect(ErrCode::Value::HostFuncError)). Depending on error handling strategy higher in the call stack a poor user experience might result.

Commit 5025c3d961fea830bfadfb337f014ccffe213ed5

This pull request introduces three new features to the zlib plugin for the WasmEdge runtime: gzfread, gzwrite, and gzfwrite. These functions allow for increased interaction with gzip files in a WebAssembly context.

Here are the key changes:

  1. WasmEdgeZlibGZFread class and its body function: This function reads data from a gzip file into an array buffer of a specified size.

  2. WasmEdgeZlibGZWrite class and its body function: This function writes a buffer of a specified length into a gzip file.

  3. WasmEdgeZlibGZFwrite class and its body function: Similar to the gzfread, this function writes data from an array buffer into a gzip file.

It's worth noting that in each function, it searches for the GZFile in Env.GZFileMap and checks if the file exists. If it doesn't, it returns HostFuncError. Each function also gets a memory instance and checks if it is nullptr. If it is, they return HostFuncError again.

However, there are some potential problems and requirements for additional information:

  • Error handling: There seems to be just basic error handling. Therefore, it would be useful to have more detailed error messages, so we know which user action caused the HostFuncError. For instance, was it because the memory was nullptr or because the GZFile was not found.

  • Test coverage: I couldn't see any tests associated with these changes in this diff. Without test coverage, we have no way of ensuring these new functions will work as intended.

  • Input validation: There is no visible input validation for the new functions. More specifically, there are no checks for null or invalid inputs for the gzfread, gzwrite, and gzfwrite APIs.

  • No file open/close mechanism: If the system has to handle numerous files simultaneously, it could cause too much resource consumption. It's essential to validate directory traversal, file opening, resource closure, etc.

Ultimately, while the committed changes help enhance the functionality of the plugin, some considerations should be made towards enhancing error handling, test coverage, and input validation to ensure the robustness of the new features.

Commit 08725d480c8443d42c9bb371fbcb33c50b8d24c8

The key changes in this pull request are as follows:

  1. Four new functions, gzputs, gzgets, gzputc, gzgetc are implemented in the WasmEdgeZlib class in the Wasmedge zlib plugin. These functions are wrappers around the respective zlib library functions and they interface with a Wasm runtime.
  2. The gzputs and gzputc functions (for writing a string and a character, respectively, to a gzip file) are properly implemented in zlibfunc.cpp.
  3. The function body for gzputs obtains the gzFile pointer associated with an identifier from an environment mapping, it then gets a string pointer from the Wasm memory, and finally, it calls the gzputs function.
  4. Similarly, the function body for gzputc obtains the gzFile pointer associated with an identifier, it then calls the gzputc function with the given character.
  5. The gzgets and gzgetc functions (for reading a line and a character, respectively, from a gzip file) seem to be declared in zlibfunc.h, but their function bodies are not yet implemented. This is probably an oversight.

Potential problems:

  1. A possible issue in the pull request is the implementation of gzputc function. It contradicts with the function name as it calls gzgetc (which is used to read a character from the compressed file) instead of gzputc.
  2. The pull request lacks error handling if gzputs or gzputc fail to write to the file.
  3. The functions gzgets and gzgetc are declared but not defined, which could lead to linking errors.

Commit 9da46e01fe3f8be30b536499de407a473e43ef7a

Summary of Key Changes:

  1. Added new functionalities to the zlib plugin of a WASM Edge:

    • Two new methods gzflush and gzrewind have been added. These features have been implemented within two new classes WasmEdgeZlibGZFlush and WasmEdgeZlibGZRewind respectively.
  2. Implemented gzflush method:

    • This function takes GZFile and Flush as parameters. It flushes the content of the gzip object. It uses the gzflush() function from the zlib library.
  3. Implemented gzrewind method:

    • This function takes GZFile as a parameter. It rewinds the gzip object. It uses the gzrewind() function from the zlib library.

Possible Potential Problems:

  1. Exception Handling:

    • In cases where GZFile is not found in the GZFileMap, the function just returns an unexpected error code, making it a bit difficult to understand what exactly went wrong.
  2. Thread safety:

    • This patch doesn't contain anything addressing thread-safety, which might lead to issues if multiple threads are accessing the same GZFile simultaneously.
  3. Memory Leak:

    • The GZFile objects are being accessed directly from the map with no apparent mechanism for closure or release. If this isn't properly handled elsewhere in the code, it could potentially cause memory leak issues.
  4. Code Duplication:

    • The error handling for when the GZFile is not found in the GZFileMap seems to be repeated twice, once for each of the new functions. Making the error handling a standalone function could reduce the code duplication.

Overall, the changes seem logically sound and properly implemented, but they could benefit from additional checks for error handling, thread safety, and potential memory leaks. Reducing code duplication can also improve maintainability.

Commit 44689d86bf46e429ad6d22683c3ea5fa582598bf

The pull request titled "[Plugin] zlib [LFX 2023 Term 2]" from Saikat Dey has made changes in the zlibfunc.cpp file. Here are the primary changes made and potential issues.

Key Changes:

  1. The unused parameter 'Frame' from the 'Runtime::CallingFrame' namespace has been removed. It was done in several places, including methods like WasmEdgeZlibGZBuffer::body, WasmEdgeZlibGZSetParams::body, and others.
  2. The parameter list has been reformatted to be on separate lines for particular methods, improving readability and aligning with good coding standards.
  3. The number of additions(+7) and deletions(-8) signifies refactoring of the code, rather than new functional changes being made.

Potential Issues:

  1. There is a lack of function comments associated with the changes. It's hard to estimate the exact purpose of the removed parameter. Any other person looking at the code will not understand the purpose of the change.
  2. If the 'Runtime::CallingFrame' parameter was not used currently but was meant to be used in a future update, its removal can lead to rework or confusion for future developers.
  3. Since the changes are only linked to the removal of the 'Frame' parameter, all dependent functions using these methods might need to be refactored or changed as well, which isn't highlighted at all.
  4. There are no updates on the unit tests to reflect or test these changes. Any major update should ideally be covered in new or updated unit tests.
  5. Any changes in the way the functions handle data may have downstream applications, especially encapsulation behavior. This should be reviewed in depth depending on the plugin's full scope.

Commit ae9f871b429bf8004f1a8b71df785bf0827d8d4b

The pull request is an enhancement to the WasmEdge Zlib plugin. The developer, Saikat Dey, has added new functionalities by extending classes for multiple zlib functions.

Changes:

  1. In zlibfunc.cpp, five functions related to zlib operations have been added: (gzeof, gzdirect, gzclose, gzclose_r, gzclose_w). This means end-of-file check, direct reading, and various closing operations are now included in the plugin. For each function, there are checks to ensure that the related file exists in the GZFileMap before attempting to perform any operations. If the file is not found, an exception is thrown.

  2. In zlibfunc.h, the corresponding class definitions for above functions (WasmEdgeZlibGZEof, WasmEdgeZlibGZDirect, WasmEdgeZlibGZClose, WasmEdgeZlibGZClose_r, and WasmEdgeZlibGZClose_w) have been provided. Constructors for the classes have also been defined, accepting a WasmEdgeZlibEnvironment object as an argument.

Findings:

  1. code repetition: The code to check if the GZFile exists in the Env.GZFileMap is repeated for each newly added function. This could be refactored into a helper method to improve readability and maintainability.

  2. missing error message: When throwing an exception with ErrCode::Value::HostFuncError, it would be helpful to include a descriptive error message.

  3. variable naming: It's not problematic, but the variable name ZRes for storing the results of the zlib operations might benefit from a more descriptive name. It may enhance readability for other developers.

  4. checking function results: For the results of gzclose, gzclose_r, and gzclose_w functions, there is no error checking. It's recommended to check the results of these functions, as they might fail in some cases (such as disk space issue, etc.)

  5. gzdirect function appears twice: Developers seem to have mistyped and copied gzdirect where they meant gzclose. That would lead to a failure when trying to compile the code.

Recommendations:

  1. Refactor the GZFile existence check into a helper method.
  2. Include a descriptive error message with HostFuncError.
  3. Use more descriptive variable names.
  4. Add error checking for the gzclose, gzclose_r, and gzclose_w operations.
  5. Correct the second instance of WasmEdgeZlibGZDirect to be WasmEdgeZlibGZClose.

Commit 429bda7dead85360841f03dbf8598bce4f3bc780

Summary of Key Changes:
This patch appears to be part of a larger set, being number 64 out of 98. The author, Saikat Dey, has added a new functionality for error handling in 'wasmedge_zlib' plugin. Specifically, they have added a 'gzclearerr' function in 'zlibfunc.cpp' which allows for error states in a 'gzFile' to be cleared. The function is a method of the 'WasmEdgeZlibGZClearerr' class, which is defined in 'zlibfunc.h'. The 'gzclearerr' function uses 'HostFuncError' as return value in case if 'GZFile' is not found in 'GZFileMap'.

Potential Problems:

  1. Lack of Error Handling for 'gzclearerr': The use of function 'gzclearerr' directly with no check on its result may be a concern. If 'gzclearerr' fails for any reason, this may not be detected and may lead to a potential silent failure that could be difficult to debug.

  2. Missing Context for 'WasmEdgeZlibGZError': The commented out 'WasmEdgeZlibGZError' class definition in 'zlibfunc.h' lacks a definition in 'zlibfunc.cpp'. If this is an essential part of error handling, it might cause an issue.

  3. Absence of Unit Tests: The pull request does not include any accompanying tests to verify that the additions to the codebase are working as expected.

  4. Documentation: There has been no documentation or comment lines describing what the 'gzclearerr' function does. This could make maintenance and future code improvements more difficult.

  5. Thread Safety: If this code is expected to run in a multi-threaded environment, the direct access and modification of 'GZFileMap' without any form of synchronization or locking could lead to inconsistent states or race conditions.

Commit 5e3f68649b97f4f67d16d4871a2eade552c86217

Summary of Changes:
The key change in this pull request revolves around the data type of the 'Buf' pointer in four different methods (WasmEdgeZlibGZRead, WasmEdgeZlibGZFread, WasmEdgeZlibGZWrite, WasmEdgeZlibGZFwrite) of the zlibfunc.cpp file. The developer has replaced 'void *' with 'unsigned char *'. The developer did this to define the specific size of the data type the pointer is referring to. A void pointer does not have any data type associated with it and can hold an address of any type. In contrast, an unsigned char pointer specifies the type of instanced data.

Potential Problems:
While this change may seem trivial at first, its effects depend largely on the rest of the codebase which was not included in the patch:

  1. Compatibility: Changing the type of pointer may cause problems elsewhere in the code if other parts of the code are depending on this function and assuming it will return a void pointer. These parts of the code may fail if they cannot handle pointers to unsigned char.

  2. Code Complexities: A reinterpret_cast might be required elsewhere in the codebase if the void pointer was being used to handle multiple different data types.

  3. Utility methods: If these methods are meant to be used as utility functions, modifying the pointers to be of a specific type i.e., 'unsigned char *' limits the usability of these functions.

  4. Breaking polymorphism: If the code relies on polymorphic behavior with the void* pointer (relying on late-binding of the pointed-to type), this change to 'unsigned char*' will break that behavior, causing unexpected results.

Leaving out these potential issues, the code change itself seems fine as long as 'unsigned char*' is the desired type to be used broadly across the application where these functions are being called. It's important to remember that checking the wider usage and compatibility of this change remains crucial for a successful incorporation into the codebase.

Commit c18a1040a8636cec084c7b7b8af30175332eee2b

The changes in this pull request include adding new features and functionality. They involve additions and minor subtractions of the code, increasing the functionality of the Zlib plugin for WasmEdge. The key changes can be summarized as follows:

File: zlibfunc.cpp

Three new methods were added to the file:

  • A method was added to write characters (gzputc) in the ZlibGZPutc class, which performs error checking before making an attempt to write the character.
  • The ZlibGZGetc class was created with a method to get characters (gzgetc) from the gzip file, checking for errors before attempting to do so.
  • Similar to gzputc & gzgetc, ZlibGZUngetc was also created and a new method gzungetc was added to unget characters into a gzip file.

File: zlibfunc.h

The class WasmEdgeZlibGZGets was commented out, means it's methods and variables cannot be accessed in the plugin.

Potential problems:

  1. The removal of the class WasmEdgeZlibGZGets may cause errors if it was being used elsewhere in the code, as it has been commented out.
  2. Detailed error handling or exception throwing is absent within the added methods. If the new methods fail to execute correctly, it might be difficult to trace the errors.
  3. The implementation of these methods haven't been shown to pass any tests, and the behavior of these implementations under edge cases or incorrect uses hasn't been demonstrated.
  4. There are no "catch" blocks to handle the exceptions thrown. If any exception is thrown, it could crash the entire application. This isn't a good practice when considering robust software development.
  5. Code documentation is missing, making it hard for other developers to understand the purpose and functionality of the added methods. Poor documentation can lead to errors or inefficient collaborative development.

Commit ab3d3e0432035c37840ae1dfd9b18f8b09c633b8

Key changes:
The main change in this patch involves a renaming operation, specifically in the method signature. The method WasmEdgeZlibGZDirect::body is renamed to WasmEdgeZlibGZClose::body. All other lines are identical with no functional change.

Potential problems:

  1. Renaming must be consistent: The change in this Pull Request seems to only rename one occurrence of WasmEdgeZlibGZDirect::body to WasmEdgeZlibGZClose::body. It is essential to ensure that this renaming is consistent across the entire codebase. If there are any other references to the old method name (WasmEdgeZlibGZDirect::body), they will break and cause compilation errors.

  2. Check for method usage: If the old WasmEdgeZlibGZDirect::body method is being used elsewhere in the codebase and its use-case doesn't align with the semantic understanding that comes with the new name WasmEdgeZlibGZClose::body, then the usage needs to be reviewed and possibly refactored.

  3. Test coverage: Ensure that there are adequate tests covering this method, and the tests are adjusted to account for the renaming.

Commit 643e32d8860f1e96be04465f0415a1d551c5f222

Key Changes:
The developer Saikat Dey has made a small but significant change to the WasmEdgeZlibGZClearerr::body function in the zlibfunc.cpp file (part of the wasmedge_zlib plugin). Specifically, the developer added a return statement. After the function call to gzclearerr, the function now returns Expect<void>{}.

Potential problems:
Based on the patch provided, it's hard to identify any specific problems because it seems like a pretty safe and straightforward change. However, some issues might arise depending on the broader context:

  1. Usage of Expect object: Returning an Expect<void>{} object doesn't clearly communicate the status of the operation. Although Expect class design may contain a success status indication, it's not immediately clear. If gzclearerr produces important return information that should be considered, the current approach ignores it. The developer might want to add some error handling here.

  2. Compatibility: If other parts of the code are anticipating a different return type, this change could cause type mismatch issues or even break the build. This depends on how this function is being used in the larger codebase.

For a thorough review, inspecting the full file and understanding the overall working of the plugin would be necessary.

Commit 21a6c6a922dce874134868c1be0c3f610ed2c1e0

From the above patch, there are a few notable changes in the file plugins/wasmedge_zlib/zlibfunc.cpp:

  1. In the method WasmEdgeZlibGZDOpen::body, there has been a modification to how a std::pair is being inserted inside the GZFileMap, which is presumably a std::map.

    • Previously: The code was directly creating and inserting a pair of a new WasmGZFile and unique pointer to the gzFile into GZFileMap.
    • Now: The code first creates an auto type element El and then moves (using std::move()) El into GZFileMap. There also seems to be a change in the type of std::unique_ptr.
  2. The return type of WasmEdgeZlibGZDOpen::body function has been changed. It now always returns 0 instead of NewWasmGZFile.

Potential Problems:

  • The return value is hardcoded as 0 in the WasmEdgeZlibGZDOpen::body. This could potentially cause problems if the outside functions rely on the returned value to track the newly inserted WasmGZFile.
  • If there is any error in the gzdopen function, it's not accounted for in the following lines. Use of the result ZRes without checking for errors could lead to undefined behavior or crashes.
  • The change of std::unique_ptr from gzFile to WasmEdgeZlibEnvironment::GZFile_s might cause problems if not handled outside of this function. It is unclear without more context whether other parts of the program are aligned with this change.
  • Original use of std::make_unique<gzFile>(ZRes) ensures the correct handling of memory. It's unclear why it was changed and it might bring memory-related bugs.
  • Debugging might be harder due to the use of auto keyword as it may not always be clear what type El is. This is, however, a minor issue and also somewhat opinion-based as some developers prefer using auto.

Commit c83cb6ee29079f598d95c323c2791b27e51143f6

This pull-request modifies two files plugins/wasmedge_zlib/zlibfunc.cpp and plugins/wasmedge_zlib/zlibfunc.h. The developer refactored the code and added additional deflate and inflate functions along with corresponding Init and End methods.

Key Changes:

  1. Added functions deflateInit2, inflateInit2, inflateBackInit2, and modified corresponding body() functions for deflateInit, inflateInit, deflateEnd, and inflateEnd.
  2. Commented out WasmEdgeZlibZlibVersion class and several memory check statements in SyncRun()
  3. Added error returns on occasions if memory or a stream was not found.
  4. Removed redundant classes such as WasmEdgeZlibDeflateInit_ and WasmEdgeZlibInflateInit_.
  5. Updated WasmEdgeZlibDeflateInit_ and WasmEdgeZlibInflateInit_ to check for stream sizes and version errors.

Potential Problems:

  1. Removing the memory check from SyncRun could possibly lead to Null Pointer Exceptions.
  2. The reason for commenting out the WasmEdgeZlibZlibVersion class is not clear and could lead to missing functionality if it was in use before.
  3. Error handling has been added but the error messages are not descriptive which might make debugging difficult.
  4. There is a lot of use of Unexpect(ErrCode::Value::HostFuncError). If these are not handled at a higher level, it could potentially crash the system.
  5. There are a few large code blocks that have been moved around. This could potentially impact the clarity and readability of code.
  6. The removal of version and stream size checks in the WasmEdgeZlibDeflateInit and WasmEdgeZlibInflateInit functions might lead to unexpected behaviour and input exploitation risks.

Commit 87b26fa532228440302a5bb35655a907c013ca32

The patch introduces several new features and refactors a few existing ones in a zlib plugin.

New Features:

  1. Added gzopen(): This function opens a gzip (.gz) file. The code indicates that the gzopen() implementation takes two arguments: PathPtr and ModePtr. If there is an issue while getting the memory instance, the function will return an error.
  2. Added gzseek(): This function seeks a given offset in the gzip file specified. It returns the resultant offset on success or an error if the file cannot be found or other issues occur.
  3. Added gztell(): This function tells the reading position in the gzip file specified. It returns the current offset or an error if the file can't be found or other issues occur.
  4. Added gzoffset(): This function gets the current reading offset for the gzip file.
  5. Added adler32_combine(): This function combines the Adler-32 checksums of two independent blocks of data.
  6. Added crc32_conbine(): This combines CRC-32 checksums of two independent blocks of data.

Refactoring:

  1. The original WasmEdgeZlibAdler32, WasmEdgeZlibAdler32_z, WasmEdgeZlibCRC32, WasmEdgeZlibCRC32_z classes have been refactored and moved around, but their core behavior seems to remain the same.
  2. In these refactored implementations, substantial code blocks checking for null memory errors have been retained, thus preserving the original error-handling.

Potential problems:

  1. No explicit handling for gzopen failure: In the implementation of gzopen(), there is no explicit check for whether the file opening was successful or not. It's assumed that the file opening operation was successful and the resultant file object is directly used without checking for errors. If there is a failure in opening a file in certain modes or if the file doesn't exist, this could potentially lead to runtime errors due to null pointer dereference or usage of an invalid file object.
  2. Absence of validity checks while accessing memory: For functions that access memory via pointers, such as MemInst->getPointer<Bytef *>(BufPtr) in the WasmEdgeZlibAdler32::body() function, there should be checks to see whether the pointer obtained is valid or not.
  3. Return value in WasmEdgeZlibGZOpen's body function: It seems that regardless of any other operation or event, the function returns 0. This might lead to ambiguity in understanding the status of the function since 0 generally indicates no errors. It would be useful to return different values or appropriate error codes for different scenarios.
  4. No Input Data Validation: There seems to be no validation on the input data properties like file size, path validity, mode specification, etc. before performing operations like open and seek, which can lead to unexpected issues or failures.

Commit 13cff39912b6a2a15757dab0e1120cedf5d0e929

The developer, Saikat Dey, has made several changes in the zlib plugin of a WasmEdge project. These changes span across two files, namely zlibfunc.cpp and zlibfunc.h. Here's a brief summary of the changes:

  1. The developer has introduced new functions (deflateInit2_, inflateInit2_, inflateBackInit_) to the zlibfunc.cpp and zlibfunc.h files. Each function includes parameters for handling specific zlib operations such as compression level and methods, memory strategy, and window bits.

  2. Existing code has been updated for better readability and efficiency. This includes changing HostZStream.get()-> to HostZStream->, which is a more efficient way of accessing members of a smart pointer.

  3. The developer has also added error checking before operations that rely on the stream size. It returns Z_VERSION_ERROR if the stream size check fails, providing better error handling.

Potential issues include:

  1. Code review should verify if the newly added functions handle all expected scenarios and edge cases. Moreover, the newly implemented error checks might change the function behavior and could potentially break the existing execution flow, which needs to be reviewed thoroughly.

  2. No actual deletion of old functionality is found in the patch, but for the codes commented as 'was ignored', it might be a sign that the developer is ignoring certain functionality intentionally or without fully understanding the implications.

  3. The check for MemInst == nullptr has been added multiple times which increases the cyclomatic complexity and could potentially introduce more errors. A single check before proceeding for further execution might be a more efficient approach.

  4. The review should ensure that the introduced changes do not affect the performance of the software as zlib operations are often critical for performance.

  5. There are no evident unit tests associated with these new functions. Testing these changes, given the complexity of the operations involved, is crucial.

Commit e72b1a22bdb353a671bf8dc3901253f14ebd418a

The pull request is made for adding a new method named gzgetc_ in an existing zlib plugin. Following are the changes:

  • 20 lines of code have been added across two files namely zlibfunc.cpp and zlibfunc.h within wasmedge_zlib plugin.

  • zlibfunc.cpp has the implementation of the function, where the GZFileMap is looked up for the GZFile received as parameter. If it exists, gzgetc_ method is invoked on it.

  • The signature of this new function is added in the header file zlibfunc.h where WasmEdgeZlibGZGetc_ class retrieves integer from the gzipped file.

Potential Issues:

  • There is no error handling if gzgetc_ fails.
  • There are also no checks to ensure that the GZFile is open or valid before it's used.
  • The return type of gzgetc_ function is assumed to be int32_t. If the function signature changes or it starts returning a different type, this could lead to issues.
  • The commit does not include any tests, so it's not obvious how these changes have been validated.
  • Adding more context in the pull request about this change's purposes would be beneficial for understanding the change and its potential impact.

Commit f295530caffde8ceb7a70b774a87f35afce191c3

This Pull Request is a code modification for a plugin named 'wasmedge_zlib', likely a zlib library for the WebAssembly Edge runtime. It has significant changes:

  1. There are 139 additions and 5 deletions majorly in two files: 'zlibfunc.cpp' and 'zlibfunc.h'.

  2. The developer added several new functions including 'WasmEdgeZlibInflateSyncPoint', 'WasmEdgeZlibInflateUndermine', 'WasmEdgeZlibInflateValidate', 'WasmEdgeZlibInflateCodesUsed', 'WasmEdgeZlibInflateResetKeep' and 'WasmEdgeZlibDeflateResetKeep'. These functions likely extend the functionality of the plugin.

  3. In 'zlibfunc.h', several classes were introduced, resembling the function names in the 'zlibfunc.cpp' file. These classes likely represent the implementation details of their counterpart functions.

Regarding potential problems:

  1. Error Handling: Every new function has error handling related to 'HostFuncError'. It checks if a ZStreamPtr key exists in ZStreamMap. However, there's no context about what happens if it doesn’t find the key or misbehaves.

  2. No Removal of Old Code: The code retains a function 'WasmEdgeZlibGZGetc_' that seems to have been replaced by new functions - this might be redundant or unnecessary.

  3. Testing: PR doesn’t include any tests. Given the significant changes, unit and integration tests should be added to ensure the proper working of new functions.

  4. Documentation: There's no link to the documentation outlining what these new functions do and how they alter the plugin functionality. This could cause problems with maintainability and usability.

Overall, the PR adds substantial functionalities related to inflate and deflate operations but lacks testing, error handling details, and documentation, which ideally should be included.

Commit d9e30b99376779fc7c80673ce1d26a1ec06ca17b

The contributor, Saikat Dey, created a patch to add a new class named WasmEdgeZlibGZVPrintf to an existing zlib plugin. This class derives from a templated WasmEdgeZlib class and appears to be designed for working with compressed files.

Key changes:

  1. An initial structure for the WasmEdgeZlibGZVPrintf class was defined in the header file, with its own constructor and a body method. This body method expects three parameters: a frame from the runtime, and file pointers for a gzipped file and a format.
  2. The constructor delegates initialization to the base class, WasmEdgeZlib, using HostEnv.

Possible issues:

  1. Currently, the new class is commented out, which means it won't have any effect on the code at all. If this is intended to be a working feature, the comments around the class declaration need to be removed.
  2. The body method has an Expect return type, and it is not currently clear where Expect is defined or what it does. This can potentially cause issues if it is not properly handled where the function is going to be used.
  3. The body method is not implemented yet. It only declares expectations.
  4. The Runtime::CallingFrame and WasmEdgeZlibEnvironment types are not defined in this patch, which suggests that they likely come from an external library or another part of the project not included in this patch. This could lead to problems if those parts of the project have been changed or removed.
  5. The commit message does not conform to best practices. It does not provide a rationale for the change or any information about whether or not this is a breaking change or what risks are involved.

Commit 50a1032285550d3a515d4e5957a83f5b760720c3

The key changes made in this pull request, specifically in the zlibfunc.cpp file, are:

  1. The Runtime::CallingFrame &Frame parameter in the body method of each class (WasmEdgeZlibAdler32Combine and WasmEdgeZlibCRC32Combine) is removed, as it is unused in the function body.

The patch involves deleting seven lines and adding seven new ones to replace them in the zlibfunc.cpp file. The changes look minimal and organized.

Potential Problems:

  1. If current code or future modifications of the code require the use of Frame, this removal will introduce errors or incompatibility. Understanding why Frame was initially present and ensuring it isn't necessary now or in the future is crucial.

  2. The removal has been done assuming Frame is unused and not affecting performance and functionality. If its existence had any hidden consequences, like an indirect impact on performance, there could be potential issues.

It is generally good practice to remove any unused variables or objects as they take up memory space, can decrease code readability, and increase complexity unnecessarily. Nevertheless, it's important to confirm no specific reason to keep it in the first place. It is highly suggested to have unittests around the modified function to ensure its behavior remains the same. Also, the deletion won't affect any other dependent functionality.

Commit 1d44df29f409a3edac08fd2a8d111be64d992a93

Summary:

The provided changes can be divided into 3 main sections:

  1. A series of modifications in 'zlibenv.h'. This includes changes of field names inside two structs, WasmZStream and WasmGZHeader. The developer has decided to shift naming style to 'LLVM style', where each word in a variable/model starts with a capital letter. Existing variables follow the 'snake_case' style, which has been updated.

  2. Similar changes are introduced in 'zlibfunc.cpp'. This includes renaming the variables in the 'SyncRun' function.

  3. Test cases are also modified in 'wasmedge_zlib.cpp'. The same naming convention changes are implemented.

Potential Problems:

  1. The changes are simple renaming of variables and do not seem to cause any functional or logical issues in the code as long as the rename operation is properly made. Although this is a huge change affecting the entire codebase, due to the consistency of changes, it forms a low-risk update.

  2. However, this may potentially affect other developers who have based their work on the previous naming convention. This renaming might cause merge conflicts for other branches which are based on older naming convention.

  3. Another consideration is of code review. Renaming a large number of variables in a single commit makes it more difficult to review other functional changes. It would generally be better to separate large refactorings like this into their own commits / pull requests.

Recommendation for moving forward:

The changes should be thoroughly reviewed to ensure consistency across all updated files. Developers working on this codebase should be notified about these changes to ensure a smooth transition. If possible, separating such large-scale renaming into a different commit or PR would improve code review efficiency in the future.

Commit b7c6368b2da387de95a0f748a2861702a15fcc51

The patch addresses three major areas:

  1. Functionality Enhancement:

It appears that the contributor is adding more support for the Zlib functions in the plugin. This includes all essential functionality needed for working with Zlib including initialization, decompression, and compression of data, managing GZ (Gzip) files, managing zlib stream, resetting zlib state, handling buffer, managing dictionary, tuning deflate’s internal compression parameters and more.

  1. Code Refactoring:

The contributor has reformatted some commented out sections for better readability, notably within the zlib module. The refactoring also includes renaming a few functions for consistency.

  1. Error Handling:

The contributor incorporated error handling code within the plugin, when a value is not found in the map.

However, certain potential issues to address:

  1. No Test Cases: There is no mention of any changes to test files, which could potentially mean either the changes are not tested or the testing is done manually.

  2. Unexplained Code Commenting: The developer uncommented blocks of code that were previously commented out without any explanation as to why they were commented out in the first place or why they are now being included.

  3. Potential Dead Code: The code that was commented out and now is uncommented speculation of being dead code, implying it may be redundant or no longer required.

  4. Code Complexity: With this update, the code complexity is increased by adding up several functions, which can make future debugging and maintaining the plugin more challenging.

  5. Lack of Context: Without an understanding of the wider codebase or specifications, I can’t determine whether these particular additions are necessary or helpful.

Recommendations for future pull requests:

  1. Including context and reasoning for changes, especially when uncommenting previously excluded blocks of code.

  2. Providing updated test cases to assist reviewers in confirming the changes work as expected and don't break anything else.

  3. Always strive for simplicity. Complex code has a higher chance of bugs and is harder to maintain.

Commit 692e6ad387444cdd4082dd6f0f906551b95dd848

This patch pertains to the code for the WasmEdge Zlib Plugin, which lives within a C++ test file (wasmedge_zlib.cpp).

Key Changes:

  1. The check expected the number of exported functions to be 6; now, it expects 74.
  2. Multiple function presence checks (using findFuncExports) have been added, implying that the plugin now supports many more operations from the Zlib library.
  3. Some pre-existing checks, like "deflateInit_" and "inflateInit_", remain unchanged.

The patch creator, Saikat Dey, seems to be expanding the plugin's capacity by adding support for more Zlib functions, which is portrayed by the numerous added EXPECT_NE lines that check for these function exports.

Potential Issues:

  1. Too many function presence checks: Numerous lines are used to manually verify all 74 Zlib functions. Implementing a more dynamic approach for conducting these verifications could make the code more maintainable and cleaner.

  2. No new test cases: Along with increasing the functionality to handle more methods, appropriate test cases should be added to ensure the methods work as expected. Test cases are an essential component of software development to maintain code quality and catch various edge cases.

  3. Error handling: There is no indication of what will happen if a specific function is not present. If one of these functions fails to export correctly, the program may crash or behave unexpectedly. It would be beneficial to have appropriate error management set in place.

  4. Resource management: The code creates a Zlib module and deletes it after the checks. If any of the checks fail and an early return or exception trigger, it could lead to resources not being freed correctly. Use of a unique_ptr, shared_ptr, or other smart pointer/R.A.I.I techniques can resolve this potential issue.

  5. Scope of review: This patch only shows changes to a test file. It does not cover any changes to the actual implementation, so we cannot verify if all the newly introduced functions have been effectively implemented.

Commit 71c819220fd4f462bf3e2a23d520d162df1692d6

The pull request introduces changes to integrate a new plugin, zlib, in the WasmEdge project. Key modifications include:

  1. build_options: An option for zlib plugin -DWASMEDGE_PLUGIN_ZLIB=ON has been added in multiple places to enable the build process to include the zlib plugin.

  2. tar_names and output_bins: The tar_names and output_bins variables in various sections of the release.yml file are updated to include the wasmedge_zlib and the binary file libwasmedgePluginWasmEdgeZlib.so respectively.

  3. Upload Steps: New upload steps are added to the Github Actions workflow for the zlib plugin, which involve renaming the plugin packages with the corresponding version and platform details and then uploading them to Github releases.

There are a few potential issues to consider:

  1. Dependent Software: The zlib plugin may rely on additional libraries or software. If these dependencies are not properly installed or configured, issues could arise during the build or function of the plugin.

  2. Compatibility: Ensure that the new zlib plugin is compatible with the existing plugins and doesn't cause conflicts.

  3. Error Handling: If there are any exceptions or errors within the zlib plugin, they need to be properly caught and handled.

  4. Testing: New tests may need to be developed to specifically test the zlib plugin functionality.

To adequately address these issues, ensure dependencies are investigated and appropriately managed, error handling is robust and consistent with existing standards, and thorough testing is conducted to maintain system integrity.

Commit df4314f5e13fe676fb727bb8f0d01a591d7fec04

This patch primarily focuses on the addition of two functions, namely deflatesetheader and inflategetheader to the WasmEdge Zlib environment.

In zlibenv.h, a new struct GZStore is declared with two fields, 'WasmGZHeaderOffset', a unique pointer, and 'HostGZHeader, a unique pointer representing the gzip header. Also, an unordered map GZHeaderMap is added to link the gzip header to the ZStream.

In zlibfunc.cpp, these two functions are defined. Within both functions, it first checks if the ZStream is present; if not, it returns a HostFuncError. If it does exist, a gzip header is initialized and stored into the GZHeaderMap with the ZStreamPtr as the key. The deflateSetHeader or inflateGetHeader function is then called, passing in the ZStream and the gzip header. If the return status is not OK, the item is removed from GZHeaderMap.

In zlibfunc.h, new classes are defined for the aforementioned two functions, each having a constructor and a method named body.

Potential pitfalls and areas to review further:

  1. Management and removal of items from the GZHeaderMap need to be carefully handled to avoid memory leaks.
  2. Error handling: In the current implementation, if the deflateSetHeader or inflateGetHeader call is unsuccessful, the entry just inserted into the map is removed. Depending on what the deflateSetHeader or inflateGetHeader does, there might be further cleanup necessary.
  3. The code could use comments to further explain what's being done, especially in regard to error handling.
  4. The thread-safety of operations on GZHeaderMap should be verified.
  5. Current implementation doesn't handle any ZStreamPtr collisions on the GZHeaderMap. This should be handled to prevent possible overwriting of data.
  6. The newly defined classes should be validated to determine how they interact with the rest of the system.

Commit 03a15ead8ba176211e504f73f7ec1b3fd943ed78

In this patch, there are several changes made, but they concern one major modification, reflected in many different places in the code of zlibfunc.cpp file. The return type of the SyncRun function has been changed from int32_t, a basic integer type, to Expect<int32_t>, which is probably a template class that wraps around an integer but also provides potential exception handling or error indication functionalities. More specifically, any function return result that uses the SyncRun function now stores the result into auto ZRes instead of const int32_t ZRes, making use of auto keyword to automatically deduce the type.

In terms of potential problems, there are no obvious syntax or logic errors in the committed changes. However, whether or not this change is suitable depends on a few things:

  1. Compatibility: The replacement of int32_t with Expect<int32_t> should be compatible with all the other parts of the code which interact with the result. If any part of the code is not expecting this change, it may cause runtime problems.
  2. Error handling: This patch can be seen as an enhancement if the Expect<int32_t> indeed provides better error handling capabilities. However, there might be an issue if the code changes are not fully tested and may throw unexpected exceptions or errors.
  3. Performance: Introduction of Expect<int32_t> could have a performance impact, positive or negative, depending on the implementation of Expect. If its overhead is non-trivial, using it instead of primitive types could slow down the execution.

Therefore, thorough testing, code review, and performance evaluation are recommended before this patch can be safely merged.

Commit a49a4ab5f894cc180e80114bf4864f31aedf5e18

This patch is for the zlibfunc.cpp file within the wasmedge_zlib plugin. It contains major refactorings and changes including 164 insertions and 302 deletions.

Key changes:

  1. The function SyncRun has been refactored to now take two new parameters: WasmEdgeZlibEnvironment &Env and uint32_t ZStreamPtr, replacing the original z_stream *HostZStream. As a result, all the calls to SyncRun function have been modified accordingly.
  2. The handling of Unexpect(ErrCode::Value::HostFuncError) has been changed to run every time memory instance is null.
  3. Introduced validation check for Memory Instance and ZStreamPtr in the Environment map. If they are not found in the map, it results in a Host Function Error.
  4. The code for the Connstexpr have been moved outside of the SyncRun function.
  5. The error handling strategy has been modified from placing an object in a map, calling a function, and then removing the object from the map if an error is detected, to placing an object in the map only after successful function execution.
  6. Two variables PreComputeNextIn and PreComputeNextOut have been introduced to handle the computation of next_in and next_out pointers.

Potential Problems:

  1. Given the refactorings of the SyncRun function, the updates to function calls may lead to potential issues if previously expected behaviour has changed.
  2. Implementing the Host Function Error before validating the ZStreamPtr existence in the Environment map might cause abrupt exit in case of a null Memory Instance, causing difficulty in debugging.
  3. Chances of Memory leaks can be there due to extensive use of pointers. Memory ownership and lifetime are not clearly defined.

Commit 20011d07bbed96b9f9f3aca66695ef86a7485c35

The key changes include:

  1. This changeset modifies the zlibfunc.cpp file in the wasmedge_zlib plugin directory.
  2. Additional local variable HostGZHeaderPtr is added to hold the gz_header pointer. This is done immediately after the creation of the auto HostGZHeader = std::make_unique<gz_header>(); This captures the original pointer before it gets moved.
  3. The developer has replaced the unique pointer's .get() method call with the stored raw pointer in the deflateSetHeader() and inflateGetHeader() function calls.
  4. The change looks like a preventative measure against a potential problem where a unique_ptr object's internal pointer (HostGZHeader.get()) might have led to issues since it was moved later.

Potential Problems:

  1. Misuse of Smart Pointers: This change might not maintain the smart pointer's benefits completely. Smart pointers like std::unique_ptr are meant to manage the memory lifetimes automatically, which might not happen entirely in this case.
  2. deflateSetHeader and inflateGetHeader functions rely on a raw pointer (HostGZHeaderPtr). If these functions somehow deallocate the memory or it gets deallocated elsewhere, we might run into problems as the raw pointer does not have ownership.
  3. Thread Safety: One more potential concern would be thread safety. If these functions are used concurrently in multi-threaded scenario, this could lead to race condition or undefined behavior.

Commit 1d0d852c224b71c772c62974d0aab80ac52b9976

This commit, labelled "Added GZHeader Sync steps in SyncRun", introduces a substantial amount of changes in the 'plugins/wasmedge_zlib/zlibfunc.cpp' file entirely contained in the 'SyncRun' function.

Key Changes:

  1. Added a new search for ZStreamPtr in the GZHeaderMap in the WasmEdgeZlibEnvironment.
  2. If the GZHeader entry is found, it retrieves (syncs) the Module and Host GZHeader data. This information includes Text, Time, Xflags, OS, Extra, Extra Length, Extra Max, Name, Name Max, Comment, Comment Max, HCRC, and Done. The pointers for Extra, Name, and Comment are stored in PreComputeExtra, PreComputeName, and PreComputeComment for later use.
  3. The GZHeader information is then used in the 'Callback' function.
  4. After the callback, the program once again checks if the GZHeader entry exists.
  5. If it does, it syncs the changes from the callback function from the Host GZHeader data back to the Module GZHeader. This covers the same fields as the earlier sync. The Extra, Name, and Comment pointers are adjusted according to their initial PreCompute values.

Potential Problems:

  1. There doesn't appear to be any null checking or error handling when the 'getPointer' function is called. If getPointer fails to retrieve a valid pointer, it could potentially crash the program.
  2. To minimize any potential issues, these 'getPointer' calls should be safeguarded with error-handling routines to prevent possible program crashes.
  3. There are also multiple places where the programmer directly modifies pointers. There is an inherent risk of segfaults, buffer overflows, or other memory errors when dealing with raw pointers.
  4. There is a possibility that the gzHeader sync could be attempted even if the initial retrieval of this object isn't successful. It could be helpful if the author confirms that the initial and final sync steps should both be performed even if one fails.

Commit f37a0654feea7d4e79e5bea07a9cedccbf13dfcb

The patch adds two new methods to the "WasmEdgeZlibModule" class named "deflateSetHeader" and "inflateGetHeader". The method "deflateSetHeader" is added to the deflate set of methods which are likely dealing with compression, while "inflateGetHeader" is added to the inflate set of methods that are assumed to be about decompression.

  1. Key Changes:

    • The method deflateSetHeader appears to be used for configuring the header information for data that's to be compressed.
    • Similarly, the method inflateGetHeader is registered and appears to be for retrieving the header information from the decompressed data.
  2. Potential Problems:

    • On its own, this patch doesn't seem to exhibit any issues. However, it is only adding the method definitions to a "ModuleInstance" class. Whether these methods are implemented correctly can't be determined from this patch.
    • Also, this patch assumes the classes WasmEdgeZlibDeflateSetHeader and WasmEdgeZlibInflateGetHeader are implemented. If they are not, this would cause a compilation error.
    • Lastly, there are no related test cases included in this patch. Testing is important to ensure that newly added functions work correctly.

Final Note: It would be important to ensure there are corresponding test cases and functional implementations to these method definitions.

Commit 867cb647fbd9f3d17af80c51e50cd70f12dfdf69

Summary:
The commit by Saikat Dey has added two new assertions in the WasmEdgeZlibTest, specifically for the 'deflateSetHeader' and 'inflateGetHeader' functions. The tests check whether these zlib functions are correctly exported by WasmEdge.

Potential Problems:

  1. The changes appear to be simple tests to ensure specific functions are exported, therefore, not prone to many errors. However, it would be assuming these additions are taking place in an automated testing environment and the said environment should be capable of correctly displaying the results if these tests fail.

  2. The patch assumes the functions 'deflateSetHeader' and 'inflateGetHeader' exist and are meant to be exported, if they aren't available in the codebase or aren't meant to be exported, the test would fail. It suggests potential dependencies that aren’t clearly outlined.

  3. There's no indication of previous testing or the expected outcome on failing. This is important to know if there is a downstream reliance on the functionality intended by these exports that would break on failing.

It would be wise to ensure that the overall test process covers common potential pitfalls and that these particular functions are properly implemented. It would also be helpful if the patch included some comments about the intent of the change.

Commit 4e431c83cfe1f562d20f696ebd0ad2ef0fa1c806

Summary of Changes:
This pull request by Saikat Dey changes a single test condition in the test file for the plugin "wasmedge_zlib". Specifically, the expected number of exported functions from the Zlib module has been modified from 74 to 76.

Potential Issues:
Without sufficient context, the reasons for this change aren't clear - the pull request may have neglected to also include changes to the codebase that add the 2 new functions, leading to failed tests. Another concern is that it's not clear if this change corresponds to a specification change or not. If the test was previously passing, then something must have modified the function exports of ZlibMod.

Important to note the test author should ensure that the new functions are idle, and do not break existing functionality. It would be beneficial if the PR author could provide a list of the 76 functions that should be present after the changes, which would enable easier debugging if needed. Additionally, the context of how these extra functions were added or why they are necessary to the Zlib module is not given here.

Commit e03337dcfe750d6bd2bd0cb043f76ceec59691a7

The patch modifies WasmEdgeZlibDeflateCopy::body and WasmEdgeZlibInflateCopy::body methods in zlibfunc.cpp within the plugin wasmedge_zlib.

Here are the summarised changes:

  1. The SyncRun function call has been altered in both methods. Previously, the function was called but the return value wasn't checked/store. The check is now added to enhance the error handling.

  2. A check has been added to see whether the SyncRun function returns a value; if not (has_value() == false), the function immediately returns Res, which will be std::nullopt or a similar non-value indicating error.

  3. The value returned by SyncRun is stored in Res for the subsequent check.

Potential issues:

  1. Since the behavior of the SyncRun function is not provided, we can't deduce how the change influences the system. If previously, a failure in SyncRun was expected and handled differently, the new early return could break the program flow.

  2. The returned value Res isn't checked for any specific error code that might provide more information about why SyncRun failed. This might be necessary for debugging or recovery purposes.

  3. The inserted error check is not done for all instances of SyncRun call, such as the second call in both modified functions. This unequal treatment could be intentional, but it might also be an oversight that could lead to unidentified problems in case of failures in other places of the code.

  4. If the SyncRun call was not expected to fail under normal circumstances and this check is added just as a preventive measure, the addition of this check might unnecessarily complicate the code.

  5. Without knowing the intent of this patch or seeing related discussion or an issue tracker, it is hard to know if this is the most appropriate way to implement this feature, bug fix or what kind of implications it has on other parts of the project.

Commit bda8761bf62ab4f5c543188b90d906a9f9061f87

Summary

These changes consist of code clean-up, removal of unused comments, and change of usage for named fields in the zlib plugin for wasmedge. The code changes mostly have to do with how elements are added to the ZStreamMap and GZHeaderMap.

There are also many old class declarations being removed/commented out from the file zlibfunc.h. Lastly, the wasmedge_zlib.cpp component has a number of identifier renaming and changed comments.

Findings

  1. The user has stopped using structured bindings for adding elements to ZStreamMap and GZHeaderMap, replacing them with .second on the result of the emplace() call. IMPORTANT: This changes how the It variable is used, as it was previously a pair of an iterator and a boolean, and now is simply a boolean, indicating whether emplace operation was successful. So, references to this variable must be revisited to reflect this change.

  2. All changes in zlibfunc.h consist of the removal of class declarations that appear to have been previously commented out.

  3. In wasmedge_zlib.cpp, the change mainly consists of the renaming of some local variables: wasm_hp to WasmHP, wasm_data to WasmData, wasm_zlib_version to WasmZlibVersion, wasm_z_stream to ModuleZStream, wasm_compressed_data to WasmCompressedData, and wasm_decompressed_data to WasmDecompressedData. Also, the variable __deflateInit_, __deflate, __deflateEnd, __inflateInit_, __inflate, and __inflateEnd were renamed into DeflateInit_, Deflate, DeflateEnd, InflateInit_, Inflate, and InflateEnd respectively.

  4. Deleted some lines that included commented out code.

Possible Issues

  1. Code Reviewers need to make sure that changing from structured bindings (custom name pair) to a simple boolean for variable It does not have unintended side effects in the rest of the code.

  2. Renamed identifiers need to be checked across the whole project to ensure that they have been updated everywhere they are referenced. The search should not only be restricted to the current diff.

  3. Removal of old, unused code blocks is generally safe unless there are any plans to re-enable or re-use them in the future. The reviewers should agree that the removed code will not be needed.

  4. It's not visible in the current diff, but other areas of the code might still be using the old, unused names. Areas not included in the diff need checks for the renamed identifiers.

  5. The test routine appears to be slightly changed but the exact intentions and impacts are unclear just from this diff. Further in-depth would be required for this specific section.

Commit 2079bfee4ffef35752177c86d025240c92a04088

This pull request primarily introduces additional error logging into the Zlib Plugin, particularly on error return segments in various functions. Here are the primary changes made:

  1. Each error-triggered return segment has been updated with a logging line that provides context on the type of error encountered along with relevant information for debugging. Looking at the code patch, nearly all error situations have been covered with a new log line. This is likely intended to aid in debugging and identifying improper handling of these errors upstream.

The code changes seem generally good as they improve traceability and debugging, however, there are a couple of aspects that need reviewing:

  • Standardization and Consistency: The error messages vary across functions, some functions give quite detailed logging information, while others are a bit vague. You could consider using a standardized format for all error messages for consistency.

  • Performance Impact: The pull request increases error log generation substantially. This may have potential performance implications, especially for scenarios where the log level is set to a detailed level in a production environment. You could look into optimizing how these logs are created or even consider making such detailed logging configurable.

  • Code duplication: The same error message is repeated in several places (e.g., 'Frame.getMemoryByIndex(0) returned nullptr'.) It might be a good idea to use a constant or a function to avoid repeating the same error strings, which would also make it easier to change these messages later if needed.

Finally, it is always a good idea to add tests when code is changed (or ensure that existing tests cover these changes), but this patch does not include any tests. If tests have not been added elsewhere, you should recommend adding unit tests to ensure the errors are logged correctly.

Commit 4e546be8726b5750933c775c7d00e5694d2fe2ed

The key changes in this patch revolve around improving the debug experience during error cases in the WasmEdge-Zlib plugin. The primary change involves including function instance names in the log error messages, thereby providing clear context when errors occur.

Additions to the code include:

  1. The introduction of the function instance name Msg as the first argument to the SyncRun function.
  2. Changes to the spdlog::error calls within the SyncRun function to insert the function instance name Msg into the log message along with the error statement.
  3. Modifications to all calls to SyncRun throughout the code to include the function instance name as the first argument.

Although this patch is comprehensive and improves the code quality, certain issues may arise:

  1. This change assumes that all calls to SyncRun from its various uses will include the function instance name. If any future uses of SyncRun do not properly include this argument, this could lead to unclear or misleading error messages.
  2. It should be noted that these changes will slightly increase the memory usage and computational overhead due to the additional argument and string formatting, which could be a concern in resource-constrained environments.
  3. Additionally, the current code changes don't seem to include any updated unit tests verifying the new error messages, which is an important part of validating such changes.
  4. There's also no null or error handling for the new Msg argument, which could become a source of problems if it's not provided or if an invalid argument is passed in the future.

Commit 1ef077e2111bf5fc7860d9b3ac227a88046e47e4

The Pull Request titled "[Plugin] zlib [LFX 2023 Term 2]" includes changes made to the 'zlibfunc.cpp' file within the 'plugins/wasmedge_zlib' directory. The author of the PR is 'Saikat Dey' and the commit message states that a space was added between the error message tag and the message.

The changes throughout the file are consistent with the commit message. The author introduced a whitespace in the argument tag of the 'spdlog::error' function which logs the error messages.

Key Changes:

  1. Addition of a space in the log tag: In each 'spdlog::error' call in the 'zlibfunc.cpp' file, a whitespace has been inserted after the closing square bracket ']' of the log tag.

Potential Problems:

  1. Logging conventions: The whitespace added could potentially interrupt conventions for logging error messages if other logs may not have this space, leading to inconsistency.
  2. Change Impact: Given the sheer number of lines changed, it's important to note that if there are any features or code that might dependent on the existing error log structure, this change can disrupt those features.

Commit 84a54ee1506193fad4d437a815f7800d5606e2af

The commit titled "[PATCH 94/98] Added info to error msg if the error is caught inside SyncRun." makes changes to the zlibfunc.cpp file in plugins/wasmedge_zlib.

Key Changes:

  1. Enhanced the logs to include the function name ('SyncRun') in the error messages which gets triggered when an error is caught inside the SyncRun function. This is made on two occasions; one when getMemoryByIndex(0) returns nullptr and the other when an invalid 'ZStreamPtr' received.

Potential Problems:
There doesn't seem to be any major problems in this specific patch. However, adding the function name in error logs doesn't necessarily improve the overall debugging process unless the function name ('SyncRun') is unique and only used in this context. If not, it might be better to add additional information, such as the arguments or variables related to the error message. Furthermore, if this type of logging convention is not followed throughout the project, it may lead to inconsistency in log analysis.

Commit 4884578b975723f22b19f528415dc0b730b8398e

The pull request titled [Plugin] zlib [LFX 2023 Term 2] by Saikat Dey removes the usage of a temporary ZRes variable where it is deemed unnecessary in the zlibfunc.cpp file. This change affects around 290 lines of code, leading to a net reduction of 94 lines (98 insertions and 192 deletions).

The key change in these modifications is that the functions no longer store the result of SyncRun in a ZRes variable before returning it. Instead, the return of the SyncRun function is now directly returned in all function calls. This streamlining could theoretically make the code perform slightly faster and be more readable, as it eliminates an unnecessary variable assignment.

Potential problems with this include:

  • If any of these methods are intended to perform any more operations on the ZRes value before returning in the future, this direct return approach could complicate those later modifications.
  • There isn't any apparent error checking after the SyncRun call before the function return. If SyncRun fails or returns an unexpected result, this erroneous result will be directly returned and may not be handled correctly by the calling function.
  • These changes do not seem to contribute to the zlib plugin's functionality; they primarily adjust the code style. Depending on the project's rules and convention, such re-factorings may not be encouraged if they don't bring clear benefits or address specific issues.

Overall, these changes seem to generally follow good coding practices and shouldn't introduce any bugs if all the affected functions are working correctly. However, thorough testing would be recommended before merging this pull request.

Commit 1d0ac2336eb2489473e93b9bba8cc0a29a64657f

Summary of Changes:

The changes in this patch primarily revolve around how gzFile_s, which is an opaque structure in the zlib plugin, interacts with the calling module (WasmEdge). Rather than passing the raw gzFile_s pointer to the module (which it shouldn't try to access directly), this patch introduces a placeholder number that functions as a pointer.

Here are the key changes:

  • In both WasmEdgeZlibGZOpen::body and WasmEdgeZlibGZDOpen::body functions, the way NewWasmGZFile is calculated has been altered. This now uses a combination of WasmGZFileStart and the current size of Env.GZFileMap.
  • It also uses static_cast to ensure that the type of the size (derived from Env.GZFileMap.size()) matches the type of WasmGZFileStart.
  • Following these changes, NewWasmGZFile is now returned by these two functions, instead of 0.

Potential Issues:

  1. The size of Env.GZFileMap might change between where NewWasmGZFile is calculated and where it's emplaced in Env.GZFileMap. This could potentially lead to incorrect code behavior, especially when concurrent operations are involved.

  2. By dealing mostly with the symptom (not exposing internal details to users) rather than the cause (why the gzFile_s pointer was given in the first place), there may be missing pieces to this fix not considered. Notably, if any other code depends on the old behavior, i.e. getting '0', it will break.

  3. Care should be taken when using static_cast to avoid unexpected behavior caused by incorrect type-casting. The size of collections is usually given in size_t type, if the type of WasmGZFileStart is not compatible with this, there may be issues.

  4. There are no error checks for the gzopen and gzdopen calls. If these calls fail, an empty unique_ptr is inserted into the Env.GZFileMap.

A full review would likely entail checking the usage of WasmEdgeZlibGZOpen::body and WasmEdgeZlibGZDOpen::body elsewhere in the code, as well as the concurrent safety of Env.GZFileMap. Unit testing the changes would also be a best practice to consider.

Commit 49018cb086054158f99e7b30fe4f13b402757cd3

Summary of changes:

The author of the patch made a minor style change. He made updates to two files: zlibenv.h and zlibfunc.cpp contained in wasmedge_zlib plugin, replacing the name GZFile_s with GZFile. This is simply a more unambiguous and stylistically better name for a type in this context. These are the key changes in the files:

  1. zlibenv.h:

    • The type alias GZFile_s was replaced with GZFile.
    • Everywhere GZFile_s was used, GZFile was replaced.
  2. zlibfunc.cpp:

    • In the functions WasmEdgeZlibGZOpen::body and WasmEdgeZlibGZDOpen::body the variable declaration was changed, replacing GZFile_s with GZFile.

Potential problems:

The changes appear straightforward and I don't see any immediate potential issues as this is a simple renaming operation. However, a couple of things should be checked:

  • The new type name GZFile is not clashing with any global or existing types in the scope.
  • All places where GZFile_s was used in the codebase have indeed been changed to GZFile, not only in the updated files but across the codebase. If this is not the case, there could be compilation or linkage issues.

Recommendation:

Before this pull request is merged, I would suggest doing a global search across the codebase to make sure GZFile_s is not used elsewhere. If it is used elsewhere, those occurrences will also need to be refactored to GZFile to maintain consistency and to avoid compilation or linkage errors.

Commit 8846a16981a272cf119c3e4167caba0a29ea0730

This commit titled "[PATCH 98/98] Removed usage of decltype, due to gcc [error: type qualifiers ignored on cast result type]" introduces changes to the file zlibfunc.cpp in the plugins/wasmedge_zlib directory.

Key Changes:

  1. The usage of the decltype keyword was removed from two operations where it was used in static casting. This was due to a gcc error message stating "type qualifiers ignored on cast result type". The developer notes that using remove_cv to suppress this error would create code bloat, which is undesirable.
  2. The changes are in WasmEdgeZlibGZOpen::body and WasmEdgeZlibGZDOpen::body methods. The decltype keyword while calculating NewWasmGZFile is removed.

Potential Problems:

  1. These changes seem to be straightforward and are a code clean-up operation targeted at removing gcc build errors. However, the use of decltype can be important if WasmGZFileStart or Env.GZFileMap.size() ever changes type. This change assumes they will always produce a type compatible with the "+" operator and the result will make sense for NewWasmGZFile.
  2. If the types change in the future, this could potentially lead to hard-to-find bugs or incorrect behaviors as the type safety check previously provided by decltype has been removed. The testing and validation should be rigorous enough to capture such potential issues.

@github-actions github-actions bot added the c-Plugin An issue related to WasmEdge Plugin label Jun 5, 2023
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Merging #2562 (01ea793) into master (c688791) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2562      +/-   ##
==========================================
- Coverage   80.97%   80.96%   -0.01%     
==========================================
  Files         150      150              
  Lines       21618    21618              
  Branches     4350     4350              
==========================================
- Hits        17505    17504       -1     
- Misses       2947     2949       +2     
+ Partials     1166     1165       -1     

see 3 files with indirect coverage changes

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

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
@github-actions github-actions bot added the c-CI label Jun 5, 2023
….yml .

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
…ib plugin source.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
…lib test executable.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
@github-actions github-actions bot added the c-Test An issue/PR to enhance the test suite label Jun 6, 2023
NeelDigonto and others added 5 commits June 6, 2023 09:17
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <57017288+notfathomless@users.noreply.github.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
@NeelDigonto
Copy link
Contributor Author

NeelDigonto commented Sep 9, 2023

@hydai This PR is ready from my side.
If have also added the deflateSetHeader & inflateGetHeader functions.

If there is anything more that I should take care of, let me know.

If everything checks out well, we can start with the merging process.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
test/plugins/wasmedge_zlib/wasmedge_zlib.cpp Outdated Show resolved Hide resolved
test/plugins/wasmedge_zlib/wasmedge_zlib.cpp Outdated Show resolved Hide resolved
test/plugins/wasmedge_zlib/wasmedge_zlib.cpp Outdated Show resolved Hide resolved
test/plugins/wasmedge_zlib/wasmedge_zlib.cpp Outdated Show resolved Hide resolved
plugins/wasmedge_zlib/zlibfunc.h Outdated Show resolved Hide resolved
plugins/wasmedge_zlib/zlibfunc.h Outdated Show resolved Hide resolved
plugins/wasmedge_zlib/zlibfunc.h Outdated Show resolved Hide resolved
plugins/wasmedge_zlib/zlibfunc.cpp Show resolved Hide resolved
plugins/wasmedge_zlib/zlibfunc.cpp Outdated Show resolved Hide resolved
plugins/wasmedge_zlib/zlibfunc.cpp Outdated Show resolved Hide resolved
…nvention fixes.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
…bug experience.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
@NeelDigonto
Copy link
Contributor Author

@hydai I have resolved all the changes requested.
Can you have a second look at it.

plugins/wasmedge_zlib/zlibenv.h Outdated Show resolved Hide resolved
plugins/wasmedge_zlib/zlibfunc.cpp Outdated Show resolved Hide resolved
NeelDigonto and others added 5 commits September 12, 2023 04:50
…cessary.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
…ointer to Host gzFile_s. We don't need to pass the raw pointer to Module, since gzFile_s is an opaque structure & the zlib app shouldn't try to access any of it's fields.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
… on cast result type]. Using remove_cv to supress, would create code bloat.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
@hydai hydai merged commit 27b3669 into WasmEdge:master Sep 15, 2023
51 checks passed
sarrah-basta pushed a commit to sarrah-basta/WasmEdge that referenced this pull request Oct 20, 2023
* Include wasmedge_zlib in plugins/CMakeLists.txt .

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Add Zlib Plugin as an option in root CMakeLists.txt .

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Add zlib-ng dependency through FetchContent.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added zlib build target for job 'build_ubuntu' in build-extensions.yml .

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added zlib build target for job 'build_manylinux' in build-extensions.yml .

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Init env & base headers, struct Wasm_z_stream & ZStreamMap

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added basic zlib function declarations.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added zlib env implementation & PluginDescriptor.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added zlib module implementation, and added Host Functions.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added zlib plugin WasmEdgeZlibDeflateInit_ implementation.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added zlib plugin WasmEdgeZlibInflateInit_ implementation.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added zlib plugin WasmEdgeZlibDeflate implementation.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added zlib plugin WasmEdgeZlibInflate implementation.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added zlib plugin WasmEdgeZlibDeflateEnd implementation.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added zlib plugin WasmEdgeZlibInflateEnd implementation.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Populated plugins/wasmedge_zlib/CMakeLists.txt to build & link the zlib plugin source.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Fix: Added ZLIB as link dep to wasmedge_zlib plugin.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added basic test for zlib plugin; WIP/TODO.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added test/plugins/wasmedge_zlib/CMakeLists.txt to build and create zlib test executable.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Include zlib plugin test sub directory in test/plugins/CMakeLists.txt.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Fixed naming convention by omitting namespace prefix from function name.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Fix case sensitivity in CMake of zlib library.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Refactor build-extensions.yml to be in-line with upstream master.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Fix typo in build-extensions.yml on zlib cmake option

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Minor type changes to better reflect the Zlib API.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added Deflate Inflate Integration Test

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Removed unnecessary nulling of wasm module's z_stream allocators | The host ignores them.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added static_assert on Wasm_z_stream struct size.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Start writing to the wasm heap from address 1 onwards, to nto collide with the semantic meaning of address/offset 0 as nullptr.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Fix: Correct Version Check and Stream Size Check Error code, and API behaviour, along with relevant test case changes.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added in-code documentation/pointer/reason on why we just compare only the first character of zlib version strings.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Remove unsafe usage of wasm MemStart(pointer from base{0}). Converted to offset based pointer calculation, based on pointer increment after zlib operation..

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added in-code documentation to every field in Wasm_z_stream.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Honor LLVM Naming Convention: Change 'Wasm_z_stream' struct name to 'WasmZStream'.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Honor LLVM Naming Convention: Change 'Wasm_z_stream' struct name to 'WasmZStream'.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Changed c style cast to static_cast.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Updated a few variable name to reflect LLVM standards.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Updated a few more variable name to reflect LLVM standards.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added a more comprehensive Host and Wasm ZStream Syncronization Func, and wrapped all zlib calls with it.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Change few comments.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Add Condition to only add ZStream to internal registry, if init succeeded.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Remove unnecessary zlib version check in wasmedge, delegated it to the zlib implementation.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added WasmEdgeZlibDeflateSetDictionary and WasmEdgeZlibDeflateGetDictionary.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added WasmEdgeZlibDeflateCopy with a comment on impl. ref.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added deflateReset

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added deflateParams

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added deflateTune & deflateBound.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added deflatePending & deflatePrime.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Defined the Wasm gz_header struct.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added few function impl of zlib.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added most of non gz functions.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added adler32, adler32_z, crc32, crc32_z.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Remove _v2 postfix

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Remove duplicate impl. of WasmEdgeZlibInflateSetDictionary.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Remove unused parameter 'Frame' from WasmEdgeZlibZlibCompilerFlags & WasmEdgeZlibCompressBound.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Correct few pointer value usage.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added GZFile support Env & implemented WasmEdgeZlibGZDOpen.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Implemented gzbuffer, gzsetparams & gzread.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added gzfread, gzwrite & gzfwrite.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added gzputs, gzgets, gzputc, gzgetc.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added gzflush & gzrewind

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Remove unused parameter 'Frame'.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added gzeof, gzdirect, gzclose, gzclose_r, gzclose_w.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added gzclearerr.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Change void* to unsigned char*

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added gzgetc & gzungetc.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Fix name Spell mistake.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Fixed no-return on a Expect<void>.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Fix move semantics related to unique_ptr.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added deflateInit2 & inflateInit2 & inflateBackInit2 & Refactor Part 1.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added gzopen | gzseek | gztell | gzoffset | adler32_combine | crc32_combine & Refactor Part 2

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added deflateInit2_ | inflateInit2_ | inflateBackInit_.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added gzgetc_

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added inflateSyncPoint | inflateUndermine | inflateValidate | inflateCodesUsed | inflateResetKeep | deflateResetKeep.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added draft WasmEdgeZlibGZVPrintf.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Removed unused Frame parameter.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Change all remaining naming convention to LLVM style.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added Host Func registration & Refactor Part 3.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Update function presence check to validate al 74 functions.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Update release.yml to include zlib.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added deflatesetheader & inflategetheader.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Change return type of SyncRun.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* SyncRun Func Design & Params Overhaul.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Remove access to moved unique_ptr.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added GZHeader Sync steps in SyncRun.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Registered deflateSetHeader | inflateGetHeader.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Updated Test to check for deflateSetHeader & inflateGetHeader.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Updated function count test to check for 76 functions.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Check if SyncRun fails even with no call to zlib API.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Removed old unused comments, revert usage of named fields & naming convention fixes.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added extensive error logging to Zlib Plugin.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added Function Instance Name, to SyncRun Error messages for better debug experience.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added a space between error msg tag & message.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Added info to error msg if the error is caught inside SyncRun.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Removed usage of a temporary ZRes variable wherever not absolutely necessary.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Fix Bug: Properly return a placeholder number to Module to act as a pointer to Host gzFile_s. We don't need to pass the raw pointer to Module, since gzFile_s is an opaque structure & the zlib app shouldn't try to access any of it's fields.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* [Style Change] Remove trailing _s from GZFile_s.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

* Removed usage of decltype, due to gcc [error: type qualifiers ignored on cast result type]. Using remove_cv to supress, would create code bloat.

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>

---------

Signed-off-by: Saikat Dey <saikatdey2100@gmail.com>
Signed-off-by: Sarrah Bastawala <sarrah.basta@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-CI c-Plugin An issue related to WasmEdge Plugin c-Test An issue/PR to enhance the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants