Repository navigation
fix(amdsmi): make the ABI compliance check catch real breaks - #14425
Merged
Merged
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
- Parse the abi-compliance-checker 2.3 report markup; the old patterns never matched it, so every report read as clean - Fail on removed symbols, High or Medium problems, and any type size change. The checker rates struct growth Low because it assumes the library allocates the struct, but amd-smi callers allocate theirs - Fail major mode when the checker exits non-zero instead of passing with a warning - Compare against the merge base with the base branch, so APIs merged after a PR forked are not reported as removed by that PR - Rewrite abi_check_test.py around real report markup and run it from pre-commit when tests/abi_check changes; no workflow ran it before Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
marifamd
force-pushed
the
users/marifamd/amdsmi-abi-check-fix
branch
from
October 28, 2026 23:16
fd41276 to
6b1dfe6
Compare
dmitrii-galantsev
approved these changes
Oct 29, 2026
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The ABI-check script’s use of text=True causes an uncaught TypeError on Python 3.6, which is included in the test plan, while mocked subprocess calls hide the incompatibility in the new tests.
1 open issue
What changed in this PR
Strengthens amd-smi’s ABI compliance checks to detect breaking header changes and avoid false reports caused by newer changes on the base branch.
Changes:
- Parses checker report headings and type-size changes, and fails when the checker exits non-zero.
- Compares headers against the merge base rather than the base branch tip.
- Expands regression tests, adds a pre-commit test hook, and updates test documentation.
| File | Description |
|---|---|
projects/amdsmi/tests/README.md |
Documents the merge-base comparison and ABI test hook. |
projects/amdsmi/tests/abi_check/abi_check.py |
Fixes report parsing, baseline selection, and failure handling. |
projects/amdsmi/tests/abi_check/abi_check_test.py |
Adds report fixtures, merge-base tests, and pass/fail checks. |
projects/amdsmi/.pre-commit-config.yaml |
Runs ABI checker tests when their directory changes. |
Review effort: Max
charis-poag-amd
approved these changes
Oct 30, 2026
charis-poag-amd
left a comment
charis-poag-amd
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Reading counts from the section headings, failing on struct size changes, and baselining on the merge base are the right fixes, and the real-report test table is convincing. Flagging amdsmi_enumeration_info_t (276 to 280 bytes in therock-10.0 to 10.1) as a break is correct.
Inline comments are non-blocking. The main one: a checker error (non-zero exit other than 1) is now reported as an ABI break, which could produce a false MAJOR ABI BREAKAGE label if the old header can't be fetched.
Since the workflow runs the base branch's abi_check.py, this PR's own ABI jobs use the old parser. Worth a follow-up check in CI after merge.
1 task
dmitrii-galantsev
added a commit
that referenced
this pull request
Oct 30, 2026
Follow-up to #14425 and its review. abi-compliance-checker exits 1 for incompatible changes and other codes when it cannot compare, for example 11 for the empty old header that prepare_headers() wrote when git show failed. #14425 reported any non-zero exit as a break, which would add a false MAJOR ABI BREAKAGE label. abi_check.py now exits 2 for a checker error, a missing merge base, an old header it cannot fetch, or a report without the 2.3 summary table, and the workflow records that as "error", which leaves the labels alone. Falling back to the base branch tip when the merge base is missing would report everything the branch gained as removed, so that is an error too. The x86_64 library is built with ENABLE_ESMI_LIB, but the checker parsed amdsmi.h without it and skipped the CPU API: 69 functions, 12 types and the SPD and power-efficiency limits. Define it at the top of both copies of the header. Callers size buffers and check limits with amdsmi.h constants when they are built, so raising AMDSMI_MAX_MM_IP_COUNT from 8 to 16 breaks old binaries. The checker rates that Low, so the check passed it. Fail on any changed non-string constant in the binary compatibility report, which already leaves out AMDSMI_LIB_VERSION_*. Also: a size row no longer inherits the type name from an earlier section, merge_base() works on Python 3.6, and the tests cover High severity, symbol problems and Low type problems that are not size changes.
dmitrii-galantsev
added a commit
that referenced
this pull request
Oct 30, 2026
Follow-up to #14425 and its review. abi-compliance-checker exits 1 for incompatible changes and other codes when it cannot compare, for example 11 for the empty old header that prepare_headers() wrote when git show failed. #14425 reported any non-zero exit as a break, which would add a false MAJOR ABI BREAKAGE label. abi_check.py now exits 2 for a checker error, a missing merge base, an old header it cannot fetch, or a report without the 2.3 summary table, and the workflow records that as "error", which leaves the labels alone. Falling back to the base branch tip when the merge base is missing would report everything the branch gained as removed, so that is an error too. The x86_64 library is built with ENABLE_ESMI_LIB, but the checker parsed amdsmi.h without it and skipped the CPU API: 69 functions, 12 types and the SPD and power-efficiency limits. Define it at the top of both copies of the header. Callers size buffers and check limits with amdsmi.h constants when they are built, so raising AMDSMI_MAX_MM_IP_COUNT from 8 to 16 breaks old binaries. The checker rates that Low, so the check passed it. Fail on any changed non-string constant in the binary compatibility report, which already leaves out AMDSMI_LIB_VERSION_*. Also: a size row no longer inherits the type name from an earlier section, merge_base() works on Python 3.6, and the tests cover High severity, symbol problems and Low type problems that are not size changes.
1 task done
Contributor
TheRock Submodule Bump ActivityNewest first
|
erman-gurses
pushed a commit
that referenced
this pull request
Nov 4, 2026
## Motivation Callers allocate most amdsmi structs and load the library by name, so a changed layout corrupts memory in programs built against the old header instead of failing to load. Recent examples: - #11741 grew `amdsmi_fabric_info_t` from 320 to 552 bytes. - #11821 grew `amdsmi_driver_info_t` from 768 to 1792 bytes. - #10226 grew `amdsmi_enumeration_info_t` from 276 to 280 bytes. All three landed within SONAME 27. #14425 made the existing `ABI Compliance Check` report such changes, but it only labels a PR and cannot block it. This PR also covers the follow-ups from #14425's review. ## Technical Details **ABI Compliance Check (#14425 follow-ups)** - `abi_check.py` exits 1 for a break and 2 when it could not compare. That covers a checker error, a missing merge base, an old header it cannot fetch, or a report without the 2.3 summary table. `abi-compliance-check.yml` records exit 2 as `error`, which leaves the labels unchanged instead of adding a false `MAJOR ABI BREAKAGE`. - It defines `ENABLE_ESMI_LIB` before parsing, so the CPU API (69 functions, 12 types) is checked. - It fails on a changed non-string constant, such as `AMDSMI_MAX_MM_IP_COUNT` going from 8 to 16. - Smaller fixes: - a size row no longer inherits a type name from an earlier section; - it works on Python 3.6; - the patterns point at `ACC_COMMIT`; - new tests cover High severity, symbol problems and Low type problems that are not size changes. **AMDSMI ABI Gate (new, blocking once required)** - `tests/abi_check/abi_gate.py` parses `amdsmi.h` with libclang and records: - each type's size, alignment and field offsets; - enumerator values and function signatures; - integer macros; - the units, sentinel values and in/out directions in field and parameter doc comments. It compares the PR with its base and the newest `release/therock-*` branch, each read from git. Additions pass, and so do fields carved from `reserved*` arrays or bitfields when the struct keeps its size. - An intended break is recorded with `abi_gate.py acknowledge --why "#NNNN: ..."`, which adds one line per change (path, old and new value, reason) to `projects/amdsmi/abi/abi_exceptions.json`, together with a CHANGELOG line naming it. The owners of `projects/amdsmi/` review both. The file starts with the three `%` units #13540 documented on develop that 10.2 does not have. - `.github/workflows/amdsmi-abi-gate.yml` runs on every PR to develop and `release/therock-*`, on merge groups, and on pushes to develop. - It has a read-only token and no secrets. - It runs the checker from the base commit, or from develop when the base has none, such as an older release branch. - There is no `paths:` filter, so it can become a required check. PRs that do not touch amdsmi's headers pass in seconds. **TheRock CI** - `.github/skip-ci-config.toml` skips the TheRock build for `projects/amdsmi/abi/*` and `projects/amdsmi/tests/abi_check/*`, which no build reads. This PR does not make the gate required. That is a ruleset change after an advisory period, together with a "require workflows" rule, because on pull requests GitHub runs the PR's own copy of the workflow file. ## Issue Tracking JIRA ID: TBD Follows up #14425. ## Test Plan - `abi_check_test.py`, `abi_gate_test.py` and the amdsmi pre-commit hooks. - `abi_check.py` with abi-compliance-checker 2.3 on real headers: no change, an enum value change, a header that does not compile, and a missing base ref. - Gate replay on the 73 develop commits that changed `amdsmi.h` in the last 12 months, the `release/therock-*` transitions, and 25 bypass fixtures. - The gate workflow run locally with `act` for same-repo and fork PRs, merge groups and pushes to develop. ## Test Result - 29 and 86 unit tests pass. - `abi_check.py` exits 0, 1, 2 and 2 for the four headers. - The gate blocks all 19 layout and API breaks in the replay and misses none. Among them: #11741, #11821, #13926, #8575, #10226, the `AMDSMI_LINK_TYPE_PCIE`/`XGMI` value swap, and the same-size `amdsmi_version_t` change. - It also blocks 4 doc-only unit corrections, which pass once acknowledged. - A PR that replaces the checker with `exit 0` still fails. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Signed-off-by: Maisam Arif <Maisam.Arif@amd.com> Co-authored-by: Maisam Arif <Maisam.Arif@amd.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The ABI compliance check never flags a break.
check_report_content()matches text that abi-compliance-checker 2.3 does not print, so every report reads as clean, and major mode passes with a warning even when the checker exits 1. It passed #8575 (removedamdsmi_gpu_driver_reload) and #11821 (grewamdsmi_driver_info_tfrom 768 to 1792 bytes, reverted in #13926).Technical Details
Report parser (
tests/abi_check/abi_check.py):<h2>Removed Symbols <span class='failed'> 1 </span>)Size of this type has been changedrow, naming the typeBaseline (
tests/abi_check/abi_check.py):git merge-base <base> HEADinstead of the base branch tip. Without this, once the check can fail, a PR that forked before an API landed ondevelopis reported as removing itTests and docs:
tests/abi_check/abi_check_test.py: fixtures are fragments of real abi-compliance-checker 2.3 reports; adds merge-base and pass/fail tests.pre-commit-config.yaml: runabi_check_test.pywhentests/abi_check/changes, so the LSTT Formatting job runs it; no workflow ran it beforetests/README.md: describe bothNot changed:
abi-compliance-check.yml. The check stays non-blocking and still reports through theMAJOR ABI BREAKAGE/MINOR ABI BREAKAGElabels. The workflow runs the base branch'sabi_check.py, so this PR's own ABI jobs still use the old parser.Issue Tracking
Related: #11821, #13926
Test Plan
abi_check_test.pyon Python 3.6, 3.10 and 3.14, againstdevelop'sabi_check.pyand this branchdevelop's and this branch'sabi_check.pywith abi-compliance-checker 2.3 on Ubuntu 22.04 (the CI job's tool and OS), major and minor mode, on realamdsmi.hpairs, each in a throwaway git repo sogit merge-baseandgit showbehave as in CIpre-commit run --from-ref origin/develop --to-ref HEADfromprojects/amdsmi, as the LSTT Formatting job runs itTest Result
abi_check_test.py: the new tests fail ondevelop'sabi_check.py(3 failures, 5 errors) and all 16 pass with this change on every Python versionReal abi-compliance-checker 2.3 runs, exit code (major / minor):
amdsmi.hchangedevelopamdsmi_gpu_driver_reloadamdsmi_driver_info_tamdsmi_driver_info_t768 to 1792 bytesrelease/therock-10.0torelease/therock-10.1amdsmi_enumeration_info_t276 to 280 bytesrelease/therock-10.1torelease/therock-10.2amdsmi_driver_info_t,amdsmi_fabric_info_tand itsfabric_info_union grewAMDSMI_LIB_VERSION_MINORbumpMinor mode keeps its meaning (any change the checker reports with
-strict); only its baseline changed.Submission Checklist