Fix xilinx_xrt() root discovery on Debian multiarch installs - #9955
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes Linux XRT install-root discovery when XRT is installed using Debian multiarch layouts (e.g. lib/x86_64-linux-gnu). It updates xilinx_xrt() to derive the root by stripping the number of path components in XRT_LIB_DIR rather than assuming a fixed .../lib/... depth, avoiding doubled-path construction that breaks module loading.
Changes:
- Add a default for
XRT_LIB_DIRand document multiarch library layouts. - Update
xilinx_xrt()to strip as many directory components asXRT_LIB_DIRcontains when deriving the XRT root.
Suppressed comments (1)
src/runtime_src/core/common/detail/linux/xilinx_xrt.h:79
- This loop currently uses XRT_LIB_DIR directly; if you switch to a local fallback (see earlier comment), update the loop to use that value so the code doesn’t rely on the macro being defined by this header.
for (const auto& comp : sfs::path(XRT_LIB_DIR)) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
991e792 to
813207b
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
stsoe
left a comment
There was a problem hiding this comment.
Yeah, true, the code was wrong. Thanks for fixing.
A few suggestions, one being a corner case.
xilinx_xrt() derived the XRT root by taking two parent_path() levels up from the libxrt_coreutil.so path reported by dladdr(). This assumes coreutil lives directly in <root>/lib, which only holds when CMAKE_INSTALL_LIBDIR is a single path component (lib, lib64). On a Debian multiarch install CMAKE_INSTALL_LIBDIR is lib/x86_64-linux-gnu, so coreutil is one level deeper and the two-level strip returns <root>/lib instead of <root>. shim_path() then re-appends XRT_LIB_DIR, producing a doubled path such as '/opt/amdgpu/lib/lib/x86_64-linux-gnu/libxrt_core.so.2' and the load fails with "No such library". Strip exactly as many trailing components as XRT_LIB_DIR contains instead of a hardcoded two levels, so the root is recovered correctly for lib, lib64, and lib/x86_64-linux-gnu layouts alike. Single-component layouts (standard, lib64, single-arch Yocto) are unaffected. Also default XRT_LIB_DIR to "lib" so the header stays self-contained; the loop additionally tolerates an empty value. Regression from 11169ba (SWSPLAT-24084) which switched XRT root discovery from XILINX_XRT to dladdr(). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Wendy Liang <wendy.liang@amd.com>
813207b to
30bf654
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
…9958) xilinx_xrt() derived the XRT root by taking two parent_path() levels up from the libxrt_coreutil.so path reported by dladdr(). This assumes coreutil lives directly in <root>/lib, which only holds when CMAKE_INSTALL_LIBDIR is a single path component (lib, lib64). On a Debian multiarch install CMAKE_INSTALL_LIBDIR is lib/x86_64-linux-gnu, so coreutil is one level deeper and the two-level strip returns <root>/lib instead of <root>. shim_path() then re-appends XRT_LIB_DIR, producing a doubled path such as '/opt/amdgpu/lib/lib/x86_64-linux-gnu/libxrt_core.so.2' and the load fails with "No such library". Strip exactly as many trailing components as XRT_LIB_DIR contains instead of a hardcoded two levels, so the root is recovered correctly for lib, lib64, and lib/x86_64-linux-gnu layouts alike. Single-component layouts (standard, lib64, single-arch Yocto) are unaffected. Also default XRT_LIB_DIR to "lib" so the header stays self-contained; the loop additionally tolerates an empty value. Regression from 11169ba (SWSPLAT-24084) which switched XRT root discovery from XILINX_XRT to dladdr(). (cherry picked from commit 5403f66) Signed-off-by: Wendy Liang <wendy.liang@amd.com> Co-authored-by: Wendy Liang <wendy.liang@amd.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
xilinx_xrt() derived the XRT root by taking two parent_path() levels up from the libxrt_coreutil.so path reported by dladdr(). This assumes coreutil lives directly in /lib, which only holds when CMAKE_INSTALL_LIBDIR is a single path component (lib, lib64). On a Debian multiarch install CMAKE_INSTALL_LIBDIR is lib/x86_64-linux-gnu, so coreutil is one level deeper and the two-level strip returns /lib instead of . shim_path() then re-appends XRT_LIB_DIR, producing a doubled path such as
'/opt/amdgpu/lib/lib/x86_64-linux-gnu/libxrt_core.so.2' and the load fails with "No such library".
Strip exactly as many trailing components as XRT_LIB_DIR contains instead of a hardcoded two levels, so the root is recovered correctly for lib, lib64, and lib/x86_64-linux-gnu layouts alike. Single-component layouts (standard, lib64, single-arch Yocto) are unaffected. Also default XRT_LIB_DIR to "lib" so the header stays self-contained; the loop additionally tolerates an empty value.
Regression from 11169ba (SWSPLAT-24084) which switched XRT root discovery from XILINX_XRT to dladdr().
Problem solved by the commit
Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered
How problem was solved, alternative solutions (if any) and why they were rejected
Risks (if any) associated the changes in the commit
What has been tested and how, request additional testing if necessary
Documentation impact (if any)