[Proposal] Add exception handling proposal on interpreter mode

[harry900831]

#2335

Note

We may now generated the wasm with exception handling by emcc -O1 -fwasm-exceptions a.cpp -o a.wasm and test it on the interpreter mode.

Example

#include <cstdio>
#include <stdexcept>

double divideNumbers(double num1, double num2) {
    if (num2 == 0) {
        throw std::invalid_argument("Error: Division by zero");
    }
    if (num1 < 0 || num2 < 0) {
        throw std::domain_error("Error: Negative number");
    }
    return num1 / num2;
}

void testDivideNumbers(double num1, double num2) {
    try {
        double result = divideNumbers(num1, num2);
        printf("The result is: %f\n", result);
    } catch (const std::invalid_argument& e) {
        printf("Caught an invalid_argument exception in testDivideNumbers: %s\n", e.what());
        throw; // re-throwing the exception
    } catch (const std::domain_error& e) {
        printf("Caught a domain_error exception in testDivideNumbers: %s\n", e.what());
        throw std::runtime_error("New runtime_error from testDivideNumbers");
    }
}

void anotherTest(double num1, double num2) {
    try {
        testDivideNumbers(num1, num2);
    } catch (const std::invalid_argument& e) {
        printf("Caught an invalid_argument exception in anotherTest: %s\n", e.what());
        throw std::out_of_range("New out_of_range from anotherTest");
    } catch (const std::runtime_error& e) {
        printf("Caught a runtime_error exception in anotherTest: %s\n", e.what());
    }
}

int main() {
    try {
	double num1 = 10.0;
	double num2 = 3.0;
        anotherTest(num1, num2);
    } catch (const std::out_of_range& e) {
        printf("Caught an out_of_range exception in main: %s\n", e.what());
    } catch (...) {
        printf("Caught an unknown exception in main.\n");
    }

    try {
	double num1 = -10.0;
	double num2 = 0.0;
        anotherTest(num1, num2);
    } catch (const std::out_of_range& e) {
        printf("Caught an out_of_range exception in main: %s\n", e.what());
    } catch (...) {
        printf("Caught an unknown exception in main.\n");
    }

    try {
	double num1 = -10.0;
	double num2 = 3.0;
        anotherTest(num1, num2);
    } catch (const std::out_of_range& e) {
        printf("Caught an out_of_range exception in main: %s\n", e.what());
    } catch (...) {
        printf("Caught an unknown exception in main.\n");
    }
    return 0;
}

Output:

The result is: 3.333333
Caught an invalid_argument exception in testDivideNumbers: Error: Division by zero
Caught an invalid_argument exception in anotherTest: Error: Division by zero
Caught an out_of_range exception in main: New out_of_range from anotherTest
Caught a domain_error exception in testDivideNumbers: Error: Negative number
Caught a runtime_error exception in anotherTest: New runtime_error from testDivideNumbers

However, if I replace printf by cout, the execution may crash. It seems that cout may contain several complicated exception handling block. I think we can merge this first and solve this issue in the future.

References

• Webassembly Specifications with exception handling
• Exception handling proposal

[juntao]

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

---

The GitHub Pull Request by Harry Chiang is an expansive update aiming at integrating and enhancing exception handling in the interpreter mode of the software. This proposal involves structural changes to the exception handling process, as well as the addition of a number of new classes, methods, and configurations related to this objective.

A principal concern is the significant increase in complexity introduced to the Instruction class, coupled with the coupling of TagTypes to FunctionTypes. Changes have been made to constructors and destructors to account for new fields like the JumpCatchList, making the memory management more intricate. The absence of a significant number of tests in areas of considerable modification presents risks, especially regarding exception scenarios, memory management and handling of edge cases, which can allow critical issues manifest undetected.

The changes in instructions handling, especially with JumpCatchList and removal of Tag, might impact other code sections, and thorough evaluation of the interdependency impact is required. The addition of new configuration options raises concerns about code repetition and handling unaccounted cases, which might negatively affect maintainability.

Certain changes lack documentation or clear rationale, like the rationale behind renaming to getTargetIndex(). Inconsistent usage of ASSERT and the lack of a clear error handling strategy may disrupt the flow of the test suite. Additionally, certain sections of code, like those for handling 'Tag' and 'Tag Type' elements, seem incomplete, as marked by TODO comments left by the developer, indicating rushed or incomplete code.

This PR is part of a broader set of changes, analysis in isolation might miss certain potential problems. Hence, it might be beneficial to review this change in the context of a broader system. It is highly recommended to increase the code test coverage, improve the documentation, and reduce complexity where possible. Additionally, the pull request could benefit from improved error messaging and memory management review, amongst others. All these steps would contribute towards a more robust and maintainable code.

Details

Commit a794a5194e8bd6ee053af9385e289856e378c0d3

Summary:
The patch submitted by Harry Chiang proposes to add an option for Exception Handling in the Wasm Runner of the Lib Driver Runtime tool. The patch checks if the ExceptionHandling value is true, and if so, adds it to the configuration proposals. This new option would be another feature alongside other proposals like Threads, MultiMemories, TailCall and ExtendedConst.

Identified Potential Problems:

1. Code duplication: The addition of the 'Exception Handling' proposal is repeated twice. It is not necessary to have this addition in the condition checking for 'PropAll.value()', since it logically already checks and adds this in the previous 'if' condition related to 'Opt.PropExceptionHandling.value()'. Reducing repetitions would make the code more maintainable.
2. Missing exception handling: Though the ExceptionHandling proposal is added, the code does not actually show how these exceptions will be handled. Some implementation details are missing or might be included in other patches not visible in this change.
3. Unclear timeout behavior: The code ends with a declaration of a Timeout variable, but there is no indication of how this timeout interacts with the rest of the code, and especially with the proposed exception handling.
4. Test Coverage: Without having the complete PR context, it's unclear whether accompanying tests for this change have been provided, these changes should ideally be verified with proper unit testing.
5. Lack of comments: There is no developer comments explaining the proposed changes, this can lead to difficulty in understanding and maintaining the code in the future.

Commit 8f182b1a4ab9feb7188bf7452142ead82cc86379

Based on the affected files and code, the patch primarily adds exception handling to the interpreter mode of the software. Notable changes include:

1. Addition of a new AstNodeAttribute, Sec_Tag for distinguishing the tag section in the Abstract Syntax Tree (AST).

2. Several structures and classes related to exceptions, such as TagType, Tag, TagSection, have been created in the Abstract Syntax Tree to handle the exceptions.

3. A lot of modifications to the Instruction class. New attributes and methods have been added to the Instruction class, substantially changing its structure.

4. Exception handling opcodes (Try, Catch, Throw, Rethrow, Delegate, and Catch_all) have been added to handle exceptions.

5. New methods to handle the flow of exception-like situations (runTryOp, runThrowOp, runRethrowOp) in executor.h.

This is a considerable change, making core additions to the script's interpretation process. While the pull request doesn't contain any apparent syntactical errors, there are several areas where problems may arise:

• This change introduces a considerable amount of complexity to the Instruction class. Too much complexity in a single class can lead to maintainability problems and can make future changes more difficult and error-prone.

• The patch added exception handling without removing any existing error handling. Depending on how the existing system works, this might result in conflicts or redundancy.

• Effective exception handling requires careful consideration of many edge cases. Without a thorough review and testing process, there is a significant risk that some situations could be handled incorrectly or not handled at all.

• There was a single TODO comment ("Need refactor") in the ProcessingInstruction class definition, indicating that the author themselves knew of potential improvements that could be made. Leaving TODOs in final code is not ideal and may indicate rushed or incomplete modifications.

Commit 97eb69f979ae87cea6b62af50b4c26d9d13e7620

Summary of the key changes:

1. Addition of exception handling support in the testing infrastructure: The reviewer made a proposed change to handle exceptions when testing the program in the interpreter mode.
2. Modification of External Type in Loader Test: The change in test/loader/descriptionTest.cpp updated the invalid external type as part of the 'LoadExportDesc' test.
3. Including exception handling tests in the build configuration: The change in test/spec/CMakeLists.txt included 'exception-handling' in the list of test suites.
4. Modification of Command ID Resolution: A new command, AssertException, has been introduced in test/spec/spectest.cpp. Its purpose is to handle cases where exceptions are to be asserted in the spec tests.
5. Addition of exception handling scenario in the test: New test scenarios have been introduced in test/spec/spectest.cpp that validates the exception handling mechanism during the execution of the modules.
6. Addition of AssertException in the CommandID class: The change in test/spec/spectest.h involved the addition of AssertException in the CommandID enum class.

Potential problems and concerns:

1. All the error scenarios during the exception handling have not been taken care of properly. Especially, the 'else' condition in ExceptionInvoke function does not handle cases where exceptions other than UncaughtException occur.
2. The Author mentioned a TODO for checking the expected exception type, which indicates that the code is incomplete, and this scenario has not been handled yet.
3. Certain command execution scenarios have not been accounted for. In the new AssertException block in the void SpecTest::run method, only "invoke" type actions are handled. Other types will hit the default EXPECT_TRUE(false) block, which may lead to unexpected results.
4. If any of these test cases fail, it may stop the function and fail the entire suite due to usage of the ASSERT macro. It would be more robust to use EXPECT notations for checking conditions, to ensure all test cases are executed irrespective of failures.

Commit bf8e5a160d15f5851d04a664f8e86c09d5a45609

Summary of the changes:

• The patch proposes numerous changes to structure and logic related to handling Tag and TagType. The primary change is the movement of aspects of the Tag class to a new TagType class. Another significant change is that TagTypes are now associated with FunctionTypes, which means the association size is now determined by the corresponding function's parameter list. The changes improve the use of the TagType architecture across multiple files.
• Direct changes have been made to 14 files (ast/description.h, ast/section.h, ast/type.h, common/enum.inc, loader/loader.h, runtime/instance/tag.h, executor/instantiate/import.cpp, executor/instantiate/tag.cpp, loader/ast/description.cpp, loader/ast/module.cpp, loader/ast/section.cpp, loader/ast/type.cpp, loader/loader.cpp, validator/validator.cpp).
• The patch includes the addition of 68 lines of code and the deletion of 41 lines of code.

Potential problems:

• Changes in the TagType may impact the interaction with FunctionType. The correct interaction between the two demands careful review.
• The patch introduces the 'setTagFunctionType' method, which connects Tags with their corresponding FunctionTypes. However, it is not clear the input vectors' element count is checked properly before they are used, potentially leading to out-of-range errors.
• The TagType class now includes a pointer to FunctionType, possibly leading to memory management issues.
• Updating the naming from Tag to TagType might cause compatibility issues with any other code dependent on the old naming system.
• The correctness of this change relies heavily on the successful execution of the 'setTagFunctionType' method. If this function fails to execute successfully or produces incorrect associations between TagTypes and FunctionTypes, the whole system could produce incorrect results.

Recommendations:

• Thorough testing must be done around the changes to the TypeTag and usage of FunctionType.
• Review the failure modes for 'setTagFunctionType'.
• Assess the implications of a TagType pointer to FunctionType and verify that memory management issues do not arise.
• Ensure compatibility with dependent code.
• Review and test the 'setTagFunctionType' method thoroughly.

Commit 28dba5d0b68b53017b7a8485dd3384d610aab22c

Summary of changes:

Harry Chiang has made various changes to the file include/runtime/stackmgr.h to add exception handling support to tail call instructions. The following are the key changes:

1. Two new members HPos and CPos have been added to the Frame struct to maintain the current positions in the handler and caught stack.

2. The Frame constructor has been modified to include HPos and CPos parameters and initialize the new members.

3. The pushFrame function has been updated to push the size of HandlerStack and CaughtStack to the respective locations in Frame in non-tail call situations.

4. In tail call situations, the function erases elements from the ValueStack, HandlerStack, and CaughtStack based on the information in the current frame. This is an attempt to clean up the current frame's context from these stacks when a tailcall is being made.

Potential problems:

1. The construction logic for the Frame struct has changed in the process, which might affect other parts of the system depending on it.

2. The ranges for erase in pushFrame on different Stacks (ValueStack, HandlerStack, CaughtStack) are calculated differently. This could potentially lead to an inconsistent state or bugs if the stacks are not managed carefully.

3. There's the possibility of an exception being thrown when trying to remove elements from a stack with insufficient elements. Exception safety should be thoroughly checked.

4. A test must be done to verify that the assumptions FrameStack.back().VPos >= FrameStack.back().Locals are always correct, especially in edge cases. If the assumption is incorrect, this may lead to unexpected errors.

5. The commit does not seem to include related test cases, which should be a part of any significant change. The absence of those might indicate untested or incorrectly functioning changes.

Overall, this commit seems to be a part of a broader set of changes, so analyzing it in isolation might not cover all of the potential problems. Further code review would be beneficial to examine the impact on the system as a whole.

