Repository navigation
build(amdsmi): auto-detect amdclang++/clang++ as default compiler - #5047
sumanth-gavini wants to merge 10 commits into
Conversation
oliveiradan
left a comment
There was a problem hiding this comment.
@sumanth-gavini, I added a few comments for you to go over.
@bill-shuzhou-liu, @marifamd, any comments?
oliveiradan
left a comment
There was a problem hiding this comment.
@sumanth-gavini, a few more points to check.
@oliveiradan Addressed review comments |
b40b93f to
95120f6
Compare
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
|
The work on this PR has been moved to: |
3bcb25f to
c292c4b
Compare
c292c4b to
11b791a
Compare
11b791a to
6481284
Compare
marifamd
left a comment
There was a problem hiding this comment.
Pending CI to include amdclang to merge
6481284 to
7b47397
Compare
7b47397 to
c58b235
Compare
- Detect the C/C++ compiler before project(): prefer amdclang++, then clang++, discovered via `hipconfig --path` with no hardcoded ROCm paths - Search ROCm dirs only (NO_DEFAULT_PATH) when ROCm is present; use system PATH only when ROCm is absent, enforcing strict ROCm/system separation - Report ROCm vs system clang from ROCM_DIR instead of a path regex match - Fail with a compiler hint scoped to the case: ROCm LLVM component when ROCm is installed, system clang when it is not - Add cmake/toolchains/amdsmi-gcc-toolchain.cmake for opt-in GCC builds - Document compiler discovery and alternatives in README and docs/install/build Co-authored-by: Sumanth Gavini <sumanth.gavini@amd.com> Signed-off-by: Oliveira, Daniel <daniel.oliveira@amd.com>
c58b235 to
72db32d
Compare
adam360x
left a comment
There was a problem hiding this comment.
I built this PR on a ROCm 7.3 host and went through its CI runs. The default amdclang++ build and the GCC toolchain-file build both compile there and pass the unit tests. Five issues should be fixed before merge; each comment below has the evidence and a suggested fix, tried locally where I could.
| if not all(shutil.which(t) for t in ("cmake", "make", "pkg-config", "git", "rpmbuild")): | ||
| return False | ||
| if not (shutil.which("gcc") or shutil.which("cc")): | ||
| if not (shutil.which("amdclang++") or shutil.which("clang++")): |
There was a problem hiding this comment.
AMDSMI CI Ubuntu20 and AzureLinux3 now fail at configure
Both legs stop at CMakeLists.txt:130 with No suitable C++ compiler found (amdclang++ or clang++), right after hipconfig found: HIPCONFIG_EXECUTABLE-NOTFOUND (Ubuntu20, AzureLinux3). On develop they build with GCC 9.4 and 13.2.
This check passed on both images (their logs have no Installing build prerequisites), so the script installed nothing. The check doesn't match what CMake needs: without hipconfig, CMake wants both clang and clang++ on PATH, and with hipconfig it only looks in the ROCm LLVM directories. So amdclang++ on PATH, or clang++ without clang, passes here and still fails at configure.
Suggested fix:
- Build these two legs with GCC. The script has no way to pass
-DCMAKE_TOOLCHAIN_FILEtoday, so this needs a small option (orCC/CXX, once they're honored; see my comment on CMakeLists.txt line 35). After this PR no other leg in this workflow builds with GCC (Debian 12/13 move to distro clang, the other six to amdclang++), so this also keeps the GCC path tested. - Make this check mirror the CMake rule.
- Update the docstring at line 19, which still lists gcc/g++ as a valid toolchain.
| git config --global --add safe.directory "$GITHUB_WORKSPACE" | ||
| apt-get install -y --no-install-recommends \ | ||
| build-essential cmake ninja-build pkg-config python3 \ | ||
| build-essential clang cmake ninja-build pkg-config python3 \ |
There was a problem hiding this comment.
Both ASAN jobs now fail, and a clang build can't run the later ASAN steps
With clang installed and no hipconfig in this image, configure picks /usr/bin/clang++ (clang 18). --no-install-recommends leaves out clang's sanitizer runtime, so anything linked with -fsanitize=address fails to link and configure stops at Could NOT find Threads (unit, GPU). Both jobs are green on develop, which builds them with GCC.
I tried the next step in a local ubuntu:24.04 container with the same packages. Adding libclang-rt-18-dev fixes configure and build, but then:
help_package.cmakeadds-shared-libsanfor Clang, soamdsmitstcan't start (libclang_rt.asan-x86_64.sonot found).- The CLI step preloads GCC's libasan (lines 197-199), and
amd-smithen can't loadlibamd_smi.so.27, which needs the clang runtime. - The header (line 11) still says both jobs build with GCC.
Suggested fix: keep these jobs on GCC. Add -DCMAKE_TOOLCHAIN_FILE="$GITHUB_WORKSPACE/projects/amdsmi/cmake/toolchains/amdsmi-gcc-toolchain.cmake" to both Configure steps and drop clang here and at line 152. Moving ASAN to clang would instead need the runtime package, the clang runtime directory on the rpath or LD_LIBRARY_PATH, a clang runtime in the CLI preload, and a header update.
The documented local ASAN build (cmake -DADDRESS_SANITIZER=ON ..) has the same problem: on a ROCm host it now picks amdclang++, and amdsmitst won't start without LD_LIBRARY_PATH=<rocm>/lib/llvm/lib/clang/22/lib/linux. On develop the GCC build runs directly.
| # See: cmake/toolchains/amdsmi-gcc-toolchain.cmake | ||
| # See also: https://github.com/ROCm/TheRock/issues/3976 | ||
| # | ||
| if(NOT CMAKE_TOOLCHAIN_FILE AND NOT DEFINED CMAKE_CXX_COMPILER AND NOT DEFINED CMAKE_C_COMPILER) |
There was a problem hiding this comment.
CC/CXX are silently ignored
CMake only reads CC/CXX when CMAKE_C_COMPILER/CMAKE_CXX_COMPILER aren't already set at project(). This block sets them, so CC=gcc CXX=g++ cmake .. still builds with amdclang++ (develop honors CC/CXX). Those variables are the standard way to pick a compiler in distro, packaging and CI scripts. The AMDSMI CI images for Ubuntu22, RHEL8/9/10, SLES and AlmaLinux8 set them to the /usr/local/bin/amdsmi-cc/amdsmi-cxx wrappers; with this PR those legs bypass the wrappers and use /opt/rocm-7.13.0/lib/llvm/bin/amdclang++ instead.
Suggested fix (gersemi 0.26.0 format). I tried it locally: CC/CXX are honored again and the default still picks amdclang++.
| if(NOT CMAKE_TOOLCHAIN_FILE AND NOT DEFINED CMAKE_CXX_COMPILER AND NOT DEFINED CMAKE_C_COMPILER) | |
| if( | |
| NOT CMAKE_TOOLCHAIN_FILE | |
| AND NOT DEFINED CMAKE_CXX_COMPILER | |
| AND NOT DEFINED CMAKE_C_COMPILER | |
| AND NOT DEFINED ENV{CXX} | |
| AND NOT DEFINED ENV{CC} | |
| ) |
| " 2. Or install ROCm from https://rocm.docs.amd.com/\n" | ||
| ) | ||
| endif() | ||
| message( |
There was a problem hiding this comment.
The internal Zuul build is expected to stop here after merge
The internal Zuul build-amdsmi job runs a bare cmake ../ -DBUILD_TESTS=ON -DENABLE_ESMI_LIB=ON in the amdsmi-ubuntu-26.04 image. That image's Dockerfile installs build-essential, g++ and cmake, but no clang and no ROCm/hipconfig. Unless its base image already has clang (I didn't check that), configure will hit this error, and the eight Zuul GPU test jobs that consume its packages would have no build to test.
The Zuul side needs to change before this merges: pass the GCC toolchain file there, or add clang to the image.
| endif() | ||
|
|
||
| if(AMDCLANG_CXX_COMPILER AND AMDCLANG_C_COMPILER) | ||
| set(CMAKE_CXX_COMPILER "${AMDCLANG_CXX_COMPILER}" CACHE FILEPATH "C++ compiler") |
There was a problem hiding this comment.
Clang as the default drops the linker hardening flags
generic_package() in cmake_modules/help_package.cmake appends flags with set(X "${X} ..." PARENT_SCOPE) 13 times. PARENT_SCOPE doesn't change the function's own copy of X, so each write starts again from the original value and only the last write for each variable survives:
- Clang: the build-id block (lines 79-81) is the last linker-flag write, so
-Wl,-z,noexecstack -Wl,-z,relro -Wl,-z,nownever reach the link line. - GCC:
-Werror=stringop-overflowis the lastCMAKE_CXX_FLAGSwrite, so-D_FORTIFY_SOURCE=2 -fstack-protector-all -Wtrampolinesare dropped.
The bug predates this PR (TheRock's build of the library already lacks BIND_NOW), but this PR makes Clang the default everywhere else, so default builds go from full to partial RELRO. The wheels show it: libamd_smi_python.so in this PR's manylinux wheel (clang 21) has no BIND_NOW; develop's wheel (GCC 14) has it.
Suggested fix: append to the local copy, then export it (string(APPEND ...) followed by set(... PARENT_SCOPE)) in all 13 places. With that, both compilers keep every flag (BIND_NOW is back) and the unit tests pass (237 passed, 18 root-only skips).
Diff for cmake_modules/help_package.cmake
--- a/projects/amdsmi/cmake_modules/help_package.cmake
+++ b/projects/amdsmi/cmake_modules/help_package.cmake
@@ -30,9 +30,11 @@
endif()
if("${CMAKE_BUILD_TYPE}" STREQUAL Release)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O2" PARENT_SCOPE)
+ string(APPEND CMAKE_CXX_FLAGS " -O2")
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}" PARENT_SCOPE)
else()
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -ggdb -O0 -DDEBUG" PARENT_SCOPE)
+ string(APPEND CMAKE_CXX_FLAGS " -ggdb -O0 -DDEBUG")
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}" PARENT_SCOPE)
endif()
# Add address sanitizer
@@ -51,34 +53,45 @@
else()
set(ASAN_LINKER_FLAGS "${ASAN_LINKER_FLAGS} -static-libsan")
endif()
- set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${ASAN_COMPILER_FLAGS}" PARENT_SCOPE)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${ASAN_COMPILER_FLAGS}" PARENT_SCOPE)
- set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${ASAN_LINKER_FLAGS}" PARENT_SCOPE)
- set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ${ASAN_LINKER_FLAGS}" PARENT_SCOPE)
+ string(APPEND CMAKE_C_FLAGS " ${ASAN_COMPILER_FLAGS}")
+ set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS}" PARENT_SCOPE)
+ string(APPEND CMAKE_CXX_FLAGS " ${ASAN_COMPILER_FLAGS}")
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}" PARENT_SCOPE)
+ string(APPEND CMAKE_EXE_LINKER_FLAGS " ${ASAN_LINKER_FLAGS}")
+ set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS}" PARENT_SCOPE)
+ string(APPEND CMAKE_SHARED_LINKER_FLAGS " ${ASAN_LINKER_FLAGS}")
+ set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}" PARENT_SCOPE)
else()
## Security breach mitigation flags
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D_FORTIFY_SOURCE=2 -fstack-protector-all -Wcast-align" PARENT_SCOPE)
+ string(APPEND CMAKE_CXX_FLAGS " -D_FORTIFY_SOURCE=2 -fstack-protector-all -Wcast-align")
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}" PARENT_SCOPE)
## More security breach mitigation flags
set(HARDENING_LDFLAGS "${HARDENING_LDFLAGS} -Wl,-z,noexecstack -Wl,-z,relro -Wl,-z,now")
- set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${HARDENING_LDFLAGS}" PARENT_SCOPE)
- set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ${HARDENING_LDFLAGS}" PARENT_SCOPE)
+ string(APPEND CMAKE_EXE_LINKER_FLAGS " ${HARDENING_LDFLAGS}")
+ set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS}" PARENT_SCOPE)
+ string(APPEND CMAKE_SHARED_LINKER_FLAGS " ${HARDENING_LDFLAGS}")
+ set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}" PARENT_SCOPE)
include(CheckCXXCompilerFlag)
check_cxx_compiler_flag("-Wtrampolines" CXX_SUPPORTS_WTRAMPOLINES)
if(CXX_SUPPORTS_WTRAMPOLINES)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wtrampolines" PARENT_SCOPE)
+ string(APPEND CMAKE_CXX_FLAGS " -Wtrampolines")
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}" PARENT_SCOPE)
endif()
check_cxx_compiler_flag("-Werror=stringop-overflow" CXX_SUPPORTS_STRINGOP_OVERFLOW)
if(CXX_SUPPORTS_STRINGOP_OVERFLOW)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror=stringop-overflow" PARENT_SCOPE)
+ string(APPEND CMAKE_CXX_FLAGS " -Werror=stringop-overflow")
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}" PARENT_SCOPE)
endif()
endif()
# Clang does not set the build-id
# similar to if(NOT CMAKE_COMPILER_IS_GNUCC)
if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
- set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,--build-id=sha1" PARENT_SCOPE)
- set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,--build-id=sha1" PARENT_SCOPE)
+ string(APPEND CMAKE_EXE_LINKER_FLAGS " -Wl,--build-id=sha1")
+ set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS}" PARENT_SCOPE)
+ string(APPEND CMAKE_SHARED_LINKER_FLAGS " -Wl,--build-id=sha1")
+ set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}" PARENT_SCOPE)
endif()
# configure packaging- Skip the amdclang++/clang++ search when CC or CXX is set; the block set CMAKE_<LANG>_COMPILER first, so CMake never read them - Keep the compilers that CI images, packaging scripts and users pick through the environment, as develop does - Document CC/CXX next to the toolchain file and explicit compilers Signed-off-by: Adam360x <Adam.pryor@amd.com>
- Configure both jobs with amdsmi-gcc-toolchain.cmake: the image's clang ships no sanitizer runtime, so configure failed, and the CLI step preloads GCC's libasan - Drop the clang package the jobs no longer use Signed-off-by: Adam360x <Adam.pryor@amd.com>
- Rename the search's ROCm path to _AMDSMI_ROCM_PATH: as ROCM_DIR it shadowed the ROCM_DIR cache entry for the rest of the first configure, so the examples used the hipconfig path, or an empty one, instead of -DROCM_DIR - Shorten the block comment to the policy and point at the CI script that mirrors it - List CC/CXX among the alternatives in the configure error Signed-off-by: Adam360x <Adam.pryor@amd.com>
- Accumulate the ASAN, hardening and build-id linker flags and export them once: each set(... PARENT_SCOPE) started again from the caller's value, so the Clang build-id block dropped -z now, relro and noexecstack, and Clang builds lost BIND_NOW - Leave the compile-flag writes alone: fixing them adds -ggdb -O0 -DDEBUG to every build without CMAKE_BUILD_TYPE, including CI's Signed-off-by: Adam360x <Adam.pryor@amd.com>
- Pass -D_FORTIFY_SOURCE=2 only when the C++ flags ask for optimization: builds without CMAKE_BUILD_TYPE have no -O, glibc on RHEL-family distributions then warns about the macro, and the -Werror AI NIC library fails; Clang keeps this flag, unlike GCC here - Keep the flag for release builds and for CXXFLAGS with -O Signed-off-by: Adam360x <Adam.pryor@amd.com>
- Check for the compiler pair the way CMakeLists.txt searches for it, and run it: the Ubuntu 20.04 and AzureLinux 3 images have an /usr/local/bin/amdclang++ ccache link but no compiler behind it, so the PATH check passed, nothing was installed, and configure failed - Install only the distro clang, since the full prerequisite list would also replace the images' gtest and cmake, and skip it when hipconfig finds ROCm, whose LLVM is then the only place CMake searches - Take clang back out of the prerequisite lists: install_clang() covers it, and one unavailable package fails the whole list - Restore the gcc/cc check for the base toolchain, which clang needs for libstdc++ and the C runtime - Share the dnf/tdnf install command with the netlink step Signed-off-by: Adam360x <Adam.pryor@amd.com>
- Clear CC/CXX on the Ubuntu22 leg so one ROCm image builds with the amdclang++ that CMake finds; the other ROCm images keep their compiler wrappers - Run the run_amdsmi_build.py unit tests once, in a hosted job, so a failure there cannot skip the GPU steps Signed-off-by: Adam360x <Adam.pryor@amd.com>
- Match the compiler search: both compilers of a pair must run, and there is no system fallback once hipconfig finds ROCm - Quote the real configure error and cover hosts without clang - Add Clang to the required software and keep g++ for its C++ library - State the CMake version the build requires (3.20) - Point README at the build docs instead of repeating them - Show how to put the Clang ASAN runtime on the library path; with amdclang++ as the default, amdsmitst does not start without it - List clang in build_wheel.py's requirements Signed-off-by: Adam360x <Adam.pryor@amd.com>
- Note that source builds now use amdclang++ or clang++ and how to keep building with GCC Signed-off-by: Adam360x <Adam.pryor@amd.com>
Add automatic compiler selection: amdclang++ → clang++ → default clang++. Searches ROCm paths for amdclang++, falls back to system compilers. Shows warning when using gcc. Respects user override flag.
Motivation
Building amd-smi meant passing a compiler to CMake by hand. CMake now selects the
compiler itself: it prefers ROCm's amdclang++, falls back to clang++, keeps ROCm
and system toolchains separate, and leaves GCC as an opt-in toolchain file.
Technical Details
Compiler detection (
CMakeLists.txt):project(); priority amdclang++ > clang++hipconfig --path; no hardcoded/opt/rocmorROCM_PATHNO_DEFAULT_PATH), never system PATH--version) before using itGCC toolchain (
cmake/toolchains/amdsmi-gcc-toolchain.cmake):Docs (
README.md,docs/install/build.md):JIRA ID
JIRA ID: AILITOOLS-280
Test Plan
cmake -S . -B buildselects amdclang++-DCMAKE_CXX_COMPILER=skips detection and honors the overrideTest Result
Test 1: With everything (uses amdclang++)
cd rocm-systems/projects/amdsmi
rm -rf build && mkdir build && cd build
cmake .. 2>&1 | grep "Using.*compiler"
Output: Using AMD Clang compiler: /opt/rocm/llvm/bin/amdclang++
dev@dev:dev amdsmi ±|amdsmi_clang ✗|→ rm -rf build && mkdir build && cd build
cmake .. 2>&1 | grep "Using.*compiler"
mkdir: created directory 'build'
/rocm-systems/projects/amdsmi/build
-- Using AMD Clang compiler: /opt/rocm/llvm/bin/amdclang++
Test 2: No ROCm (uses clang++)
rm -rf ../build && mkdir ../build && cd ../build
ROCM_PATH=/nonexistent cmake .. 2>&1 | grep "Using.*compiler"
Output: Using system Clang compiler: /usr/bin/clang++
dev@dev: build ±|amdsmi_clang ✗|→ ROCM_PATH=/nonexistent cmake .. 2>&1 | grep "Using.*compiler"
-- Using system Clang compiler: /usr/bin/clang++
Submission Checklist