Use MSVC_DEBUG_INFORMATION_FORMAT for CoreCLR object libraries

[elinor-fung]

Follow-up to #130933: convert the /Z7 compile options on the CoreCLR cDAC data descriptor and NativeAOT GC OBJECT libraries to the MSVC_DEBUG_INFORMATION_FORMAT Embedded target property. The property (added in CMake 3.25) is usable now that the repo minimum is 3.26.

Also remove setting COMPILE_PDB_NAME for the cDAC descriptor, since it is embedded rather than a separate PDB.

cc @MichalStrehovsky @max-charlamb @rcj1

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR updates CoreCLR/NativeAOT Windows CMake build logic to use the CMake 3.25+ MSVC_DEBUG_INFORMATION_FORMAT target property (set to Embedded) instead of hand-applied /Z7 compiler options, ensuring object libraries embed CodeView debug info and avoid referencing an external vc140.pdb.

Changes:

• Switch NativeAOT GC OBJECT libraries from target_compile_options(... /Z7) to set_target_properties(... MSVC_DEBUG_INFORMATION_FORMAT Embedded).
• Switch the cDAC data descriptor OBJECT library from target_compile_options(... /Z7) (and prior PDB-name workaround) to MSVC_DEBUG_INFORMATION_FORMAT Embedded.

Show a summary per file

File	Description
src/coreclr/nativeaot/Runtime/Full/CMakeLists.txt	Replaces /Z7 compile options with MSVC_DEBUG_INFORMATION_FORMAT Embedded for GC object libraries on Windows.
src/coreclr/clrdatadescriptors.cmake	Uses MSVC_DEBUG_INFORMATION_FORMAT Embedded for the generated cDAC contract descriptor OBJECT library on MSVC builds.

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 2

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "d922bf573a6c0cf0c98748b813ab7f05e3f991ae",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "ce12d869155b7fba9b8701db6e0e4254a74275b2",
  "last_reviewed_commit": "d922bf573a6c0cf0c98748b813ab7f05e3f991ae",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "ce12d869155b7fba9b8701db6e0e4254a74275b2",
  "last_recorded_worker_run_id": "29687157080",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "d922bf573a6c0cf0c98748b813ab7f05e3f991ae",
      "review_id": 4730767187
    }
  ]
}

[github-actions]

Holistic Review

Motivation: Follow-up to #130933. That earlier change worked around LNK4099 (missing vc140.pdb) errors — fatal under /WX in the VMR NativeAOT publish — by embedding CodeView debug info into the cDAC data descriptor and NativeAOT GC OBJECT libraries via the raw /Z7 compile option (plus a per-target COMPILE_PDB_NAME). Now that the repo's CMake minimum is 3.26, the intent can be expressed with the higher-level MSVC_DEBUG_INFORMATION_FORMAT Embedded target property (added in CMake 3.25).

Approach: Replaces the target_compile_options(... /Z7) calls in clrdatadescriptors.cmake and nativeaot/Runtime/Full/CMakeLists.txt with set_target_properties(... MSVC_DEBUG_INFORMATION_FORMAT Embedded), and drops the now-unnecessary COMPILE_PDB_NAME/COMPILE_PDB_OUTPUT_DIRECTORY properties (irrelevant once debug info is embedded rather than in an external PDB). Comments are condensed while preserving the rationale.

Summary: Correct and well-scoped. The MSVC_DEBUG_INFORMATION_FORMAT property requires policy CMP0141 to be NEW; because both affected CMake trees declare cmake_minimum_required(VERSION 3.26), that policy is automatically NEW, so the property takes effect. Setting Embedded without a generator expression applies /Z7 across all configurations, preserving the previous unconditional behavior, and removing the external-PDB properties is consistent with embedding. This is a clean, idiomatic simplification with no behavioral regression. LGTM.

No actionable findings.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 42.2 AIC · ⌖ 14.5 AIC · ⊞ 10K

[MichalStrehovsky]

Native AOT (up to about a month ago when the lines touched in this PR were added) used to rely on install_static_library both setting the name of the PDB and binplacing it:

runtime/eng/native/functions.cmake

Lines 502 to 519 in 17c839c

	function(install_static_library targetName destination component)
	if (NOT "${component}" STREQUAL "${targetName}")
	get_property(definedComponents GLOBAL PROPERTY CLR_CMAKE_COMPONENTS)
	list(FIND definedComponents "${component}" componentIdx)
	if (${componentIdx} EQUAL -1)
	message(FATAL_ERROR "The ${component} component is not defined. Add a call to `add_component(${component})` to define the component in the build.")
	endif()
	add_dependencies(${component} ${targetName})
	endif()
	install (TARGETS ${targetName} DESTINATION ${destination} COMPONENT ${component})
	if (WIN32)
	set_target_properties(${targetName} PROPERTIES
	COMPILE_PDB_NAME "${targetName}"
	COMPILE_PDB_OUTPUT_DIRECTORY "$<TARGET_FILE_DIR:${targetName}>"
	)
	install (FILES "$<TARGET_FILE_DIR:${targetName}>/${targetName}.pdb" DESTINATION ${destination} COMPONENT ${component})
	endif()
	endfunction()

Looks like /Z7 was added as a hotfix in dotnet/dotnet#7280 because a different change was breaking the VMR.

Should this be a repo-wide policy kind of thing? After the VMR hotfix we're now in a state where we ship the PDB for some of the static libraries but not for others. This PR doesn't make it better or worse, but since this is generalizing the approach from a different PR, maybe we should generalize for the entire repo? (I don't have a strong opinion either way, I don't know why the MSVC team prefers the external PDB, it feels it just makes everything worse, but I assume there is something that it makes better, otherwise they wouldn't have chosen it I hope.)

[elinor-fung]

Should this be a repo-wide policy kind of thing? After the VMR hotfix we're now in a state where we ship the PDB for some of the static libraries but not for others.

My understanding is that before that VMR hotfix, consumers were actually missing debug info for the object libraries if they were compiled into static libraries. The PDB for the static library itself was and continues to be shipped. But previously, symbols for any object libraries in it weren't there since the object libraries' PDBs were separate and not shipped (hence LNK4099 for consumers).

For anything repo-wide, I guess it'd be a function for object libraries so that they embed? I'm somewhat leery of that just because the majority of cases don't need it since our shipping of static libs is limited.

[MichalStrehovsky]

I was more wondering whether everything that builds into a static library should just have the debug info embedded instead of all the machinery to set the PDB name and binplace the PDB. But this works, so LGTM.

[elinor-fung]

/ba-g failure was #130961