Commit 4177263b849332a471d8f48c35673b23078fd3a9

This is a code formatting patch. The following are the key changes:

1. The indentation of the parameters of the setTagFunctionType method in both loader.h and loader.cpp files were adjusted to align correctly.

2. The formatting of the constructor in tag.h file was also changed to be spread across multiple lines for better readability.

3. In the loader.cpp file, the patch also adds a space between "auto" and "&TgType" to adhere with the code spacing convention.

There doesn't appear to be much potential for breaking functionality in this patch as it only does reformatting, so there are no logical changes. Nevertheless, developers need to ensure that consistent formatting rules and style guides are used project-wide to avoid such piecemeal formatting adjustments, which might disrupt the development flow and lead to clutter in version history.

All commits should be tested for compilation and functionality just to be sure, even if they are expected not to affect the functionality.

Commit 322621757d593d91e91c3a542b2ba57847c818b9

The pull request titled "[Proposal] Add exception handling proposal on interpreter mode" is proposing the removal of a Data Tag in the Instruction class and changing code logic to match this modification. The key information is:

1. Removed the Tag structure inside the Instruction class in instruction.h.
2. Removed getter methods getTagIdx()
3. Renamed getTagIdx() with getTargetIndex() where it is called in controlInstr.cpp, instruction.cpp, and formchecker.cpp.

The most important change is in the instruction header file, where the struct named Tag is removed, as well as its getter methods getTagIdx(). The references to getTagIdx() are replaced with getTargetIndex() on different files.

Potential issues might include:

1. If other unseen parts of the code utilise the Data Tag or the getter methods we might see functionality break or unexpected behaviour. Thorough testing and examination needs to be done to ensure removing these doesn't impact parts of the application.

2. The pull request does not contain any new tests, which makes it difficult to evaluate if removing the Tag or TagIdx will break other functionalities without some unintended consequences.

3. The renaming to getTargetIndex() isn't accompanied by any documentation indicating why this name was chosen or whether it accurately reflects the kind of data it's going to return, which can possibly lead to confusion later.

Commit c9228b05e8d6cf1c9a0d098dc51feccceb01fd3c

Summary of Key Changes:

1. Refactoring the Instruction class of ast/instruction.h by adding a JumpCatchList field to each instruction to handle exceptions. This list is initialized when an error is detected and added to within the setJumpCatchList method.

2. Modifying the memory management functionalities within different constructors and destructor for the Instruction class to accommodate newly introduced JumpCatchList.

3. Replacing the usages of TryBlockJumpCatch with the new JumpCatchList method and accordingly implementing the necessary getter and setter methods.

4. Modifying the blockstack data structure in instruction.cpp to hold the list of Catch inside each Try block.

5. Modifying controlInstr.cpp to handle the new JumpCatchList during execution.

Potential Problems:

1. Memory Management: With the introduction of the JumpCatchList, there are changes in the destructor, copy constructor, and move constructor of the Instruction class. If any of these operations are not handled correctly, it could lead to memory leaks or undefined behavior.

2. Compatibility: These modifications, especially in the Instruction class and controlInstr.cpp, are quite significant. They may cause compatibility issues with other sections of the codebase that depend on the original version of these methods or classes.

3. Exception Handling: The changes are directly related to exception handling mechanism. If not implemented correctly, it can lead to unhandled exceptions or incorrect handling of exceptions, ultimately causing the software to crash or behave unpredictably.

Suggestions:

1. Extensive testing must be carried out to ensure that the refactoring does not introduce memory leaks or undefined behaviors.

2. The developer needs to ensure that the impact on other parts of the codebase has been thoroughly assessed and required modifications have been made.

3. Adequate testing of exception scenarios is needed to ensure that the changes work as expected.

Commit e5aa96ae0804615c34d4e6ccdab30d7938bd4e76

The patch by developer 'Harry Chiang' introduces changes in description.cpp located in the directory lib/loader/ast.

Key Changes:

1. The developer has added a total of 11 new lines in two places. Both seem to check if the Exception Handling proposal is enabled or not.
2. In the function loadDesc(AST::ImportDesc &ImpDesc), an if statement has been added to check the presence of the proposal ExceptionHandling. If the proposal is not present, the new code calls the function logNeedProposal() followed by a return statement.
3. Similar changes have been made in the function loadDesc(AST::ExportDesc &ExpDesc) for the ExternalType::Tag case.

Potential Problems:

1. There doesn't seem to be a fallback mechanism if the proposal is absent, as logNeedProposal() is immediately followed by a return statement. The flow of the program might be abruptly stopped or interrupted in these cases.
2. There might be an issue if the function logNeedProposal() does not handle tuple types returned by FMgr.getLastOffset() and ASTNodeAttr::Module properly.
3. Since the review process cannot review the full functionality, there might be other side effects not visible from the changes shown.
4. There aren't any input or output validations, nor are there any exception handlers for the newly added code. The patch might introduce bugs if the ExceptionHandling proposal attribute is not handled appropriately elsewhere in the code.
5. The introduced patch does not seem to handle all case scenarios while dealing with ExternalType. The presence of ExceptionHandling proposal is only checked for Tag type, leaving other types not dealt with.
6. The patch does not include any unit tests for the added functionality which is essential for verifying the logic and robustness of the new code.

Please make sure the tests are written and all scenarios are considered. Also, handle all the possible error conditions.

Commit 9c7a6cf0172b6172a38c428a16064ff3fb3112f4

Changes:

• The pull request introduces exception handling with tag support into the WasmEdge API.
• This update primarily consists of the addition of the "Tag" concept to the WasmEdge API including support for Tag Types and Tag Instances.
• The proposal has led to 205 lines of code being added to the files "wasmedge.h" and "wasmedge.cpp" in the include/api/wasmedge and lib/api directories respectively.
• The edits made to the .h and .cpp files include adding new functions for supporting tag types and instances in the WasmEdge_ModuleInstanceContext class.
• These additions are not limited to creating, querying, and deleting tag types and instances, but also includeTag exports and imports, as well as getting various forms of tag types.

Potential problems:

• The pull request touches several parts of the API code, which may have a broad impact on the functionality, increasing the risk of new bugs being introduced.
• The functions for "tag" elements were added, but the corresponding unit test code to ensure their accuracy does not seem to be present. This could result in unforeseen errors.
• There have been several checks included to return null pointers in case of failures which might make it harder to debug issues. Proper error handling and messages would be more beneficial to diagnose problems.
• Based on the code patch, there might be potential multithreading issues to consider, as the code seems to involve shared resource handling, but it is not clear how or if these resources are protected.

Commit 09a7829097e616fd73ebf1065fd97c68b9a203cb

Key Changes:

1. Formatting of the code base: The developer has decided to run clang-format to reformat their code. Clang-format is a tool that automatically formats C/C++ code according to certain specified rules and conventions.

2. Code Refactor: The changes proposed in this commit deal mostly with spacing and indentation adjustments in wasmedge.h and instruction.h files. The spacings between the function parameters in function prototypes have been reformatted to align better with the start of the function name.

3. Removal of redundant code: In the instruction.h file, an instruction block, although not changing its functionality, has been reduced to a single line for clarity and readability.

Potential Problems:

1. Functional Changes: Although this commit doesn't introduce any functional changes, it may cause merge conflicts with other Pull Requests that are simultaneously introducing changes to the same lines in these files.

2. Readability: Such type of changes can impact code readability if all team members are not on board or don't follow the same conventions specified in the clang-format configuration. It's crucial that all team members have agreed upon a code style and are following it consistently.

3. History Tracking: Reformatting the entire codebase can also make tracking changes via git history harder since you'll see these formatting changes rather than actual logic changes in the git diffs.

Suggestion:

It would be best to integrate clang-format or any other formatters as part of the CI/CD pipeline so that the codebase follows the same style guide and avoids such massive reformatting PRs.

Commit 97efe019f1336700f24ca7a1adfc2603b82f1d34

This patch from Harry Chiang includes some changes to the comments and function names on an interface in the file include/ast/instruction.h.

The changes mainly revolve around three functions that act as getters and setters for flag states of instructions in a code interpreter. The changes are as follows:

1. The comment and function "IsLast for End instruction" are renamed to "IsTryLast for End instruction".
2. The comment and function "IsLast for End instruction" are renamed to "IsCatchLast for End instruction".
3. The comment and function "IsLast for End instruction" are renamed to "IsDelegate for Try instruction".

These changes imply the introduction of exception handling for Try, Catch, and Delegate instructions in the interpreted code.

Potential problems:

1. This is a small part of a larger pull request (12/28). I cannot see if the changes proposed here align well with the overall architectural and performance implications of the larger change set.
2. The renaming of functions can break existing usage of these functions in other parts of the code. The author should ensure that all usages of these functions are updated.
3. Wherever these getter/setter methods are used, the logic might need to be updated to handle these new instruction flags; this isn't just a simple renaming.
4. Without more background, it's unclear what the exact functionality of "TryLast", "CatchLast" and "Delegate" should be and if it correctly implemented. Further test plans or clarification should be requested.

Commit a1236e7a16e9f897f3008c490c5a54f8915682b8

The Pull Request is a patch on the test/spec/spectest.cpp file. The developer, Harry Chiang, is suggesting modifications on the "exception-handling" section of the TestsuiteProposals array.

Key Changes:

• The developer has added a "TailCall" proposal to the "exception-handling" section. Initially, it only contained "ExceptionHandling".

Potential Problems:

• If the "TailCall" proposal is not fully supported or has not been thoroughly tested in conjunction with "ExceptionHandling", this could lead to unexpected behaviour or system crashes.
• The issue of dependencies: if the "TailCall" proposal is dependent on other updates or proposals not included in this patch, it could cause problems.
• If the ordering of proposals in the array impacts the application behavior, this change might affect other parts of the code.

Recommendation: It would be helpful if the developer could provide more context on this change in the comments. Information about the purpose of this change and any necessary testing that has been conducted is useful. It would also help to know how this change impacts the rest of the project and if dependencies have been taken into account.

Commit ade6e5916af4e2ba701dc05ad7398818b9c74e4a

The pull request introduces exception handling to the interpreter mode. Here are the key changes:

1. An option for enabling a feature proposal for exception handling (PropExceptionHandling) has been added to DriverToolOptions in tool.h. This suggests that the software can now be configured to use exception handling if the proposal is enabled.

2. An exception invocation function has been adjusted in spectest.cpp to work with the new exception handling. Changes include parsing the variables from the invocation action differently and modifying the onInvoke call.

3. There also seems to be a change in the JSON library in use, possibly switching from rapidjson to simdjson. This is indicated by the change in code context around JSON objects fetch.

Potential problems that may arise include:

1. Compatibility: The change from rapidjson to simdjson might have a major impact on the rest of the software if not handled or managed properly.

2. Extensibility: Addition of new properties to the DriverToolOptions struct introduces more complexity to the system, thereby affecting maintenance in the future.

3. Testing: The if (auto Res = onInvoke...) condition seems to have a strange check included (EXPECT_NE(LineNumber, LineNumber);). This line will always fail as it is saying that it expects the line number not to be equal to the line number.

4. Exception handling: In the context of the AssertException command, a TODO comment notes that the expected exception type should be checked, yet this doesn't seem to have been implemented.

This pull request needs some changes in exception testing and properly handling the outlined potential problems before it could be accepted.

Commit d86639f0e8b19be9e6dfe75d6f4380f8cd0fc6de

Changes:
The primary change in this patch is the addition of a new driver tool option named "enable-exception-handling". This option was added to the DriverToolOptions structure.

Potential Problems:

1. Risk of Breaking Other Features: Without understanding the context of this change or knowing the impact in the larger project, I can't be certain, but there could potentially be a risk that this new option might accidentally break other aspects of the code.

2. Lack of Tests: There is no addition of new tests for this change. As a good practice, every new feature or modification should come with corresponding tests that ensure it behaves as expected and doesn't break the existing functionality.

3. Missing Documentation: There is no update in the documentation about this new option. Proper documentation is necessary to help other developers understand what this new option is used for.

Important Notes:
In general, while the code seems okay from a review perspective, it may be beneficial to also make sure that the change does not have unintended side effects, is properly tested, and well-documented.

Commit dcf092273bdcc6b55274d0dd92e220553aa84f71

Summary:
This patch changes two files lib/executor/helper.cpp and lib/validator/formchecker.cpp in the codebase. The developer is trying to fix exceptions handling in the interpreter mode.

Changes:

1. In lib/executor/helper.cpp, a new logging line is added to log 'UncaughtException' error with spdlog library when there's an issue with executing the code and properly handling exceptions.

2. In lib/validator/formchecker.cpp, there is a logic change in the FormChecker::checkInstr method that controls how the Try Block size is set. An if-else condition is added to check if "*D + 1" is less than the CtrlStack size; if true, the Try Block size is set to the sum of Locals.size() and CtrlStack[*D + 1].Height. Otherwise, the Try Block size is set to the sum of Locals.size() and ValStack.size().

Potential Problems:

1. The logging of 'UncaughtException' does not provide much information about the exception itself. It would be useful to include more details about what caused this exception.

2. In the formchecker file, there are potential risks of accessing out-of-bound indices with "*D + 1". If "*D + 1" is equal to CtrlStack.size(), this might lead to a buffer overflow issue since this index does not exist.

3. If CtrlStack.size() or ValStack.size() is large, the method setTryBlockVSize could potentially be setting a large Try Block size that could lead to performance issues or exceed the intended limit. It is uncertain whether the program correctly handles such cases.

In conclusion, it is advised to take buffer access and potential size growth into consideration when making changes related to exception handling. These areas introduce potential risks that could lead to bugs or performance issues.

Commit 630fa6f2998daa0aebff9b101d1c43d314664b57

The main changes proposed in this commit titled "[PATCH 17/28] Resolve windows CI issue" revolve around modifying the existing data types from size_t to uint32_t, primarily affecting four files:

• ast/instruction.h
• ast/type.h
• runtime/instance/tag.h
• runtime/stackmgr.h

More specifically, the changes are aimed at typecasting the size_t values to uint32_t in each location. Their objective is to resolve issues encountered with Windows CI.

Possible problem areas:

1. Type safety: Signedness of size_t depends on the data model of the compiler (unsigned in most systems), while uint32_t is always unsigned. There's a risk that when the codebase is used on a system where size_t is signed, the conversion to uint32_t could cause integer overflow or underflow, therefore loss of data.

2. Range: size_t is platform-dependent, usually 32-bit on a 32-bit platform, and 64-bit on a 64-bit platform, whereas uint32_t is always 32-bit regardless of the platform. If the size_t value exceeds the 32-bit range, the typecast to uint32_t could cause out-of-range issues, resulting in bugs that are difficult to track.

3. Compatibility: The changes seem to be targeted at resolving issues on Windows. But it might trigger other unexpected behaviors or compatibility issues on different platforms/compilers where the size of size_t is not the same as uint32_t.

Lastly, since the commit modifies central files within the codebase, comprehensive unit and integration testing should be carried out to understand the impact of changes. If this hasn't been done, it could be a potential area of concern.

Commit 7ec47a766742c35dbc4fa6f738e5fdfb39945fb5

Title: [Proposal] Add exception handling proposal on interpreter mode

The contributor, Harry Chiang, primarily made changes to the formchecker.cpp file. This file is located in the validator library of the software project.

The primary changes involved fixing type casting issues. There are situations where the .size() member function used on containers Locals and ValStack (presumably standard library containers) could return larger signed integer values. However, the functions setTryBlockVSize and setTryBlockParamNum expect an input of the 'uint32_t' type, which could potentially cause issues in conversions. If the number of Locals or ValStack exceeded the maximum value of 'uint32_t', it could cause unexpected behavior.

Three changes in the checkInstr function were:

1. In line 351, setTryBlockVSize now statically casts the Locals.size() to 'uint32_t' before it adds CtrlStack[*D + 1].Height.

2. In line 353, setTryBlockVSize similarly now statically casts the Locals.size() to 'uint32_t' before it adds ValStack.size().

3. In line 373, setTryBlockParamNum also forcefully casts T1.size() to 'uint32_t' before setting it.

This pull request seems like an attempt to fix CI issues related to type conversion on a Windows build system. However, an additional validation could be added to ensure that .size() doesn't exceed the maximum value of 'uint32_t'.

Commit 378fd143c0c7bc973b1ebdefc3969932ef1537e9

Summary of Changes:
The changes in this patch involve altering the calculation of TryBlockVSize in the FormChecker::checkInstr function. It appears the parenthesis has been moved from just encapsulating the Locals.size() to now wrap around both the Locals.size() + CtrlStack[*D + 1].Height or Locals.size() + ValStack.size(). Basically, the developer is changing the order of operations that the compiler will perform this calculation.

Potential Problems:

1. Bad Cast: The casting operation to uint32_t may lose information or precision if the result of the addition contains a value that can't be represented in uint32_t.
2. Math Errors: The parenthetical rearrangement of the addition operations could introduce unexpected results or mathematical errors if the order of operations isn't carefully considered.
3. Side Effects: Const Cast operation (const_cast<AST::Instruction &>(Instr)) can lead to undefined behavior if Instr is actually a const variable. It's generally not a good practice to remove the const qualifier from a const object.
4. Code Readability: Depending on the complexity of these calculations, this new arrangement might actually decrease overall code readability, making it harder for future developers to understand this piece of code.
5. Tests: Without proper tests that handle these changes, it’s impossible to tell if these changes will bring in bugs in the existing software.

Commit f3d1452869b6b24077e95739249cbdc44eefead4

The main changes in this patch are as follows:

• The developer has reformatted the code to enforce style consistency by using Clang-format. This promotes better code readability and maintenance.
• There are two calls to the method setTryBlockVSize in the FormChecker::checkInstr method. These calculations for the variable size of the try block have been broken up into multiple lines, likely for readability.
• When the index *D + 1 is less than CtrlStack.size(), Locals.size() + CtrlStack[*D + 1].Height is calculated and casted to uint32_t, and then passed as an argument to the setTryBlockVSize method.
• If the above condition is not met, Locals.size() + ValStack.size() is calculated, casted to uint32_t, and passed as an argument to the setTryBlockVSize method.

Potential problems and considerations:

• There are no changes to functional code, only the formatting has been impacted in this patch. So, functional issues are unlikely.

• As there has been no change to the functionality of the code, existing unit tests should continue to pass (assuming they exist). If no tests are in place, it would be good to add some to verify these calculations and function calls.

• The readability of the code has been improved, which is a good practice to follow. However, it's important that the entire team agrees on using tools like Clang-format to enforce style consistency.

• Lastly, ensure the cast to uint32_t provides the desired results and does not cause any precision loss or overflow issues, especially if Locals.size() + CtrlStack[*D + 1].Height or Locals.size() + ValStack.size() is larger than the maximum limit of uint32_t.

Commit 9f3e630d89beab2be8790ca825db31a68e6ede28

This patch from Harry Chiang is titled "Resolve Windows CI issue" and it involves the modification of a single file: 'lib/executor/instantiate/export.cpp'.

Key Change:

• The main change that Harry made to the file is an insertion of a 'break' command in the switch-case construct for handling 'ExternalType::Tag'.

Potential Issues:

1. No detailed explanation about why the 'break' was added: It looks like it's aimed at resolving a Windows Continuous Integration (CI) issue, but it's not clear why this change is necessary or what specific issue it solves.

2. No tests provided: As it's a change that apparently solves an issue, it would be anticipated that it comes with a test reproducing the issue before the fix, for future regression tests.

3. Likely to impact only a single platform: As the title of the PR suggests, this change intends to resolve a CI issue for Windows, so it might not impact other platforms (e.g., Linux, MacOS). This should be considered when adding changes targeting specific platforms.

4. Potential inconsistent state: If 'ExternalType::Tag' was supposed to fall-through the switch-case block, adding this 'break' might lead to an inconsistent state. It would be helpful if the developer could provide more information about why this potential fall-through has been prevented.

5. Depending on the specific logic of the case blocks above 'ExternalType::Tag', there might be a lack of exception handling or error checking code which poses a risk of silent failures.

Overall, more context around this change is needed from the developer before proceeding with the merge.

Commit 0e44fd58fefd0c7c917b38262d6a7160c5b1f3a9

This patch by Harry Chiang focuses on correcting a spelling error in the file formchecker.cpp, which is part of lib/validator in the software. Specifically, the nomeclature of the iteration variable in a for loop has been changed from 'it' to 'It', for maintaining code consistency and better readability.

Summary of changes:

• it has been changed to It at all places it occurs in a for-loop in the function checkInstr of the class FormCheck.

Potential problems:

1. No exception handling changes, functional code changes, or bug fixes are included in this particular patch. Therefore, the title of the PR is not suitable for the changes it includes. It only resolves a typo in the code in the formchecker.cpp.

2. The change is just a refactoring one which should not bring any behavioural changes to the functionality of the code. But still, it's important to ensure that these changes are validated since renaming variables can lead to unintended consequences if any references to the variable were missed to be updated.

As this is only a simple renaming of a variable, and this does not cause any side effects throughout the rest of the code, no other potential problems significantly stand out in this patch. However, regression testing to ensure no functionality is unexpectedly altered due to this change would still be a good practice.

Commit 90f826d3b906674181ca67bb2b104931fa9e0b70

The provided patch mainly comprises of two key changes.

1. It introduces the ExceptionHandling test mode to the Interpreter in TestsuiteProposal. This integrates exception handling in the interpreter's test mode, which could perhaps aid in handling unexpected scenarios or errors during the testing phase.

{"exception-handling"sv,
  {Proposal::ExceptionHandling, Proposal::TailCall},
  WasmEdge::SpecTest::TestMode::Interpreter},

2. The patch also modifies the existing reinterpret cast code by adding a reference sign (&) after the typecast. This operation is performed to avoid copying data while typecasting which optimizes the memory usage. This changes are applied for different typecasts in various places within the code. Here's an example:

// Before
const auto V = reinterpret_cast<const uint32x4_t&>(V64);
// After
const auto V = reinterpret_cast<const uint32x4_t &>(V64);

Potential Problem:

1. Although the changes in casting may optimize memory utilization, it could potentially introduce risk of data corruption or unexpected behavior, especially if the referred data is altered in any way, since the casted variables are referencing the same space in memory rather than creating a new copy.

For a comprehensive review, a thorough investigation of the referenced variables is necessary, their scope, lifespan, and whether they are being modified elsewhere, especially in multithreading context. The existing code testing and error handling should be reconfirmed to handle this change. Additionally, more tests should be added to validate the new test mode Interpreter for ExceptionHandling.

Commit d626285b034b53eada8b5f7e4cf54f5f78306a1e

The patch includes changes made to the stackmgr.h file in the runtime directory. Specifically, the changes are made in the pushHandler method of the StackManager class. The key changes are as follows:

• The emplace_back method of HandlerStack is modified to include a type cast for FrameStack.size(). The size of the FrameStack is explicitly cast to a uint32_t type before pushing into the HandlerStack.

There is no addition or deletion of any functionality, and only the function perimeters for the method are changed. The purpose of the change is likely to fix a type mismatch warning on windows.

Potential problems to be aware of:

1. Explicit casting can be dangerous if not used carefully as it can lead to incorrect results due to overflow or underflow. Here, size of FrameStack is cast to a uint32_t type, the original value must fit into uint32_t without any data loss. \
2. Ensuring that the FrameStack's size will never exceed the maximum value of a uint32_t (about 4 billion) is crucial. If this assumption is not held, it will result in incorrect behavior.
3. It is not clear from the patch why this change is needed for Windows, but not for other platforms. It might be an indication that data types are not being used consistently across the codebase.
4. The comment about the change or the reasoning behind it is missing in the commit message and in the code, which might make understanding and maintaining the codebase harder in the future.
5. There are no associated unit tests with this patch to validate these changes. This is a good practice to follow to ensure that the change doesn’t break any existing functionality or behave in unexpected ways. It is recommended that unit tests should be included with any code change.

Commit b28ab89f1daaabd8b001a23d3af0e0da60f27412

Summary of Changes:
The GitHub patch, [PATCH 26/28] by Harry Chiang, mainly makes changes to the file stackmgr.h in the runtime/include directory. Here are the main changes:

1. For the class StackManager, three handlers namely the HandlerStack, FrameStack, and CaughtStack have been typecast to uint32_t during emplacement in two methods – in an unidentified function and pushHandler function. Specifically, HandlerStack.size() - HOffset, CaughtStack.size() - COffset in the first method and FrameStack.size(), HandlerStack.size(), CaughtStack.size() in pushHandler function are cast to uint32_t.

Potential Problems:

1. Typecasting Obscurity: The difference in value after typecasting to uint32_t is not clear. This might hide some kind of overflow, underflow, or truncation. It's important to note that casting will remove the compiler warning, but will not address the underlying issue.

2. Loss of Data: If any of the .size() calculations result in a value that cannot be represented by a uint32_t, it will result in data loss. This could cause unexpected behavior in the stack management logic.

3. No Exception Handling: Despite the change proposing "Add exception handling", there's no visible exception handling logic in the given patch.

Suggestions:
The developer should add comments to make clear why typecasting was used, what values are expected, and how potential loss of data is handled or prevented. Ideally, the root cause of discrepancy leading to the warning should be addressed rather than just hiding the warning through casting.

Commit 03d8066e6ef700b5d1d03163ff259fc44f7404bc

Summary of Changes:
The patch essentially affects one method within the class StackManager in the file stackmgr.h. It modifies the pushHandler method by relocating the type-casting operation on the second parameter passed to the emplace_back function of the HandlerStack object. Rather than obtaining the size of ValueStack, deduct BlockParamNum and later cast the result to uint32_t in a single expression, now the deduction and casting are separated into two operations. The casting operation now only involves the ValueStack.size() - BlockParamNum expression.

Potential Issues:
The implemented changes do not seem to introduce any direct problem, as they only segregate an operation into two separate steps. However, several indirect issues could still occur:

1. Type Compatibility: This change assumes that the resultant value for the expression ValueStack.size() - BlockParamNum can still fit into a uint32_t. If for any reason the value exceeds the maximum limit of uint32_t, the behavior could be undefined.

2. Performance Overhead: Although quite negligible, the additional type-casting operation could introduce additional overhead, especially if this function is called frequently.

3. Applicability: The patch is titled "Fix windows warning". It's unclear how this change addresses a Windows-specific warning. The impact of this change on other platforms (like Linux, macOS) should be tested or at least considered.

In conclusion, while the code should still work as expected, additional tests under different conditions or platforms could help ensure the change does not introduce unexpected side effects.

Commit 8ddd4490306e85460ab098ad7c10951704e7475f

This patch is primarily concerned with the handling of exceptions within the interpreter mode. More specifically, it modifies the class StackManager in the header file stackmgr.h. The changes predominantly change the order of operations within the stack size calculations in methods emplace_back of HandlerStack and pushHandler.

Key modifications include:

1. StackManager: The arithmetic operation priority has been altered in the attribute calculations of emplace_back and pushHandler methods in the StackManager class. Subtract operations are moved to occur after casting via static_cast<uint32_t>, potentially reducing the chance of negative number results.

Potential problems and considerations:

1. Assuming the purpose of the code is to ensure that all inputs to static_cast<uint32_t> are non-negative, these changes should accomplish that goal. However, if negative inputs are possible and intended, these modifications could break existing functionality.
2. If multithreading or concurrency is involved, the added calculations could potentially introduce race conditions, as stack size could change between the computations. However, this is highly unlikely given that the stack size wouldn't usually change so fast.
3. There must be adequate tests around these areas in order to ensure that the changes in operation priority have not introduced any unforeseen side effects. If there are no tests for this, I will suggest adding some to ensure the safety and functionality of this change.

[harry900831]

According to the spec test json file, there's an "expected" section to check the type of uncaught exception. However, it would be quite unclean to deal with it under the current code structure if we want to get the type of the uncaught exception. Therefore, I think it's enough for us to check whether there's an uncaught exception.

[hydai]

Sounds acceptable? Does the expected section require by the Spec?

[harry900831]

I'm not sure whether this is require by the spec.

Below is the example json config for spec test. If I understand correctly, "type" inside "expected" is the type of the value associated with the exception. Our implementation to handle "expected" actually suppose there should be a "value" after "type" just like the below "assert_return". Furthermore, "assert_trap" also have the "expected" section, but it seems that our spectest.cpp doesn't deal with it either. So, I believe it's acceptable to not deal with the "expected" section with "assert_exception".

  {"type": "assert_trap", "line": 160, "action": {"type": "invoke", "field": "trap-in-callee", "args": [{"type": "i32", "value": "1"}, {"type": "i32", "value": "0"}]}, "text": "integer divide by zero", "expected": [{"type": "i32"}]}, 
  {"type": "assert_return", "line": 162, "action": {"type": "invoke", "field": "catch-complex-1", "args": [{"type": "i32", "value": "0"}]}, "expected": [{"type": "i32", "value": "3"}]}, 
  {"type": "assert_return", "line": 163, "action": {"type": "invoke", "field": "catch-complex-1", "args": [{"type": "i32", "value": "1"}]}, "expected": [{"type": "i32", "value": "4"}]}, 
  {"type": "assert_exception", "line": 164, "action": {"type": "invoke", "field": "catch-complex-1", "args": [{"type": "i32", "value": "2"}]}, "expected": [{"type": "i32"}]}, 
  {"type": "assert_return", "line": 166, "action": {"type": "invoke", "field": "catch-complex-2", "args": [{"type": "i32", "value": "0"}]}, "expected": [{"type": "i32", "value": "3"}]}, 
  {"type": "assert_return", "line": 167, "action": {"type": "invoke", "field": "catch-complex-2", "args": [{"type": "i32", "value": "1"}]}, "expected": [{"type": "i32", "value": "4"}]}, 
  {"type": "assert_exception", "line": 168, "action": {"type": "invoke", "field": "catch-complex-2", "args": [{"type": "i32", "value": "2"}]}, "expected": [{"type": "i32"}]}, 

[harry900831]

I'm not sure whether it's a proper way to distinguish the tag by the address. The current implementation may compare two tag address to indicate whether they are the same tag.

[harry900831]

The current implementation may pass the exception handling spec test. But, it may failed on the following test right now:

• wasmedgeAOTCoreTests
• wasmedgeExecutorCoreTests
• wasmedgeAPIVMCoreTests
• wasmedgeAPIStepsCoreTests
• wasmedgeAPIAOTCoreTests

For the AOT test, I believe it's reasonable since I hasn't implemented AOT yet.

For the rest of the tests, they all failed on the same part:

[2023-05-22 04:57:17.976] [error] loading failed: unexpected end, Code: 0x22
[2023-05-22 04:57:17.976] [error]     Bytecode offset: 0x0000000e
[2023-05-22 04:57:17.976] [error]     At AST node: tag section
[2023-05-22 04:57:17.976] [error]     At AST node: import section
[2023-05-22 04:57:17.976] [error]     At AST node: module
[2023-05-22 04:57:17.976] [error]     File name: "../spec/testSuites/core/binary/binary.139.wasm"
[2023-05-22 04:57:17.976] [error]    ##### expected text : malformed import kind
[2023-05-22 04:57:17.976] [error]    ######## error text : unexpected end
/Users/hchiang/WasmEdge/test/spec/spectest.cpp:530: Failure
Value of: stringContains(Text, WasmEdge::ErrCodeStr[Res.error().getEnum()])
  Actual: false
Expected: true
[2023-05-22 04:57:17.976] [error] loading failed: unexpected end, Code: 0x22
[2023-05-22 04:57:17.976] [error]     Bytecode offset: 0x0000000f
[2023-05-22 04:57:17.976] [error]     At AST node: tag section
[2023-05-22 04:57:17.977] [error]     At AST node: import section
[2023-05-22 04:57:17.977] [error]     At AST node: module
[2023-05-22 04:57:17.977] [error]     File name: "../spec/testSuites/core/binary/binary.140.wasm"
[2023-05-22 04:57:17.977] [error]    ##### expected text : malformed import kind
[2023-05-22 04:57:17.977] [error]    ######## error text : unexpected end
/Users/hchiang/WasmEdge/test/spec/spectest.cpp:530: Failure
Value of: stringContains(Text, WasmEdge::ErrCodeStr[Res.error().getEnum()])
  Actual: false
Expected: true

It seems that it's because the binary format has changed, the malformed part may be distinguished as another type of error. I hasn't come up with a good work around. Do yo have any idea?

[harry900831]

Another question, do I have to add several tag related function to WasmEdge C API? For example: WasmEdge_TagTypeCreate(), WasmEdge_TagInstanceGetTagType(), .......

[hydai]

The current implementation may pass the exception handling spec test. But, it may failed on the following test right now:

• wasmedgeAOTCoreTests
• wasmedgeExecutorCoreTests
• wasmedgeAPIVMCoreTests
• wasmedgeAPIStepsCoreTests
• wasmedgeAPIAOTCoreTests

For the AOT test, I believe it's reasonable since I hasn't implemented AOT yet.

If we are going to implement the AOT part, then we should have this PR as a standalone branch until all tests are green.

For the rest of the tests, they all failed on the same part:

[2023-05-22 04:57:17.976] [error] loading failed: unexpected end, Code: 0x22
[2023-05-22 04:57:17.976] [error]     Bytecode offset: 0x0000000e
[2023-05-22 04:57:17.976] [error]     At AST node: tag section
[2023-05-22 04:57:17.976] [error]     At AST node: import section
[2023-05-22 04:57:17.976] [error]     At AST node: module
[2023-05-22 04:57:17.976] [error]     File name: "../spec/testSuites/core/binary/binary.139.wasm"
[2023-05-22 04:57:17.976] [error]    ##### expected text : malformed import kind
[2023-05-22 04:57:17.976] [error]    ######## error text : unexpected end
/Users/hchiang/WasmEdge/test/spec/spectest.cpp:530: Failure
Value of: stringContains(Text, WasmEdge::ErrCodeStr[Res.error().getEnum()])
  Actual: false
Expected: true
[2023-05-22 04:57:17.976] [error] loading failed: unexpected end, Code: 0x22
[2023-05-22 04:57:17.976] [error]     Bytecode offset: 0x0000000f
[2023-05-22 04:57:17.976] [error]     At AST node: tag section
[2023-05-22 04:57:17.977] [error]     At AST node: import section
[2023-05-22 04:57:17.977] [error]     At AST node: module
[2023-05-22 04:57:17.977] [error]     File name: "../spec/testSuites/core/binary/binary.140.wasm"
[2023-05-22 04:57:17.977] [error]    ##### expected text : malformed import kind
[2023-05-22 04:57:17.977] [error]    ######## error text : unexpected end
/Users/hchiang/WasmEdge/test/spec/spectest.cpp:530: Failure
Value of: stringContains(Text, WasmEdge::ErrCodeStr[Res.error().getEnum()])
  Actual: false
Expected: true

It seems that it's because the binary format has changed, the malformed part may be distinguished as another type of error. I hasn't come up with a good work around. Do yo have any idea?

Does the binary format change during this proposal?

[hydai]

Another question, do I have to add several tag related function to WasmEdge C API? For example: WasmEdge_TagTypeCreate(), WasmEdge_TagInstanceGetTagType(), .......

Yes.

[harry900831]

If we are going to implement the AOT part, then we should have this PR as a standalone branch until all tests are green.

Yes, so this would be merged into WasmEdge:proposal/exception-handling

The below issue has been resolved.

Does the binary format change during this proposal?

To be more specific, some wasm may be parsed successfully after enabling exception handling while they are malformed before.

The current problem is that our implementation may generate a different error message compare to the spec test. I believe it's because part of the malformed part become valid after enabling exception handling proposal, but it's still malformed. And I believe the error message generated by the current implementation is reasonable. We can check the failed binary.139.wasm and binary.140.wasm by wasm2wat. It has the different error message too.

[q82419]

Suggestion of the tag section:

• TagSection: vector of TagType
• TagType: 0x00 (uint8_t in current), TypeIndex (uint32_t), FunctionType*
  • At the end of the module loading, find the pointer of function types and set into the content of tag section.
• TagInstanance: TagType

[codecov]

Codecov Report

Merging #2459 (8ddd449) into proposal/exception-handling (f90b1a2) will decrease coverage by 0.15%.
The diff coverage is 78.10%.

@@                       Coverage Diff                       @@
##           proposal/exception-handling    #2459      +/-   ##
===============================================================
- Coverage                        80.99%   80.85%   -0.15%     
===============================================================
  Files                              150      152       +2     
  Lines                            21596    22138     +542     
  Branches                          4338     4477     +139     
===============================================================
+ Hits                             17492    17900     +408     
- Misses                            2944     3042      +98     
- Partials                          1160     1196      +36     

Files Changed	Coverage Δ
include/executor/executor.h	70.00% <ø> (ø)
include/validator/validator.h	100.00% <ø> (ø)
lib/api/wasmedge.cpp	91.43% <0.00%> (-2.78%)	⬇️
lib/driver/runtimeTool.cpp	17.03% <0.00%> (-0.29%)	⬇️
lib/loader/ast/type.cpp	86.23% <30.76%> (-5.77%)	⬇️
lib/validator/validator.cpp	89.17% <57.89%> (-3.40%)	⬇️
lib/loader/ast/instruction.cpp	84.45% <61.33%> (-6.26%)	⬇️
lib/loader/ast/description.cpp	86.95% <70.00%> (-2.88%)	⬇️
include/runtime/instance/module.h	88.58% <75.00%> (-1.42%)	⬇️
lib/loader/ast/module.cpp	84.61% <76.92%> (-0.56%)	⬇️
... and 20 more

... and 1 file with indirect coverage changes

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

[harry900831]

@q82419
It seems that there’s a lot of windows compile warning when it comes to the conversion from size_t to uint32_t.
I’m wondering whether it’s fine to change the VPos, HPos, FPos, … from stackmgr.h to size_t

[q82419]

@q82419 It seems that there’s a lot of windows compile warning when it comes to the conversion from size_t to uint32_t. I’m wondering whether it’s fine to change the VPos, HPos, FPos, … from stackmgr.h to size_t

You can use static_cast the type into uint32_t instead on line 165.

[hydai]

LGTM. @q82419 PTAL.