Core add timer base min - #2
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Buddy - CI Failure Analysis
Important
This analysis was generated by AI and may not be fully accurate. Please review the suggestions critically before applying any changes.
馃煝 Confidence: high
Multiple build jobs are failing during compilation because the extern declaration for timer_vtable was removed from src/core/timer.h. This causes an 'undeclared identifier' error in src/core/unit.c, which still uses this variable. Other unrelated build and CodeQL analysis failures appear to be caused by a missing bpftool in the CI environment.
Commit validationTracker - RHEL-155454 The following commits meet all requirements
Tracker validation馃敶 Tracker 'RHEL-155454' does not exist on https://redhat.atlassian.net Pull Request validationFailed馃敶 Failed or pending checks:
馃敶 Review - Missing review from a member (1 required) Triggered by Workflow Run |
There was a problem hiding this comment.
Review Buddy - CI Failure Analysis
[!IMPORTANT]\n> This analysis was generated by AI and may not be fully accurate. Please review the suggestions critically before applying any changes.
馃煝 Confidence: high
The CI failures are not caused by the code changes in this pull request. All build jobs (GCC, Clang, CodeQL C++) are failing during the Meson configuration step with the error Command /usr/sbin/bpftool --version failed, which indicates bpftool is missing or misconfigured in the CI runner environment. While unrelated to these failures, a semantic bug was found in the code changes.
| TIMER_CALENDAR, | ||
| _TIMER_BASE_MAX, | ||
| _TIMER_BASE_MIN, | ||
| _TIMER_BASE_INVALID = -EINVAL, |
There was a problem hiding this comment.
While unrelated to the current CI failures (which appear to be an environmental issue), this change introduces a semantic bug. By adding _TIMER_BASE_MIN after _TIMER_BASE_MAX, its value will be greater than _TIMER_BASE_MAX, which will break any logic that relies on a MIN < MAX ordering.
If this is intended to define the minimum value, it should be placed at the beginning of the enum. If it serves another purpose, it should be renamed to avoid confusion.
| _TIMER_BASE_INVALID = -EINVAL, | |
| _TIMER_BASE_MAX, |
Please remove this line and re-introduce it in the correct location at the top of the enum if it's needed.
There was a problem hiding this comment.
Review Buddy - CI Failure Analysis
[!IMPORTANT]\n> This analysis was generated by AI and may not be fully accurate. Please review the suggestions critically before applying any changes.
馃煝 Confidence: high
Multiple build jobs are failing due to a CI environment issue (missing bpftool), which appears unrelated to the code changes. However, the mkosi integration test job shows an AddressSanitizer failure (stack-buffer-underflow). This memory error is very likely caused by the incorrect ordering of enum members in src/core/timer.h, where _TIMER_BASE_MIN is defined after _TIMER_BASE_MAX, resulting in _TIMER_BASE_MIN > _TIMER_BASE_MAX.
| TIMER_CALENDAR, | ||
| _TIMER_BASE_MAX, | ||
| _TIMER_BASE_MIN, | ||
| _TIMER_BASE_INVALID = -EINVAL, |
There was a problem hiding this comment.
The mkosi integration test job is failing with an AddressSanitizer stack-buffer-underflow error, which is very likely caused by this change.
By adding _TIMER_BASE_MIN after _TIMER_BASE_MAX, their values become _TIMER_BASE_MAX = 2 and _TIMER_BASE_MIN = 3. This violates the common MIN <= MAX invariant and is likely causing out-of-bounds memory access in code that uses this enum.
Please reorder these members to ensure _TIMER_BASE_MIN has a smaller value than _TIMER_BASE_MAX.
| _TIMER_BASE_INVALID = -EINVAL, | |
| _TIMER_BASE_MIN, | |
| _TIMER_BASE_MAX, |
There was a problem hiding this comment.
Review Buddy - CI Failure Analysis
[!IMPORTANT]\n> This analysis was generated by AI and may not be fully accurate. Please review the suggestions critically before applying any changes.
馃煛 Confidence: medium
Most build jobs are failing during the configuration phase due to a bpftool version check. This appears to be a CI environment issue unrelated to the code changes. However, the mkosi integration test, which did build successfully, failed with sanitizer errors. A logic error in the modified TimerBase enum in src/core/timer.h is a likely cause of these test failures, as it breaks the convention for _MIN and _MAX enum sentinels.
| TIMER_CALENDAR, | ||
| _TIMER_BASE_MAX, | ||
| _TIMER_BASE_MIN, | ||
| _TIMER_BASE_INVALID = -EINVAL, |
There was a problem hiding this comment.
This adds _TIMER_BASE_MIN after _TIMER_BASE_MAX, which is incorrect and breaks the logic of the enum. Conventionally, _MAX is the last item and its automatically-assigned value is used as a count of the preceding items. With this change, _TIMER_BASE_MAX has a value of 2, while _TIMER_BASE_MIN has a value of 3.
This will break any code that uses _TIMER_BASE_MAX for array sizing or loop bounds, potentially leading to out-of-bounds access, which could explain the sanitizer errors seen in other CI jobs. Please reorder these members.
| _TIMER_BASE_INVALID = -EINVAL, | |
| _TIMER_BASE_MIN, | |
| TIMER_UNIT_INACTIVE, | |
| TIMER_CALENDAR, | |
| _TIMER_BASE_MAX, |
There was a problem hiding this comment.
Review Buddy - CI Failure Analysis
[!IMPORTANT]\n> This analysis was generated by AI and may not be fully accurate. Please review the suggestions critically before applying any changes.
馃煝 Confidence: high
Multiple build jobs failed due to a missing bpftool dependency, which appears to be a CI environment issue unrelated to the code changes. However, the mkosi sanitizer job failed with a stack-buffer-underflow. This is very likely caused by the incorrect ordering of the new _TIMER_BASE_MIN enum member in src/core/timer.h, which gives it a larger value than _TIMER_BASE_MAX.
| TIMER_CALENDAR, | ||
| _TIMER_BASE_MAX, | ||
| _TIMER_BASE_MIN, | ||
| _TIMER_BASE_INVALID = -EINVAL, |
There was a problem hiding this comment.
This enum member has been added in the wrong place. By placing _TIMER_BASE_MIN after _TIMER_BASE_MAX, it is assigned a larger integer value (3) than _TIMER_BASE_MAX (2), which is logically incorrect.
This is the likely cause of the AddressSanitizer: stack-buffer-underflow error reported by the mkosi test job. To fix this, _TIMER_BASE_MIN should be moved to the beginning of the enum to ensure it has the lowest value.
| _TIMER_BASE_INVALID = -EINVAL, | |
| _TIMER_BASE_MIN, | |
| TIMER_UNIT_INACTIVE, | |
| TIMER_CALENDAR, | |
| _TIMER_BASE_MAX, |
107381d to
3c4663e
Compare
rhel-only: doc Resolves: RHEL-155454
rhel-only: policy Resolves: RHEL-155454
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> rhel-only: ci Related: RHEL-155454
3c4663e to
2cc1e5e
Compare
1 participant
No description provided.