Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hsa-runtime: set underlying type of hsa_region_info_t, hsa_amd_region_info_t to int #274

Open
wants to merge 1 commit into
base: amd-staging
Choose a base branch
from

Conversation

LunNova
Copy link
Contributor

@LunNova LunNova commented Jan 1, 2025

When an enum's backing type is not set explicitly it is undefined behavior to load a value that is not one within the range of values it was defined with after https://cplusplus.github.io/CWG/issues/1766.html

This causes UB because ROCR-Runtime casts back and forth between hsa_amd_region_info_s and hsa_region_info_t types.

region, (hsa_region_info_t)HSA_AMD_REGION_INFO_HOST_ACCESSIBLE,

Fixes one of the errors reported in #272:

/build/source/runtime/hsa-runtime/core/runtime/amd_memory_region.cpp:206:11: runtime error: load of value 40961, which is not a valid value for type 'hsa_region_info_t'
    #0 0x7fff754f159e in GetInfo /build/source/runtime/hsa-runtime/core/runtime/amd_memory_region.cpp:206
    #1 0x7fff75504228 in hsa_region_get_info /build/source/runtime/hsa-runtime/core/runtime/hsa.cpp:1077
    #2 0x7fff75700018 in Initialize /build/source/runtime/hsa-runtime/image/image_manager_kv.cpp:179

Copy link
Collaborator

@dayatsin-amd dayatsin-amd left a comment

Choose a reason for hiding this comment

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

Please change commit subject to:
rocr: set underlying type of hsa_region_info_t, hsa_amd_region_info_t

And wrap commit message body at 72 characters.
Otherwise looks good to me

When an enum's backing type is not set explicitly it is undefined
behavior to load a value that is not one of the specific values
it was defined with.

See https://eel.is/c++draft/expr.static.cast#9
@LunNova LunNova force-pushed the lunnova/fix-ub-region-info branch from 39a6a16 to 441137d Compare January 25, 2025 17:23
@LunNova LunNova requested a review from dayatsin-amd January 25, 2025 17:24
@LunNova
Copy link
Contributor Author

LunNova commented Jan 25, 2025

@dayatsin-amd Commit message and title updated

@dayatsin-amd
Copy link
Collaborator

Thank you @LunNova we will submit this patch from out internal repo.

@dayatsin-amd
Copy link
Collaborator

!verify full

@dayatsin-amd
Copy link
Collaborator

!verify

@dayatsin-amd
Copy link
Collaborator

We are getting some compiler errors internally due to this change. Will have to re-visit this.

@dayatsin-amd
Copy link
Collaborator

@LunNova I wrapped the defines because we get errors when compiling C applications.

Can you check the PR here:
#289

@LunNova
Copy link
Contributor Author

LunNova commented Feb 4, 2025

@dayatsin-amd Looks good, approved the PR you opened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants