- Notifications
You must be signed in to change notification settings - Fork3.5k
[AIX]Blocking the call of dladdr under _AIX#26513
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
ranjitshs commentedNov 6, 2025
@tianleiwu@snnn@edgchen1@yuslepukhin |
| } | ||
| PathStringGetRuntimePath()constoverride { | ||
| // In AIX, dladdr is not supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
is there another way to get the current shared library path on AIX?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No. I don't see any way to get these shared library paths in AIX.
| }(); | ||
| constauto* vendor_info =FindCpuVendorInfo(vendor); | ||
| // Do below logging only if CPUINFO_SUPPORTED by the OS and still vendor_info is not available. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think that it is an implementation detail that the vendor info lookup depends on cpuinfo. I also think that if the lookup is unsuccessful, it is worth logging.
do you think that this information should be logged at a different level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Are you suggesting to use the same code but with INFO level of message ?
is it okay to define like below and use in case of AIX and if vendor_info is NULL after calling FindCpuVendorInfo()
constexpr auto kIBMCpuVendorInfo = CpuVendorInfo{cpuinfo_vendor_ibm, "IBM", 0x0000};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Are you suggesting to use the same code but with INFO level of message ?
I'm suggesting keeping the logging. I am fine with changing the level.
is it okay to define like below and use in case of AIX and if vendor_info is NULL after calling FindCpuVendorInfo()
constexpr auto kIBMCpuVendorInfo = CpuVendorInfo{cpuinfo_vendor_ibm, "IBM", 0x0000};
doesdefined(_AIX) only apply to IBM CPUs? if so, then I think doing something like this is fine. perhaps an entry can be added to thekCpuVendorInfos table, andvendor in this function can be initialized tocpuinfo_vendor_ibm ifdefined(_AIX). also, the vendor ID can be the IBM PCI vendor ID value of0x1014 instead of 0x0000.
| ${onnxruntime_runtime_path_test_shared_library_src}) | ||
| if (CMAKE_SYSTEM_NAMEMATCHES"AIX") | ||
| target_link_libraries(onnxruntime_runtime_path_test_shared_libraryPRIVATE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
please indent theif()/else() blocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I will update in next commit.
| ASSERT_TRUE(std::filesystem::is_regular_file(canonical_shared_library_path)); | ||
| } | ||
| #else | ||
| TEST(GetRuntimePathFromSharedLibraryTest, Basic) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if GetRuntimePath() returns an empty path, it is not a valid one. perhaps the documentation needs to be updated, but I believe at least one place is relying on the returned path to be absolute.
onnxruntime/onnxruntime/core/session/utils.cc
Lines 419 to 427 in760eea4
| if (!resolved_library_path.is_absolute()) { | |
| resolved_library_path =Env::Default().GetRuntimePath() /std::move(resolved_library_path); | |
| } | |
| // if it's a provider bridge library we need to create ProviderLibrary first to ensure the dependencies are loaded | |
| // like the onnxruntime_provider_shared library. | |
| auto provider_library = std::make_unique<ProviderLibrary>(resolved_library_path.native().c_str(), | |
| true, | |
| ProviderLibraryPathType::Absolute); |
if it is unsupported on AIX, I think it's not worth testing that it returns an empty path and we can just skip this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Okay. I will update and skip this test.
Uh oh!
There was an error while loading.Please reload this page.
Description
In AIX, dladdr() is not supported so blocking the call of dladdr API under _AIX.
we don't have support of cpuinfo pkg also which generates a warning at runtime.
This PR is to fox the issues mentioned above.
Motivation and Context
2025-11-06 07:23:44.176700000 [W:onnxruntime:Default, cpuid_info.cc:95 LogEarlyWarning] Unknown CPU vendor. cpuinfo_vendor value: 0