Add support for hooks in bytecode execution #8
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe changes involve updates to opcode constant representation across multiple files, changing them from lowercase to uppercase for consistency. A new hook mechanism was introduced in the virtual machine, allowing pre-execution functions to log and manage instruction execution. Additionally, a new test function was added to validate the interpreter's handling of excessive recursion and backtracking. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (4)
interpreter_test.go (4)
1190-1190: RenamenbCalltocallCountfor improved readabilityThe variable name
nbCallmay not be immediately clear. Renaming it tocallCountwould enhance code readability and follow Go naming conventions.Apply this diff to rename the variable:
- limitHooker := func(nbCall *int) engine.HookFunc { + limitHooker := func(callCount *int) engine.HookFunc {Don't forget to update all references to
nbCallwithin the function and where it's called.
1203-1203: Use consistent variable namingTo align with the suggested renaming in
limitHooker, renamenbCallstocallCountfor consistency.Apply this diff to rename the variable:
- nbCalls := 0 + callCount := 0Ensure you update all references to
nbCallswithin this test.
1202-1221: Consider using descriptive test names without emojisUsing emojis in test names, such as
"💣 recursion of death", may affect readability and tooling compatibility. Consider using plain text names for clarity and consistency.Apply this diff to rename the test:
- t.Run("💣 recursion of death", func(t *testing.T) { + t.Run("Recursion of Death", func(t *testing.T) {
1222-1241: Consider using descriptive test names without emojisSimilar to the previous comment, consider renaming
"💣 backtrack of death"to improve clarity and maintain consistency across test names.Apply this diff to rename the test:
- t.Run("💣 backtrack of death", func(t *testing.T) { + t.Run("Backtrack of Death", func(t *testing.T) {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Commits
Files that changed from the base of the PR and between ea00e9b and e0b6f193c3f2d2eabec47ea55c1911b15e7a36a0.
Files selected for processing (3)
- engine/clause.go (4 hunks)
- engine/vm.go (4 hunks)
- interpreter_test.go (2 hunks)
Additional comments not posted (8)
engine/clause.go (7)
74-74: LGTM: Consistent opcode naming conventionThe change from
opExittoOpExitaligns with the new uppercase naming convention for opcodes. This modification improves consistency without altering the functionality.
Line range hint
91-97: LGTM: Consistent opcode naming in compileBody methodThe change from
opEntertoOpEntermaintains consistency with the new uppercase naming convention for opcodes. The rest of the function logic remains intact, ensuring that the functionality is preserved.
110-113: LGTM: Consistent opcode naming in compilePred methodThe changes from
opCuttoOpCutandopCalltoOpCallare in line with the new uppercase naming convention for opcodes. These modifications improve consistency while maintaining the original functionality of the code.
119-119: LGTM: Consistent opcode naming for compound termsThe change from
opCalltoOpCallfor compound terms maintains consistency with the new uppercase naming convention for opcodes. The functionality remains unchanged.
129-153: LGTM: Comprehensive opcode naming updates in compileHeadArg methodThe changes in the
compileHeadArgmethod demonstrate a thorough application of the new uppercase naming convention for opcodes. The modifications include:
OpGetVarOpGetConstOpGetListOpPopOpGetPartialOpGetFunctorThese changes improve consistency across the codebase while preserving the original functionality of each instruction.
160-189: LGTM: Consistent opcode naming updates in compileBodyArg methodThe changes in the
compileBodyArgmethod mirror those in thecompileHeadArgmethod, applying the uppercase naming convention to the following opcodes:
OpPutVarOpPutConstOpPutListOpPopOpPutPartialOpPutFunctorThese modifications maintain consistency with the rest of the codebase while preserving the original functionality. The parallel structure between
compileBodyArgandcompileHeadArgis maintained, with "Put" operations used instead of "Get" operations.
Line range hint
1-203: Summary: Consistent opcode naming convention applied throughout the fileThe changes in this file demonstrate a systematic update of opcode naming conventions from lowercase to uppercase. This modification improves code consistency and readability without altering the underlying logic or functionality. While these changes don't directly implement the hooks mentioned in the PR objectives, they contribute to a more uniform codebase, which can facilitate the integration of the new hook system in the virtual machine.
Key points:
- All opcode constants have been updated to uppercase (e.g.,
opExittoOpExit,opEntertoOpEnter, etc.).- The changes are consistent across different methods and code paths.
- No functional changes or logic alterations were introduced.
These updates lay a solid foundation for the implementation of hooks in the bytecode execution process, as described in the PR objectives.
interpreter_test.go (1)
1216-1216: Ensure error messages are accurately comparedThe expected error message
"error(resource_error(calls),recursionOfDeath/0)"may not match the actual error due to differences in formatting or content. Consider verifying the exact error message returned or using assertions that check error types instead of exact strings.Run the following script to check the actual error message:
Review the output to ensure the error message in the assertion matches the actual error.
|
@coderabbitai pause |
ccamel
force-pushed
the
feat/vm-hooks
branch
from
e0b6f19 to
7466eb6
Compare
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #8 +/- ##
==========================================
- Coverage 98.31% 98.29% -0.02%
==========================================
Files 23 23
Lines 6519 6561 +42
==========================================
+ Hits 6409 6449 +40
Misses 81 81
- Partials 29 31 +2
|
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (5)
interpreter_test.go (2)
1202-1241: Add comments explaining the expected behavior of each test caseWhile the test cases are well-structured, adding comments to explain the expected behavior would improve readability and maintainability. For example:
t.Run("💣 recursion of death", func(t *testing.T) { + // This test case verifies that the interpreter correctly handles + // and terminates infinite recursion by throwing a resource error + // when the call limit is exceeded. nbCalls := 0 // ... rest of the test case ... }) t.Run("💣 backtrack of death", func(t *testing.T) { + // This test case ensures that the interpreter properly manages + // excessive backtracking by throwing a resource error when + // the call limit is reached during repeated backtracking attempts. nbCalls := 0 // ... rest of the test case ... })These comments provide context for each test case, making it easier for other developers to understand the purpose and expected outcome of each test.
1203-1203: Consider reusingnbCallsvariableThe
nbCallsvariable is reset to 0 for each sub-test. If the intention is to have a fresh count for each test, this is correct. However, if you want to test the cumulative effect of calls across both tests, you might want to declarenbCallsonce at the beginning ofTestInterpreter_Bombing.If the current behavior is intended, it might be worth adding a comment to clarify why
nbCallsis reset for each test:+// nbCalls is reset for each test to ensure independent call counting nbCalls := 0This clarifies the intention behind resetting the variable for each test.
Also applies to: 1224-1224
engine/builtin_test.go (3)
Line range hint
21-108: LGTM with suggestion: TestCall functionThe
TestCallfunction provides comprehensive test coverage for various scenarios of theCallfunctionality. It sets up a virtual machine with predefined procedures and runs multiple test cases to verify different aspects.Consider splitting this large test function into smaller, more focused test cases. This would improve readability and maintainability. For example, you could create separate test functions for each major scenario (e.g.,
TestCallWithUndefinedAtom,TestCallWithDefinedCompound, etc.).
Line range hint
110-458: LGTM with refactoring suggestion: TestCall1 to TestCall7 functionsThe
TestCall1toTestCall7functions provide good test coverage for the various arities of theCallfunctionality. They follow a consistent structure and cover different scenarios, including error cases.Consider refactoring these similar test functions to reduce code duplication. You could create a helper function that takes the arity as a parameter and generates the appropriate test cases. This would make the code more maintainable and easier to extend in the future. For example:
func testCallN(t *testing.T, n int) { // Common setup code tests := []struct { // ... test case structure }{ // ... generate test cases based on n } for _, tt := range tests { t.Run(tt.title, func(t *testing.T) { // ... common test logic }) } } func TestCall1(t *testing.T) { testCallN(t, 1) } func TestCall2(t *testing.T) { testCallN(t, 2) } // ... and so onThis approach would significantly reduce the amount of repeated code while maintaining the same level of test coverage.
Line range hint
544-2713: LGTM with suggestion: TestUnify functionThe
TestUnifyfunction provides comprehensive test coverage for various unification scenarios. The use of comments to describe each test case improves readability and understanding of the tests.Consider splitting this large test function into smaller, more focused test cases. This would improve readability and maintainability. You could group similar test cases into separate test functions, for example:
func TestUnifySimpleCases(t *testing.T) { // Test cases for simple unifications } func TestUnifyComplexStructures(t *testing.T) { // Test cases for complex structures } func TestUnifyErrorCases(t *testing.T) { // Test cases for error scenarios }This approach would make it easier to understand and maintain the tests, while still maintaining the same level of coverage.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Commits
Files that changed from the base of the PR and between e0b6f193c3f2d2eabec47ea55c1911b15e7a36a0 and 7466eb6.
Files selected for processing (5)
- engine/builtin_test.go (7 hunks)
- engine/clause.go (4 hunks)
- engine/text_test.go (18 hunks)
- engine/vm.go (4 hunks)
- interpreter_test.go (2 hunks)
Files skipped from review due to trivial changes (1)
- engine/text_test.go
Files skipped from review as they are similar to previous changes (2)
- engine/clause.go
- engine/vm.go
Additional context used
Learnings (1)
interpreter_test.go (1)
Learnt from: ccamel PR: axone-protocol/prolog#8 File: interpreter_test.go:1193-1193 Timestamp: 2024-09-23T10:32:12.852Z Learning: In Go, the expression `*nbCall++` is valid and increments the value pointed to by `nbCall`.
Additional comments not posted (3)
engine/builtin_test.go (3)
Line range hint
1-19: LGTM: Import statements and package declarationThe import statements and package declaration look good. The use of the
orderedmappackage suggests that order-preserving maps are utilized in this package, which can be useful for maintaining specific orderings in data structures.
Line range hint
460-542: LGTM: TestCallNth functionThe
TestCallNthfunction is well-structured and provides good test coverage for theCallNthfunctionality. It covers various scenarios, including error cases, and uses assert statements to validate the results properly.
2713-2714: LGTM: Bytecode opcode updatesThe changes to the bytecode opcodes, updating them from lowercase to uppercase (e.g.,
opGetConsttoOpGetConst,opExittoOpExit), improve consistency in the codebase. This aligns with the earlier mentioned update to opcode constant representation.Also applies to: 2727-2728
Actions performedReviews paused. |
Purpose
This PR introduces support for hooks in the VM’s bytecode execution, allowing for finer control over how instructions are processed.
Rationale
These hooks are crucial for preventing certain kind of requests from leading to uncontrolled memory growth or infinite execution without termination (see tests). By utilizing hooks, execution can be more effectively monitored and halted when predefined limits are reached, mitigating potential attacks and preventing resource exhaustion (to a certain extent).
Scope
Although general in scope, this functionality is first intended for use in the Axoned chain to enable specific safety hooks.