Add test cases for INTMIN and -1 #325
Conversation
INTMIN * -1 => INTMIN This PR adds test cases for NEG, MUL, SDIV, and SMOD Signed-off-by: Dave Thaler <[email protected]>
WalkthroughThis pull request updates the Changes
Poem
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: 1
🧹 Outside diff range and nitpick comments (5)
tests/smod32-intmin-by-negone-reg.data (1)
1-9: Test case aligns with PR objectives, consider adding more cases.This test case for smod32 with INTMIN and -1 as operands aligns well with the PR objectives. It covers the SMOD operation for 32-bit integers as mentioned in the PR description.
To fully meet the PR objectives, consider adding similar test cases for:
- NEG (negation) of INTMIN
- MUL (multiplication) of INTMIN and -1
- SDIV (signed division) of INTMIN by -1
Also, consider adding 64-bit versions of these test cases if applicable to your project.
tests/neg32-intmin-imm.data (1)
1-8: Important edge case: Negating INT32_MINThis test case covers an important edge case in two's complement arithmetic. Negating INT32_MIN (0x80000000) results in the same value due to integer overflow. This behavior is crucial to verify in BPF implementations to ensure correct handling of extreme values.
Consider adding a comment explaining this edge case behavior to improve the test's documentation and make it easier for other developers to understand the purpose of this test.
tests/neg32-intmin-reg.data (1)
1-9: LGTM! Well-structured test case for an important edge case.This test case effectively verifies the behavior of the
neg32instruction when applied to INT32_MIN (0x80000000), which is a critical edge case in two's complement arithmetic. The expected result correctly reflects that negating INT32_MIN should result in the same value due to the limitations of 32-bit signed integer representation.A few suggestions to enhance the test case:
The value in register
%r1is unused in this test. Consider removing it or using it in the test if relevant.It would be helpful to add a comment explaining the edge case behavior, e.g.:
# Test case for negating INT32_MIN (0x80000000) # Expected behavior: neg32(INT32_MIN) == INT32_MINtests/neg64-intmin-imm.data (2)
9-10: LGTM: Expected result is correct, but warrants explanation.The expected result of 0x8000000000000000 is correct. When negating the minimum 64-bit signed integer (-2^63), the result remains the same due to integer overflow in two's complement representation. This behavior is consistent with most programming languages and CPU architectures.
This test case effectively verifies that NEG(INTMIN) == INTMIN, which aligns with the PR objective stating that "INTMIN * -1" is expected to yield "INTMIN".
Consider adding a comment explaining this behavior, as it might not be immediately obvious to all readers why negating INTMIN results in the same value.
1-10: LGTM: Well-structured test case for NEG operation on 64-bit INTMIN.This test case effectively verifies the behavior of the NEG instruction on the minimum 64-bit signed integer (INTMIN). It correctly sets up the scenario, performs the negation, and specifies the expected result. The implementation aligns with the PR objectives and contributes valuable coverage to the test suite.
To enhance the test suite further, consider adding similar test cases for other operations mentioned in the PR objectives, such as MUL, SDIV, and SMOD, if they haven't been implemented in other files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
- tests/mul32-intmin-by-negone-imm.data (1 hunks)
- tests/mul32-intmin-by-negone-reg.data (1 hunks)
- tests/mul64-intmin-by-negone-imm.data (1 hunks)
- tests/mul64-intmin-by-negone-reg.data (1 hunks)
- tests/neg32-intmin-imm.data (1 hunks)
- tests/neg32-intmin-reg.data (1 hunks)
- tests/neg64-intmin-imm.data (1 hunks)
- tests/neg64-intmin-reg.data (1 hunks)
- tests/sdiv32-intmin-by-negone-imm.data (1 hunks)
- tests/sdiv32-intmin-by-negone-reg.data (1 hunks)
- tests/sdiv64-intmin-by-negone-imm.data (1 hunks)
- tests/sdiv64-intmin-by-negone-reg.data (1 hunks)
- tests/smod32-intmin-by-negone-imm.data (1 hunks)
- tests/smod32-intmin-by-negone-reg.data (1 hunks)
- tests/smod64-intmin-by-negone-imm.data (1 hunks)
- tests/smod64-intmin-by-negone-reg.data (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/mul64-intmin-by-negone-imm.data
- tests/mul64-intmin-by-negone-reg.data
🧰 Additional context used
🔇 Additional comments (48)
tests/smod32-intmin-by-negone-imm.data (4)
1-2: LGTM: Copyright notice is correctly formatted.The copyright notice includes the author's name and specifies the MIT license, which is appropriate for this project.
3-6: LGTM: Assembly code correctly tests smod32 with INTMIN and -1.The assembly code is well-structured to test the
smod32instruction with edge case inputs:
- It initializes
%r0with 0x80000000 (INTMIN for 32-bit integers).- It performs
smod32 %r0, -1to test the signed modulus operation with -1.- It exits the program to complete the test.
This test case effectively validates the behavior of
smod32with the minimum 32-bit signed integer and -1 as operands.
7-8: LGTM: Expected result is correct.The expected result of 0x0 is correct for this test case. In signed integer arithmetic, any number modulo -1 should always be 0. This holds true even for the minimum signed 32-bit integer (0x80000000). Therefore, the expected result of 0x0 accurately reflects the correct behavior of the
smod32instruction for this edge case.
1-8: Excellent test case for smod32 with INTMIN and -1.This test case is well-constructed and directly addresses the PR objectives. It effectively tests the
smod32instruction with the minimum 32-bit signed integer (INTMIN) and -1, which is an important edge case. The expected result is correct, and the test will help ensure proper behavior of thesmod32instruction in extreme scenarios.Some key points:
- The copyright and licensing information is clear and appropriate.
- The assembly code is concise and directly tests the intended scenario.
- The expected result is correct and well-justified.
This test case contributes to the overall robustness of the test suite and aligns perfectly with the goal of adding test cases for INTMIN and -1 operations.
tests/sdiv32-intmin-by-negone-imm.data (4)
1-2: LGTM: Copyright and license information is correct.The copyright notice and license specification are clear and follow standard practices for open-source projects.
3-6: LGTM: Assembly code correctly tests INT_MIN / -1 edge case.The assembly code effectively tests the behavior of
sdiv32instruction for the edge case of dividing INT_MIN (0x80000000) by -1. This is a crucial test because:
- It verifies the correct handling of integer overflow in signed division.
- It checks if the implementation adheres to the expected behavior where INT_MIN / -1 should result in INT_MIN due to overflow.
The sequence of instructions is correct and concise:
- Loading INT_MIN into %r0
- Performing the signed division by -1
- Exiting the program
7-8: LGTM: Expected result is correct for INT_MIN / -1.The expected result of 0x80000000 (INT_MIN) is correct for the operation INT_MIN / -1. This result is expected because:
- In two's complement arithmetic, dividing INT_MIN by -1 should theoretically result in |INT_MIN|, which is not representable in a signed 32-bit integer.
- Most implementations, following common CPU behavior, return INT_MIN in this case due to integer overflow.
- This behavior is consistent with many programming languages and is important for maintaining consistency across different platforms.
1-8: Excellent addition: Crucial edge case test for sdiv32.This test case is a valuable addition to the test suite. It effectively covers an important edge case in 32-bit signed division, specifically INT_MIN / -1. The test is well-structured, concise, and correctly implements the intended scenario.
Key points:
- Correctly tests the behavior of
sdiv32with INT_MIN and -1.- Verifies proper handling of integer overflow in this edge case.
- Aligns with common implementation behaviors across various platforms.
This test case significantly contributes to ensuring the robustness and correctness of the
sdiv32implementation.tests/smod32-intmin-by-negone-reg.data (3)
1-2: LGTM: Copyright and license information is present and correct.The file includes appropriate copyright information and specifies the MIT license, which is good practice for open-source projects.
3-7: LGTM: Assembly code correctly implements the intended test case.The assembly code accurately sets up the test case for smod32 operation with INTMIN (0x80000000) and -1 as operands, which aligns with the PR objectives. The sequence of instructions is correct and complete.
8-9: LGTM: Expected result is correct for the given operation.The expected result of 0x0 is mathematically correct for the operation INTMIN % -1 in 32-bit signed integer arithmetic. This accurately represents the behavior of smod32 with the given operands.
tests/neg32-intmin-imm.data (2)
1-8: LGTM! Well-designed test case for an important edge case.The test case correctly implements the negation of INT32_MIN (0x80000000) and expects the result to remain 0x80000000. This accurately represents the behavior of two's complement arithmetic for this edge case.
1-8: Verify completeness of PR objectivesThis test case covers the NEG operation for INT32_MIN, which is valuable. However, the PR objectives also mention test cases for MUL, SDIV, and SMOD operations with INTMIN * -1. Please ensure that these additional test cases are included in the PR, either in separate files or by extending this file.
To check for the presence of other test files related to the PR objectives, you can run the following script:
✅ Verification successful
PR Completeness Confirmed
All required test cases for MUL, SDIV, and SMOD operations with INTMIN * -1 have been included in the PR. The test files are comprehensive and cover the necessary scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the presence of test files related to MUL, SDIV, and SMOD operations with INTMIN * -1 # Test: Search for test files containing MUL, SDIV, or SMOD operations with INTMIN and -1 echo "Searching for related test files:" fd -e data | xargs grep -l -E "(mul|sdiv|smod).*(0x80000000|-1)" # Test: Count the number of new test files added in this PR echo "Number of new test files added in this PR:" git diff --name-only --diff-filter=A origin/main | grep -c "tests/.*\.data"Length of output: 551
tests/mul32-intmin-by-negone-imm.data (3)
1-8: LGTM: Well-structured test case for INT32_MIN multiplicationThis test case is well-structured and correctly tests an important edge case in 32-bit integer arithmetic: multiplying INT32_MIN (-2147483648 or 0x80000000) by -1. The test is divided into clear 'asm' and 'result' sections, making it easy to understand the input and expected output.
The test case correctly captures the expected behavior of this operation, which typically results in the same value (INT32_MIN) due to integer overflow in two's complement arithmetic. This is crucial for ensuring that the BPF implementation handles this special case correctly.
3-6: Assembly code correctly implements the test scenarioThe assembly code section is concise and accurately implements the test scenario:
mov %r0, 0x80000000: Correctly loads INT32_MIN into register %r0.mul32 %r0, -1: Multiplies the value in %r0 by -1 using the 32-bit multiplication instruction.exit: Properly terminates the program after the operation.This sequence of instructions effectively tests the multiplication of INT32_MIN by -1.
7-8: Expected result correctly specifiedThe result section correctly specifies the expected outcome of the operation:
-- result 0x80000000This accurately reflects the expected behavior of multiplying INT32_MIN by -1, which should result in the same value (0x80000000) due to integer overflow in two's complement arithmetic.
tests/smod64-intmin-by-negone-imm.data (4)
1-2: LGTM: Copyright and license information are correctly included.The copyright notice and SPDX license identifier (MIT) are properly added to the file.
3-6: LGTM: Assembly code correctly implements the test case.The assembly code effectively tests the signed modulus operation with INTMIN and -1:
ldxdw %r0, [%r1]loads the INTMIN value (0x8000000000000000) from memory into %r0.smod %r0, -1performs the signed modulus operation.exitterminates the program, allowing the result to be checked.This implementation aligns well with the PR objective of testing operations involving INTMIN and -1.
8-10: LGTM: Memory initialization and expected result are correct.The test case is well-constructed:
- The memory section correctly initializes with INTMIN (0x8000000000000000) in little-endian format.
- The expected result of 0x0 is accurate for INTMIN % -1, as any integer modulo -1 should yield 0.
This setup properly tests the edge case of signed modulus with INTMIN and -1, aligning with the PR objectives.
1-10: Excellent test case for smod with INTMIN and -1.This test case is well-designed and thoroughly covers the edge case of signed modulus operation with INTMIN and -1. It aligns perfectly with the PR objectives and provides valuable coverage for this specific scenario. The assembly code, memory initialization, and expected result are all correct and work together to create a comprehensive test.
Great job on implementing this test case!
tests/neg32-intmin-reg.data (1)
1-9: Verify alignment with PR objectivesThis test case partially aligns with the PR objectives by focusing on the NEG operation with INTMIN (0x80000000). However, it doesn't directly use the -1 value in the operation as mentioned in the PR description.
Consider enhancing this test or adding additional tests to fully cover the PR objectives:
- Implement test cases for MUL, SDIV, and SMOD operations.
- Ensure that the expression
INTMIN * -1is tested, as it's specifically mentioned in the PR description.To confirm the coverage of all mentioned operations, you can run the following script:
This script will help ensure that all the operations mentioned in the PR objectives are covered by the test cases.
tests/mul32-intmin-by-negone-reg.data (4)
1-2: LGTM: Copyright notice is correct and properly formatted.The copyright notice includes the necessary information and uses the appropriate Apache-2.0 license.
8-9: LGTM: Expected result is correct and demonstrates important arithmetic behavior.The expected result of 0x80000000 is correct for the operation INTMIN * -1 in 32-bit signed integer arithmetic. This result effectively demonstrates the overflow behavior in two's complement arithmetic, where multiplying the minimum signed integer by -1 results in the same minimum value due to overflow.
1-9: Overall, this is a well-implemented and valuable test case.This new test file correctly implements a test case for multiplying the minimum 32-bit signed integer (INTMIN) by -1, which is an important edge case in integer arithmetic. The file is well-structured with a proper copyright notice, clear assembly code, and the correct expected result. This test case will contribute to improved coverage of arithmetic operations involving extreme values.
3-7: LGTM: Assembly code correctly implements the test case.The assembly code accurately sets up the test for multiplying the minimum 32-bit signed integer (0x80000000) by -1 using the
mul32instruction. This aligns with the PR objectives.To ensure the correctness of the assembly code, we can verify that similar test cases exist for other operations mentioned in the PR objectives:
✅ Verification successful
Verified: Test cases for NEG, SDIV, and SMOD operations with INTMIN and -1 are present.
tests/smod32-intmin-by-negone-reg.datatests/sdiv32-intmin-by-negone-reg.datatests/neg32-intmin-reg.data🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of test cases for NEG, SDIV, and SMOD operations with INTMIN and -1 # Test: Search for NEG operation test case echo "Searching for NEG operation test case:" rg --type-add 'data:*.data' --type data 'neg.*0x80000000' tests/ # Test: Search for SDIV operation test case echo "Searching for SDIV operation test case:" rg --type-add 'data:*.data' --type data 'sdiv.*0x80000000.*-1' tests/ # Test: Search for SMOD operation test case echo "Searching for SMOD operation test case:" rg --type-add 'data:*.data' --type data 'smod.*0x80000000.*-1' tests/Length of output: 475
Script:
#!/bin/bash # Description: Search for any test cases involving the minimum 32-bit signed integer (INTMIN) and -1 in the tests directory. echo "Searching for test cases with INTMIN (0x80000000):" rg --type-add 'data:*.data' --type data '0x80000000' tests/ echo "Searching for test cases with operand -1:" rg --type-add 'data:*.data' --type data '\-1' tests/Length of output: 3636
tests/sdiv32-intmin-by-negone-reg.data (3)
1-2: LGTM: Copyright notice is correct and properly formatted.The copyright notice includes the necessary information and uses the Apache-2.0 license, which is appropriate for this project.
3-7: LGTM: Assembly code correctly tests INT32_MIN / -1 edge case.The assembly code effectively sets up and executes the test case for dividing the minimum 32-bit signed integer (INT32_MIN) by -1. This is an important edge case for signed division operations, as it can potentially lead to overflow or undefined behavior in some implementations.
The sequence of instructions is correct:
- Initialize r0 with 0x80000000 (INT32_MIN)
- Set r1 to -1
- Perform signed 32-bit division
- Exit the program
This test case will help ensure that the
sdiv32instruction handles this edge case correctly.
8-9: LGTM: Expected result correctly handles INT32_MIN / -1 edge case.The expected result of 0x80000000 (INT32_MIN) for the operation INT32_MIN / -1 is correct and aligns with common implementation practices. This result occurs due to integer overflow, as the mathematically correct result (-INT32_MIN) is not representable in a 32-bit signed integer format.
It's important to note that while this behavior is common and expected in many systems, it might be considered undefined or implementation-defined in some contexts. This test case ensures that the
sdiv32instruction behaves consistently with this common implementation.To ensure consistency across the codebase, let's verify if similar edge cases are handled for 64-bit operations:
tests/sdiv64-intmin-by-negone-imm.data (5)
1-2: LGTM: Copyright and license information is correctly provided.The copyright notice and license specification are properly included at the beginning of the file.
3-6: LGTM: Assembly code correctly implements the test case.The assembly code accurately reflects the PR objective:
- Loads the 64-bit INTMIN value from memory into %r0.
- Performs signed division (SDIV) of INTMIN by -1.
- Exits to return the result.
This implementation aligns perfectly with the stated goal of testing SDIV with INTMIN and -1.
7-8: LGTM: Memory section correctly initializes INTMIN.The memory section contains the value 0x8000000000000000, which is the correct representation of the minimum 64-bit signed integer (INTMIN):
- In binary: 1000...0000 (63 zeros after the leading 1)
- This represents -2^63 in two's complement notation
This initialization aligns perfectly with the PR objective of testing operations with INTMIN.
9-10: LGTM: Result section correctly specifies the expected output.The result section specifies 0x8000000000000000 (INTMIN) as the expected output, which is correct:
- For 64-bit signed integers: INTMIN / -1 = INTMIN
- This occurs because INTMIN (-2^63) divided by -1 should yield 2^63, but this overflows and wraps back to INTMIN in two's complement representation
This result is consistent with the behavior of most CPU architectures for this edge case and aligns with the PR objective.
1-10: LGTM: Excellent implementation of the SDIV test case for INTMIN and -1.This test case successfully implements and validates the signed division (SDIV) operation with the minimum 64-bit signed integer (INTMIN) and -1. The file includes:
- Correct copyright and license information
- Accurate assembly code to perform the operation
- Proper memory initialization with INTMIN
- Correct expected result
The implementation aligns perfectly with the PR objective and provides a valuable test case for this edge condition in 64-bit signed arithmetic.
tests/smod64-intmin-by-negone-reg.data (4)
1-2: LGTM: Copyright and license are correctly specified.The copyright notice and SPDX license identifier are properly included at the beginning of the file.
3-7: LGTM: Assembly instructions correctly implement the test case.The assembly section accurately sets up and executes the test for
INTMIN % -1:
- Loads INTMIN (0x8000000000000000) from memory into %r0.
- Moves -1 into %r1.
- Performs signed modulo operation.
- Exits the program.
This aligns with the PR objective of testing operations with INTMIN and -1, specifically for the SMOD (signed modulus) operation.
9-11: LGTM: Memory initialization and expected result are correct.The memory and result sections are accurately set up:
- Memory is initialized with 0x8000000000000000 (INTMIN for 64-bit integers) in little-endian format.
- The expected result of 0x0 is correct, as any number modulo -1 is always 0.
This correctly tests the edge case of INTMIN % -1 for the SMOD operation.
1-11: Excellent test case for SMOD operation with INTMIN and -1.This test case is well-structured and comprehensive:
- It correctly tests the SMOD operation with INTMIN (0x8000000000000000) and -1.
- The assembly code, memory initialization, and expected result are all consistent and accurate.
- It aligns perfectly with the PR objectives of adding test cases for operations involving INTMIN and -1.
The test case will effectively verify the behavior of the SMOD instruction for this edge case, contributing to the overall robustness of the test suite.
tests/neg64-intmin-imm.data (2)
3-6: LGTM: Assembly code correctly implements the test case.The assembly code accurately loads the minimum 64-bit integer value from memory, negates it, and exits. This implementation aligns with the PR objective of testing the NEG operation on INTMIN.
7-8: LGTM: Memory correctly initialized with INTMIN.The memory section accurately initializes an 8-byte value representing the minimum 64-bit signed integer (0x8000000000000000 or -2^63). The byte order is correct for little-endian systems.
tests/neg64-intmin-reg.data (5)
1-2: LGTM: Copyright notice is correct and properly formatted.The copyright notice includes the necessary information and uses the appropriate Apache-2.0 license for open-source projects.
3-7: LGTM: Assembly instructions are correct and align with the test objective.The assembly code correctly implements the test case for negating INTMIN:
- It loads the INTMIN value (0x8000000000000000) from memory into %r0.
- Moves -1 into %r1 (although this step is not used in the current test).
- Negates the value in %r0, which is the operation being tested.
- Exits the program, allowing the result to be checked.
This sequence effectively tests the behavior of the NEG instruction with INTMIN as input.
9-9: LGTM: Memory initialization correctly sets up INTMIN.The memory is initialized with the value 0x8000000000000000, which represents the minimum value for a 64-bit signed integer (INTMIN). This setup is crucial for testing the negation of INTMIN, which is the primary objective of this test case.
11-11: LGTM: Expected result is correct for negating INTMIN.The expected result of 0x8000000000000000 is correct. When negating INTMIN (0x8000000000000000) in two's complement arithmetic, the result remains the same due to integer overflow. This is a crucial edge case in signed integer arithmetic and is correctly captured by this test.
1-11: Excellent test case for INTMIN negation edge case.This test case is well-structured and effectively covers the important edge case of negating INTMIN in 64-bit signed integer arithmetic. It aligns perfectly with the PR objective of adding test cases for INTMIN operations. The assembly instructions, memory initialization, and expected result all work together to create a comprehensive and clear test.
Some key points:
- The test correctly sets up INTMIN (0x8000000000000000) in memory.
- The NEG instruction is applied to this value.
- The expected result correctly reflects that negating INTMIN results in the same value due to integer overflow.
This test will be valuable in ensuring correct behavior of the NEG instruction with extreme input values.
tests/sdiv64-intmin-by-negone-reg.data (4)
3-7: LGTM: Assembly code correctly implements the test case.The assembly code accurately sets up the test scenario:
- Loads the 64-bit value (INTMIN) from memory into %r0
- Sets %r1 to -1
- Performs signed division of %r0 by %r1
- Exits the program
This correctly implements the intended test case of dividing the minimum 64-bit integer by -1.
9-9: LGTM: Memory correctly initialized with INTMIN.The memory is correctly initialized with the byte pattern
00 00 00 00 00 00 00 80, which represents the minimum 64-bit integer value (0x8000000000000000) in little-endian format. This setup is consistent with the test case requirements.
11-11: LGTM: Expected result is correct and consistent with integer overflow behavior.The expected result of 0x8000000000000000 (INTMIN) is correct for the operation INTMIN / -1 in 64-bit signed integer arithmetic. This result occurs due to integer overflow, as the true mathematical result cannot be represented in a 64-bit signed integer.
This behavior is consistent with the C standard and most programming languages, where INTMIN / -1 is defined to result in INTMIN due to overflow.
1-11: Excellent test case for INTMIN / -1 edge case.This test case is well-constructed and covers an important edge case for 64-bit signed division. It correctly tests the behavior of dividing the minimum 64-bit integer (INTMIN) by -1, which is expected to result in INTMIN due to integer overflow.
The test is valuable for several reasons:
- It verifies correct handling of extreme input values.
- It ensures that the sdiv instruction behaves consistently with standard integer overflow rules.
- It's related to the INTMIN * -1 case mentioned in the PR objectives, providing coverage for related edge cases.
This test will help ensure the robustness and correctness of the implementation when dealing with boundary conditions in signed 64-bit arithmetic.
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Since ubuntu hits an exception on release builds Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/Build.yml (1)
88-88: LGTM: Unified CMake configuration for Ubuntu and macOSThe change to combine Ubuntu and macOS CMake configuration steps is a good simplification of the workflow. It maintains consistency with the previous Ubuntu-related changes.
Consider using a similar
startsWithapproach for macOS to future-proof against potential version changes:if: ${{ startsWith(inputs.platform, 'ubuntu-') || startsWith(inputs.platform, 'macos-') }}This would allow the step to work with future macOS versions as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/Build.yml (4 hunks)
- .github/workflows/CICD.yml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/CICD.yml
🧰 Additional context used
🔇 Additional comments (4)
.github/workflows/Build.yml (4)
50-51: LGTM: Improved flexibility for Ubuntu platformsThe change from
inputs.platform == 'ubuntu-latest'tostartsWith(inputs.platform, 'ubuntu-')is a good improvement. It allows the workflow to run on any Ubuntu version, making it more flexible and future-proof.
69-69: LGTM: Consistent improvement for libbpf build stepThe change to use
startsWith(inputs.platform, 'ubuntu-')for the libbpf build step is consistent with the previous modification. It ensures that libbpf is built and installed on all Ubuntu versions, maintaining flexibility in the workflow.
132-132: LGTM: Consistent improvement for test executionThe change to use
startsWith(inputs.platform, 'ubuntu-')for the test execution step is consistent with the previous modifications. It ensures that tests are run on all Ubuntu versions, maintaining flexibility in the workflow.
Line range hint
1-180: Overall assessment: Improved workflow flexibilityThe changes in this file consistently enhance the workflow's flexibility, particularly for Ubuntu platforms. By using
startsWith(inputs.platform, 'ubuntu-')instead of exact matches, the workflow now supports multiple Ubuntu versions, making it more robust and future-proof.Key improvements:
- Prerequisite installation for Ubuntu
- libbpf build process
- CMake configuration
- Test execution
These changes maintain the overall structure and logic of the workflow while improving its adaptability. Consider applying a similar approach to macOS platform checks for even greater flexibility.
Signed-off-by: Dave Thaler <[email protected]>
INTMIN * -1 => INTMIN.
See https://mailarchive.ietf.org/arch/msg/bpf/nvaEmn-ydpHd0_0CvEzELp092FU/ for discussion
This PR adds test cases for NEG, MUL, SDIV, and SMOD
I verified all test cases using ebpf-verifier.
Summary by CodeRabbit
neg32andneg64instructions validating their behavior with the input value0x80000000.ubuntu-22.04for certain jobs, enhancing consistency in the build environment.