Skip to content

fix(amdsmi) Fix backwards includes in rocm_smi.h - #10853

Merged
marifamd merged 2 commits into
ROCm:developfrom
asasfu:patch-2
Aug 31, 2026
Merged

marifamd merged 2 commits into
ROCm:developfrom
asasfu:patch-2

Conversation

@asasfu

@asasfu asasfu commented Aug 27, 2026

Copy link
Copy Markdown

The includes for cplusplus and C were backwards, and extern C is redundant in this case, I believe.

Motivation

Fixes compilation errors or warnings for rocm_smi

Technical Details

Issue Tracking

Test Plan

Test Result

Submission Checklist

@asasfu
asasfu requested a review from a team August 27, 2026 21:16
@therock-pr-bot

Copy link
Copy Markdown

❌ PR Check — Action Required

Check Status Details
📝 PR Description ❌ Fail Error: PR description must reference a JIRA ID, ISSUE ID, or a GitHub closing keyword.
Expected: include a JIRA ID / ISSUE ID line (separator : or -, or omitted; value may be a JIRA key, a number with/without #, or a link), OR a closing keyword + issue reference. Accepted examples:
• JIRA ID : TESTAUTO-6039
• JIRA ID - #330
• JIRA ID #330
• JIRA ID (on separate line)
ROCM-25757
• ISSUE ID : TESTUTO-3334
• ISSUE ID #3334
• ISSUE ID - TESTAUTO-3433
• ISSUE ID (on separate line)
AIRUNTIME-2352
• ISSUE ID : https://github.com/<org_name>/<repo_name>/issues/1234
• Closes #10
• Fixes octo-org/octo-repo#100
• Resolves: #123
• #123
• https://github.com/<org_name>/<repo_name>/issues/123
Current: no valid JIRA/ISSUE/closing-keyword reference found
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ⚠️ Warning Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h; no test file found
🔎 pre-commit ⏳ Pending ⏳ Still running…
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

⚠️ 1 policy check(s) failed. Please address the issues above before this PR can be Reviewed.

🚫 Please fix the failed policies

  • ❌ PR Description

The Not ready to Review label was added to this PR. Once all policies pass, the label is removed automatically.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot

Copy link
Copy Markdown

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ PR Description

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

@dmitrii-galantsev dmitrii-galantsev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

claude:

Correct diagnosis of a real bug — the #ifdef __cplusplus include branches were genuinely backwards and broke C consumers. Approving with b8a5620 layered on top, which restores the extern "C" { that had been dropped while its closing brace remained; the header now compiles clean as both C11 and C++17 with all rsmi_* symbols verified at C linkage.

@asasfu

asasfu commented Aug 28, 2026

Copy link
Copy Markdown
Author

Thank you @dmitrii-galantsev I rushed this PR since I knew it was an issue but was headed away for the weekend. Thanks for validating and fixing the PR. I couldn't figure out where the matching } was for extern C {. It seemed that it might have made sense to remove it. Your changes make sense.

Glad to see you glance at this half baked PR since it was actually needed.

@dmitrii-galantsev

Copy link
Copy Markdown
Contributor

yeah no prob! Whatever makes the code better! I'll take a half baked PR over an issue any day :)

we're fighting some gremlins right now, so CI might look broken. it's only partially true

@dmitrii-galantsev

Copy link
Copy Markdown
Contributor

my bad meant to rebase. maybe i wont touch more PRs on friday.

@marifamd marifamd changed the title rocm_smi.h - Fix backwards includes fix(amdsmi) Fix backwards includes in rocm_smi.h Aug 29, 2026
asasfu and others added 2 commits August 31, 2026 12:46
The includes for cplusplus and C were backwards, and extern C is redundant in this case, I believe.
The include-block swap is correct, but the opening extern "C" { was
removed while its closing brace remained at the end of the header, so
every C++ translation unit including this public header hit a stray '}'.
It cannot simply go back where it was: the C++ branch now includes
<cstddef>/<cstdint>, which must not be pulled in under C language
linkage. Reopening the block after the includes keeps all rsmi_*
declarations at C linkage and avoids an ABI break from name mangling.
@marifamd
marifamd merged commit 7db675a into ROCm:develop Aug 31, 2026
44 of 45 checks passed
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants