Fix building the sanitizer instrumentation and hooking it into the hosts

[39otsu]

Carry-over from @jkoritzinsky's PR: #112378

This PR addresses multiple issues that were preventing the ASan instrumentation from building and functioning as intended.

• Ensures that the correct symbols are exported for AddressSanitizer support when building with MSVC
• Fix linkage attributes and calling convention specification for public asan functions
• Disables /DEPENDENTLOADFLAG:0x800 when building with sanitizers as it will look for the dll in C:\Windows\System32

Verified successful build and test run with VS 17.14 p7, MSVC 14.44.35207

[jkoritzinsky]

/azp run runtime-sanitized

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[39otsu]

/azp run runtime-sanitized

[azure-pipelines]

Commenter does not have sufficient privileges for PR 116166 in repo dotnet/runtime

[jkoritzinsky]

/azp run runtime-sanitized

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

This PR fixes multiple issues preventing AddressSanitizer (ASan) instrumentation from building and functioning correctly on Windows with MSVC. The changes ensure proper symbol export, correct calling conventions, and appropriate linker flag handling for sanitizer-enabled builds.

Key changes:

• Changed ASan callback calling convention from __stdcall to __cdecl for consistency and correctness
• Simplified MSVC sanitizer callback macros by removing conditional dllexport logic
• Added .def file to properly export ASan symbols on Windows
• Disabled /DEPENDENTLOADFLAG:0x800 when sanitizers are enabled to avoid DLL loading issues

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/tests/nativeaot/SmokeTests/SharedLibrary/SharedLibrary.cpp	Added __cdecl macro for non-Windows platforms and corrected ASan callback calling convention
src/tests/nativeaot/CustomMainWithStubExe/CustomMainWithStubExeNative.cpp	Added __cdecl macro for non-Windows platforms and corrected ASan callback calling convention
src/tests/nativeaot/CustomMain/CustomMainNative.cpp	Added __cdecl macro for non-Windows platforms and corrected ASan callback calling convention
src/native/minipal/utils.h	Simplified MSVC sanitizer macro definitions by removing conditional dllexport logic
src/native/minipal/sansupport.c	Added extern "C" linkage guards for C++ compatibility
src/native/minipal/sanitizer_exports.def	New file defining exports for ASan callback functions on Windows
src/native/minipal/CMakeLists.txt	Added .def file dependency and linker options for sanitizer support; disabled default build; typo in comment
src/native/corehost/CMakeLists.txt	Added minipal subdirectory to make sanitizer support available to corehost
src/coreclr/vm/interpexec.cpp	Reordered standard library includes before internal headers; removed trailing whitespace
src/coreclr/vm/common.h	Added GCSafeMemCpy to std namespace to fix macro redefinition issues with std::memcpy
src/coreclr/utilcode/ex.cpp	Simplified delete operator by removing redundant DACCESS_COMPILE conditional
src/coreclr/nativeaot/CMakeLists.txt	Added ASAN runtime DLL installation for NativeAOT
eng/native/configurecompiler.cmake	Extended condition to also skip DEPENDENTLOADFLAG when sanitizers are enabled

[agocke]

LGTM aside from some questions about the GCSafeMemCpy usage

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

[JulieLeeMSFT]

@39otsu, please check CI test failures.

[jkotas]

@39otsu Could you please resolve the merge conflicts?

please check CI test failures.

The CI run is old. The results are no longer available.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "9bdee542462db9e5369d5067e429d00bfff8c7a0",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "f7dfaf6882e030191b6a70e6441c4141887d8d3b",
  "last_reviewed_commit": "00991677afe5d4b608fb3a3cc391b042608dabdf",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "c404ba174e46ce0ceb07bb2e7effb6030822bf5a",
  "last_recorded_worker_run_id": "29676142410",
  "review_attempt_commit": "9bdee542462db9e5369d5067e429d00bfff8c7a0",
  "review_attempt_base_ref": "main",
  "review_attempt_count": 1,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "00991677afe5d4b608fb3a3cc391b042608dabdf",
      "review_id": 4730525531
    }
  ]
}

[github-actions]

Holistic Review

Motivation: This PR is a carry-over of #112378 and fixes several problems that prevented the AddressSanitizer instrumentation from building and working correctly with MSVC. It ensures the correct ASan callback symbols are exported when building the host/runtime as a DLL, corrects the calling convention and linkage of the public __asan_* callbacks, and prevents /DEPENDENTLOADFLAG:0x800 from forcing the ASan runtime DLL to be resolved only from System32. It also removes the debug-only GCSafeMemCpy memcpy override, which shadowed the C runtime memcpy and interfered with sanitizer instrumentation.

Approach: The changes are cohesive and mostly build-plumbing:

• eng/native/configurecompiler.cmake: also skip /DEPENDENTLOADFLAG:0x800 when sanitizers are enabled, matching the existing PGO carve-out.
• src/native/minipal/*: add sanitizer_exports.def and wire it in via /DEF: for MSVC so __asan_default_options/__asan_on_error are exported; wrap the callbacks in extern "C"; add a set_source_files_properties(... OBJECT_DEPENDS ...) dependency so the object rebuilds when the def changes; simplify SANITIZER_CALLBACK_CALLCONV/SANITIZER_INTERFACE_CALLCONV on MSVC to plain __cdecl (dropping the dllexport/dllimport path now that exports are handled via the .def file).
• src/native/corehost/CMakeLists.txt: add the minipal subdirectory so the host can link the sanitizer support target.
• src/coreclr/nativeaot/CMakeLists.txt: install the dynamic ASan runtime alongside the NativeAOT output, consistent with the existing corerun/corehost install logic.
• src/coreclr/vm/common.h + object.cpp + inc/formattype.h: remove the _DEBUG-only GCSafeMemCpy memcpy redefinition (endorsed by @jkotas in review as low-value and a source of trouble with standard-header overrides).
• src/coreclr/utilcode/ex.cpp: Exception::Delete now unconditionally uses delete instead of the non-DAC ::delete, which is a natural follow-on to removing the global operator-shadowing.
• Test host files (CustomMain*, SharedLibrary.cpp): change __asan_default_options from __stdcall to __cdecl, addressing @jkoritzinsky's note that the callback must be cdecl on all platforms; the non-Windows #define __cdecl keeps these compiling on Unix.
• src/coreclr/vm/interpexec.cpp: only whitespace/include-reordering cleanup, no behavioral change.

Summary: This is a focused, low-risk build/infrastructure fix that is confined to sanitizer-enabled and debug configurations plus test hosts; the retail runtime code paths are unaffected. The reasoning is sound, the changes are consistent with existing ASan install/export patterns in the repo, and prior reviewer feedback (calling convention, comment wording, GCSafeMemCpy removal) has been addressed. The PR is already approved by a runtime maintainer. The only issues I see are cosmetic: three files (sanitizer_exports.def, sansupport.c, and nativeaot/CMakeLists.txt) are missing a trailing newline (flagged inline). Verdict: LGTM.

Detailed Findings

No functional issues found. Two minor, non-blocking nits are noted inline:

• Missing trailing newline in src/coreclr/nativeaot/CMakeLists.txt, src/native/minipal/sansupport.c, and src/native/minipal/sanitizer_exports.def.

Observations (non-actionable): The SANITIZER_INTERFACE_CALLCONV/SANITIZER_SHARED_RUNTIME machinery in utils.h is now only meaningful on the non-MSVC branch; if the shared-runtime import path is no longer exercised on Windows this is fine, but worth keeping in mind for future cleanup. The ASan symbol exports rely on the .def file being applied via the object target's INTERFACE link options to whatever final binary links minipal_sanitizer_support; this depends on that target being linked into each host/runtime binary that needs the callbacks, which matches the added add_subdirectory in corehost.

Note

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

Generated by Holistic Review · 107.2 AIC · ⌖ 11.1 AIC · ⊞ 10K

[github-actions]

Holistic Review

Motivation: This PR (carry-over of #112378) fixes several problems preventing the AddressSanitizer instrumentation from building and working correctly with MSVC: exporting the correct __asan_* callback symbols when building as a DLL, correcting their calling convention and linkage, preventing /DEPENDENTLOADFLAG:0x800 from forcing the ASan runtime DLL to resolve only from System32, and removing the debug-only GCSafeMemCpy memcpy override that shadowed the CRT memcpy and interfered with instrumentation.

Approach: The changes remain cohesive build-plumbing: skip /DEPENDENTLOADFLAG:0x800 under sanitizers; add sanitizer_exports.def wired via /DEF: and extern "C" callbacks in src/native/minipal; simplify the MSVC callback calling convention to plain __cdecl now that exports go through the .def file; install the dynamic ASan runtime alongside NativeAOT output; remove the _DEBUG-only GCSafeMemCpy redefinition (common.h/object.cpp/formattype.h) and the corresponding ::delete in ex.cpp; and switch test-host __asan_default_options from __stdcall to __cdecl.

Summary: The current PR patch is materially unchanged from the previously reviewed head. All 17 commits are identical in the range-diff; the only differences from the prior scope are rebase artifacts against a newer merge base — the src/native/corehost/CMakeLists.txt add_subdirectory(minipal) hunk and the interpexec.cpp whitespace/include cleanup have been absorbed into main and are no longer part of the PR's incremental patch. These are not new authored changes and introduce no new findings. This remains a focused, low-risk build/infrastructure fix confined to sanitizer-enabled and debug configurations plus test hosts, leaving retail runtime paths unaffected, and it is already approved by a runtime maintainer. Verdict: LGTM.

Assessment History

• review 4730525531 reviewed commit 00991677afe5d4b608fb3a3cc391b042608dabdf with verdict LGTM. Current verdict: LGTM. Assessment unchanged — motivation, approach, and risk are the same; the incremental delta since that review is only a rebase onto a newer base that upstreamed two previously PR-carried hunks, with no change to the PR's authored patches.

Detailed Findings

No new findings. The PR patch has not changed in substance since the prior review; the prior review's two non-blocking trailing-newline nits (in sanitizer_exports.def, sansupport.c, and nativeaot/CMakeLists.txt) still stand and are unchanged by this rebase.

Note

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

Generated by Holistic Review · 59.2 AIC · ⌖ 10.6 AIC · ⊞ 10K