Repository navigation
Replace hardcoded MSVC debug flags with CMAKE_MSVC_DEBUG_INFORMATION_FORMAT - #1113
zichguan-amd wants to merge 2 commits into
Conversation
…FORMAT Signed-off-by: zichguan-amd <zichuan.guan@amd.com>
ScottTodd
left a comment
ScottTodd
left a comment
There was a problem hiding this comment.
LGTM, will let others weigh in on whether this CMake min version update is okay for CLR (there are ongoing discussions about the minimum version across all ROCm projects, FYI)
|
Would also need CI to update CMake version so that Azure pipelines run |
|
This pull request has been inactive for 25 days and will be marked as stale. If you would like to keep this PR open, please:
This PR will be automatically closed in 5 days if no further activity occurs. |
There was a problem hiding this comment.
Pull request overview
This PR modernizes MSVC debug info handling in the CLR subprojects by switching from hardcoded /Zi (and linker debug flags) to CMake’s CMAKE_MSVC_DEBUG_INFORMATION_FORMAT (introduced with CMake 3.25 / CMP0141), primarily to improve compatibility with compilation caches like ccache/sccache.
Changes:
- Bump
projects/clrto require CMake 3.25 (to enable CMP0141 behavior). - Replace hardcoded MSVC
/Zi(and/DEBUG:FULL) flags withCMAKE_MSVC_DEBUG_INFORMATION_FORMATinopenclandhipamd.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| projects/clr/CMakeLists.txt | Raises the CLR project’s minimum CMake version to 3.25 to support CMP0141/MSVC debug format selection. |
| projects/clr/opencl/CMakeLists.txt | Uses CMAKE_MSVC_DEBUG_INFORMATION_FORMAT instead of hardcoded MSVC debug flags. |
| projects/clr/hipamd/CMakeLists.txt | Uses CMAKE_MSVC_DEBUG_INFORMATION_FORMAT instead of hardcoded MSVC debug flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Add flags to generate PDB files with full symbolic information | ||
| if(MSVC) | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /Zi") | ||
| set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} /DEBUG:FULL") | ||
| set(CMAKE_MSVC_DEBUG_INFORMATION_FORMAT "$<$<CONFIG:Release,Debug,RelWithDebInfo>:ProgramDatabase>") | ||
| endif() |
| ON) | ||
|
|
||
| if(MSVC) | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /Zi") | ||
| set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} /DEBUG:FULL") | ||
| set(CMAKE_MSVC_DEBUG_INFORMATION_FORMAT "$<$<CONFIG:Release,Debug,RelWithDebInfo>:ProgramDatabase>") | ||
| endif() |
|
This really helps building with sccache, can we push further? |
|
@ROCm/clr-reviewers @ROCm/lrt-build-infra can anyone help with review? Thanks |
|
This pull request has been inactive for 25 days and will be marked as stale. If you would like to keep this PR open, please:
This PR will be automatically closed in 5 days if no further activity occurs. |
|
This pull request has been automatically closed due to inactivity (30 days with no updates). If you'd like to continue working on this, feel free to reopen the PR or create a new one. |
Motivation
Update the way
/Ziflag is set to useCMAKE_MSVC_DEBUG_INFORMATION_FORMATavailable since CMake 3.25. This allows better building experience for TheRock with caching enabled such asccacheorsccache.ccacheandsccacheonly supports/Z7, usingCMAKE_MSVC_DEBUG_INFORMATION_FORMATmakes it easy to change the debug format to/Z7.fixes ROCm/TheRock#748
Technical Details
Bump up CMake minimum version to 3.25. This implies
CMAKE_POLICY_CMP0141=NEW.https://cmake.org/cmake/help/latest/policy/CMP0141.html
Can also remove the linker flag since
/Ziimplies/DEBUGand/DEBUGwith no extra arguments is equivalent to specifying/DEBUG:FULL.https://learn.microsoft.com/en-us/cpp/build/reference/z7-zi-zi-debug-information-format?view=msvc-170
https://learn.microsoft.com/en-us/cpp/build/reference/debug-generate-debug-info?view=msvc-170
Test Plan
Test build with TheRock
Test Result
TheRock builds clr with no errors using
ccacheandsccachewith-DCMAKE_MSVC_DEBUG_INFORMATION_FORMAT=Embedded.Submission Checklist