Conversation
dotnet-policy-service
Bot
added
the
community-contribution
There was a problem hiding this comment.
Pull request overview
This PR primarily removes a large number of unused/redundant #include directives across CoreCLR, NativeAOT, tooling, and host components, likely aiming to reduce header coupling and improve build hygiene.
Changes:
- Removed unused or transitive
#includedirectives across many C/C++ translation units. - Simplified include sets in VM, JIT, MD, debug, and NativeAOT runtime sources.
- Updated a few tool/source files to use more specific headers (e.g.,
caparser.h).
Show a summary per file
| File | Description |
|---|---|
| src/native/corehost/hostmisc/longfile.windows.cpp | Remove unused include |
| src/coreclr/vm/virtualcallstub.cpp | Remove unused include |
| src/coreclr/vm/typehandle.cpp | Remove unused include (but see opt-restore comment) |
| src/coreclr/vm/tieredcompilation.cpp | Remove unused include |
| src/coreclr/vm/threadsuspend.cpp | Remove unused include |
| src/coreclr/vm/threads.cpp | Remove unused includes |
| src/coreclr/vm/stublink.cpp | Remove unused include |
| src/coreclr/vm/stubhelpers.cpp | Remove unused includes |
| src/coreclr/vm/stdinterfaces_wrapper.cpp | Remove unused include |
| src/coreclr/vm/stdinterfaces.cpp | Remove unused include |
| src/coreclr/vm/runtimehandles.cpp | Remove unused include (but see opt-restore comment) |
| src/coreclr/vm/readytoruninfo.cpp | Remove unused include |
| src/coreclr/vm/qcallentrypoints.cpp | Remove unused includes |
| src/coreclr/vm/prestub.cpp | Remove unused include |
| src/coreclr/vm/precode_portable.cpp | Remove unused include |
| src/coreclr/vm/pgo.cpp | Remove unused include |
| src/coreclr/vm/perfmap.cpp | Remove unused include |
| src/coreclr/vm/peassembly.cpp | Remove unused includes |
| src/coreclr/vm/onstackreplacement.cpp | Remove unused include |
| src/coreclr/vm/multicorejit.cpp | Remove unused include |
| src/coreclr/vm/methodtable.cpp | Remove unused include |
| src/coreclr/vm/method.cpp | Remove unused include |
| src/coreclr/vm/jitinterfacegen.cpp | Remove unused include |
| src/coreclr/vm/interoputil.cpp | Remove unused include |
| src/coreclr/vm/interoplibinterface_comwrappers.cpp | Remove unused includes |
| src/coreclr/vm/interopconverter.cpp | Remove unused include |
| src/coreclr/vm/gchelpers.cpp | Remove optimization pragma includes |
| src/coreclr/vm/finalizerthread.cpp | Remove unused include |
| src/coreclr/vm/fieldmarshaler.cpp | Remove unused include |
| src/coreclr/vm/exceptionhandling.cpp | Remove unused include |
| src/coreclr/vm/dllimport.cpp | Remove unused include |
| src/coreclr/vm/corelib.cpp | Remove unused includes |
| src/coreclr/vm/comutilnative.cpp | Remove unused includes (but see opt-restore comment) |
| src/coreclr/vm/commodule.cpp | Remove unused include |
| src/coreclr/vm/cominterfacemarshaler.cpp | Remove unused include |
| src/coreclr/vm/comcallablewrapper.cpp | Remove unused includes (but see opt-restore comment) |
| src/coreclr/vm/clsload.cpp | Remove unused include |
| src/coreclr/vm/classcompat.cpp | Remove unused includes |
| src/coreclr/vm/class.cpp | Remove unused include |
| src/coreclr/vm/ceeload.cpp | Remove unused include (but see opt-restore comment) |
| src/coreclr/vm/callcounting.cpp | Remove unused include |
| src/coreclr/vm/binder.cpp | Remove unused include |
| src/coreclr/vm/autotrace.cpp | Remove unused include |
| src/coreclr/vm/assembly.cpp | Remove unused includes (but see opt-restore comment) |
| src/coreclr/vm/appdomain.cpp | Remove unused includes |
| src/coreclr/vm/amd64/cgenamd64.cpp | Remove unused include |
| src/coreclr/utilcode/sstring.cpp | Remove unused include |
| src/coreclr/utilcode/rangelist.cpp | Remove unused include |
| src/coreclr/utilcode/log.cpp | Remove unused include |
| src/coreclr/utilcode/hostimpl.cpp | Remove unused include |
| src/coreclr/tools/superpmi/superpmi/streamingsuperpmi.cpp | Remove unused includes |
| src/coreclr/tools/superpmi/superpmi-shim-simple/superpmi-shim-simple.cpp | Remove unused include |
| src/coreclr/tools/superpmi/superpmi-shim-simple/jithost.cpp | Remove unused include |
| src/coreclr/tools/superpmi/superpmi-shim-simple/icorjitinfo_generated.cpp | Remove unused includes |
| src/coreclr/tools/superpmi/superpmi-shim-counter/superpmi-shim-counter.cpp | Remove unused include |
| src/coreclr/tools/superpmi/superpmi-shim-counter/jithost.cpp | Remove unused include |
| src/coreclr/tools/superpmi/superpmi-shim-counter/icorjitinfo_generated.cpp | Remove unused includes |
| src/coreclr/tools/superpmi/mcs/verbmerge.cpp | Remove unused include |
| src/coreclr/tools/superpmi/mcs/removedup.cpp | Remove unused include |
| src/coreclr/tools/superpmi/mcs/mcs.cpp | Remove unused include |
| src/coreclr/tools/metainfo/mdobj.cpp | Remove unused standard includes |
| src/coreclr/tools/metainfo/mdinfo.cpp | Replace CA helper include with caparser.h |
| src/coreclr/runtime/MiscNativeHelpers.cpp | Remove unused include |
| src/coreclr/nativeaot/Runtime/yieldprocessornormalized.cpp | Remove unused includes |
| src/coreclr/nativeaot/Runtime/windows/PalMinWin.cpp | Remove unused include |
| src/coreclr/nativeaot/Runtime/windows/PalCommon.cpp | Remove unused includes |
| src/coreclr/nativeaot/Runtime/threadstore.cpp | Remove unused includes |
| src/coreclr/nativeaot/Runtime/stressLog.cpp | Remove unused include |
| src/coreclr/nativeaot/Runtime/startup.cpp | Remove unused includes |
| src/coreclr/nativeaot/Runtime/rhassert.cpp | Remove unused include |
| src/coreclr/nativeaot/Runtime/portable.cpp | Remove unused includes |
| src/coreclr/nativeaot/Runtime/interoplibinterface_java.cpp | Remove unused include |
| src/coreclr/nativeaot/Runtime/eventtrace_gcheap.cpp | Remove unused includes |
| src/coreclr/nativeaot/Runtime/eventtrace.cpp | Remove unused includes |
| src/coreclr/nativeaot/Runtime/eventpipeinternal.cpp | Remove unused includes |
| src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp | Remove unused include |
| src/coreclr/nativeaot/Runtime/eventpipe/ds-rt-aot.cpp | Remove unused include |
| src/coreclr/nativeaot/Runtime/event.cpp | Remove unused includes |
| src/coreclr/nativeaot/Runtime/disabledruntimeeventinternal.cpp | Remove unused include |
| src/coreclr/nativeaot/Runtime/UniversalTransitionHelpers.cpp | Remove unused includes |
| src/coreclr/nativeaot/Runtime/TypeManager.cpp | Remove unused includes |
| src/coreclr/nativeaot/Runtime/ThunksMapping.cpp | Remove unused include |
| src/coreclr/nativeaot/Runtime/SyncClean.cpp | Remove unused includes |
| src/coreclr/nativeaot/Runtime/RuntimeInstance.cpp | Remove unused includes |
| src/coreclr/nativeaot/Runtime/RestrictedCallouts.cpp | Remove unused include |
| src/coreclr/nativeaot/Runtime/MiscHelpers.cpp | Remove unused includes |
| src/coreclr/nativeaot/Runtime/MathHelpers.cpp | Remove unused include |
| src/coreclr/nativeaot/Runtime/HandleTableHelpers.cpp | Remove unused include |
| src/coreclr/nativeaot/Runtime/GcStressControl.cpp | Remove unused includes |
| src/coreclr/nativeaot/Runtime/GCHelpers.cpp | Remove unused includes |
| src/coreclr/nativeaot/Runtime/FinalizerHelpers.cpp | Remove unused include |
| src/coreclr/nativeaot/Runtime/EHHelpers.cpp | Remove unused include |
| src/coreclr/nativeaot/Runtime/DebugHeader.cpp | Remove unused include |
| src/coreclr/nativeaot/Runtime/Crst.cpp | Remove unused include |
| src/coreclr/nativeaot/Runtime/AsmOffsetsVerify.cpp | Remove unused include |
| src/coreclr/nativeaot/Bootstrap/main.cpp | Remove unused include |
| src/coreclr/minipal/Windows/memory.cpp | Remove unused standard include |
| src/coreclr/md/runtime/mdinternaldisp.cpp | Remove unused include |
| src/coreclr/md/enc/stgtiggerstream.cpp | Remove unused include |
| src/coreclr/md/compiler/regmeta_vm.cpp | Remove unused include |
| src/coreclr/md/compiler/regmeta_import.cpp | Remove unused include |
| src/coreclr/md/compiler/regmeta_emit.cpp | Remove unused include |
| src/coreclr/md/compiler/regmeta_compilersupport.cpp | Remove unused includes |
| src/coreclr/md/compiler/regmeta.cpp | Remove unused include |
| src/coreclr/md/compiler/custattr_import.cpp | Remove unused includes |
| src/coreclr/jit/simdcodegenxarch.cpp | Remove unused include |
| src/coreclr/jit/promotiondecomposition.cpp | Remove unused include |
| src/coreclr/jit/promotion.cpp | Remove unused include |
| src/coreclr/jit/loopcloning.cpp | Remove unused include |
| src/coreclr/jit/importer.cpp | Remove unused include |
| src/coreclr/jit/ee_il_dll.cpp | Remove unused include |
| src/coreclr/jit/dllmain.cpp | Remove unused include |
| src/coreclr/jit/block.cpp | Remove unused include |
| src/coreclr/interop/interoplib.cpp | Remove unused include |
| src/coreclr/ildasm/windasm.cpp | Remove unused include |
| src/coreclr/ildasm/ildasmpch.cpp | Remove include (now empty TU) |
| src/coreclr/ildasm/dres.cpp | Remove unused include |
| src/coreclr/gcinfo/arraylist.cpp | Remove unused include |
| src/coreclr/gc/vxsort/do_vxsort_avx512.cpp | Remove redundant includes |
| src/coreclr/gc/vxsort/do_vxsort_avx2.cpp | Remove redundant includes |
| src/coreclr/dlls/mscorpe/ceefilegenwriter.cpp | Remove unused include |
| src/coreclr/dlls/mscoree/exports.cpp | Remove unused include |
| src/coreclr/debug/runtimeinfo/runtimeinfo.cpp | Remove unused include |
| src/coreclr/debug/ee/funceval.cpp | Remove unused include |
| src/coreclr/debug/ee/debuggermodule.cpp | Remove unused include |
| src/coreclr/debug/ee/dactable.cpp | Remove unused include |
| src/coreclr/debug/ee/amd64/walker.cpp | Remove unused include |
| src/coreclr/debug/di/dbgtransportmanager.cpp | Remove unused include |
| src/coreclr/debug/debug-pal/win/processdescriptor.cpp | Remove unused includes |
| src/coreclr/debug/daccess/dacfn.cpp | Remove unused include |
| src/coreclr/debug/createdump/main.cpp | Remove unused include |
| src/coreclr/binder/assembly.cpp | Remove unused include |
Copilot's findings
Comments suppressed due to low confidence (1)
src/coreclr/vm/ceeload.cpp:4469
<optsmallperfcritical.h>is included with the comment 'Optimization intended for Module::EnsureActive only', but the corresponding<optdefault.h>restore was removed. This causes subsequent code in the file to be built under the perf-critical optimization settings; re-add the restore include afterModule::EnsureActiveto keep the optimization scope limited as intended.
- Files reviewed: 132/132 changed files
- Comments generated: 5
| RETURN; | ||
| } | ||
|
|
||
| #include <optdefault.h> | ||
| CHECK Assembly::CheckLoadLevel(FileLoadLevel requiredLevel, BOOL deadlockOK) | ||
| { |
There was a problem hiding this comment.
<optsmallperfcritical.h> is used for Assembly::EnsureLoadLevel (see the comment immediately above), but the matching <optdefault.h> restore was removed here. That makes the perf-critical optimization settings apply to the rest of this translation unit; re-add the restore include after EnsureLoadLevel so the optimization scope matches the comment intent.
|
|
||
| RETURN NULL; | ||
| } | ||
| #include <optdefault.h> | ||
|
|
||
| //-------------------------------------------------------------------------- |
There was a problem hiding this comment.
This file still switches into the perf-critical optimization mode via <optsmallperfcritical.h> earlier, but the matching <optdefault.h> restore was removed here. Without restoring defaults, the remainder of the file will compile under the perf-critical settings; re-add the restore include after QIStandardInterface to keep the optimization scope limited (see the common pattern in src/coreclr/vm/methodtable.cpp around GetDispatchMap).
|
|
||
| *ppMethod = pMethod; | ||
| } | ||
| FCIMPLEND | ||
| #include <optdefault.h> | ||
|
|
||
| FCIMPL1(AssemblyBaseObject*, RuntimeTypeHandle::GetAssemblyIfExists, ReflectClassBaseObject *pTypeUNSAFE) | ||
| { |
There was a problem hiding this comment.
<optsmallperfcritical.h> is used for RuntimeTypeHandle::GetNextIntroducedMethod, but the corresponding <optdefault.h> restore was removed after the FCIMPL block. This will keep the rest of the file under the perf-critical optimization settings; re-add the restore include after FCIMPLEND to avoid unintended codegen/code-size changes.
|
|
||
| return CastCache::TryGetFromCache(*this, type); | ||
| } | ||
| #include <optdefault.h> | ||
|
|
||
| #endif // #ifndef DACCESS_COMPILE |
There was a problem hiding this comment.
<optsmallperfcritical.h> is still used for TypeHandle::CanCastToCached, but the matching <optdefault.h> restore was removed after the method. Without restoring defaults, the rest of the translation unit will compile under the perf-critical optimization settings; re-add the restore include after CanCastToCached to keep the scope minimal.
| FCIMPLEND | ||
|
|
||
| #include <optdefault.h> | ||
|
|
||
| static BOOL HasOverriddenMethod(MethodTable* mt, MethodTable* classMT, WORD methodSlot) | ||
| { |
There was a problem hiding this comment.
<optsmallperfcritical.h> is enabled for the COMInterlocked::* FCIMPL block, but the corresponding <optdefault.h> restore was removed before the following non-interlocked helpers. This keeps the remainder of the file under the perf-critical optimization settings; re-add the restore include after the last COMInterlocked FCIMPL to limit the scope.
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (4)
src/coreclr/gc/vxsort/do_vxsort_avx2.cpp:10
- The
vxsort_targets_enable_avx2.h/vxsort_targets_disable.hincludes were removed. These headers push/pop compiler target attributes (e.g.,target("avx2")) so this function is compiled with the required ISA. Without them, the build can fail (unsupported intrinsics) or the function may not be properly isolated to AVX2-capable CPUs. Please restore the enable/disable includes around this implementation.
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
#include "common.h"
#include "vxsort.h"
#include "machine_traits.avx2.h"
#include "packer.h"
src/coreclr/vm/perfmap.cpp:10
common.hwas removed, but this file includesperfmap.h, which depends on CoreCLR VM types/macros (e.g.,MethodDesc,PCODE, contracts,CrstStatic) that are normally provided viacommon.h. As-is, this is very likely to fail to compile. Please re-add#include "common.h"(typically first) so the required VM definitions are available.
src/coreclr/vm/autotrace.cpp:26common.hwas removed, but this file uses CoreCLR VM infrastructure (CLRConfig,W(...),u16_strlen,FormatInteger,HANDLE, etc.) that is typically made available viacommon.h/the VM PCH. As written, this is likely to fail to compile. Please restore#include "common.h"near the top of the file.
src/coreclr/gc/vxsort/do_vxsort_avx512.cpp:10- The
vxsort_targets_enable_avx512.h/vxsort_targets_disable.hincludes were removed. These headers push/pop compiler target attributes so this function is compiled with AVX-512 enabled. Without them, the build can fail or the function may not be correctly isolated to AVX-512-capable CPUs. Please restore the enable/disable includes around this implementation.
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
#include "common.h"
#include "vxsort.h"
#include "machine_traits.avx512.h"
void do_vxsort_avx512 (uint8_t** low, uint8_t** high, uint8_t* range_low, uint8_t* range_high)
- Files reviewed: 131/131 changed files
- Comments generated: 6
|
|
||
| return CastCache::TryGetFromCache(*this, type); | ||
| } | ||
| #include <optdefault.h> | ||
|
|
||
| #endif // #ifndef DACCESS_COMPILE | ||
|
|
There was a problem hiding this comment.
optsmallperfcritical.h is included for TypeHandle::CanCastToCached, but the corresponding #include <optdefault.h> that resets #pragma optimize was removed. This can leave the rest of the translation unit compiled under the perf-critical optimization settings. Add #include <optdefault.h> immediately after the perf-critical section (right after CanCastToCached) to restore default optimization settings.
|
|
||
| RETURN NULL; | ||
| } | ||
| #include <optdefault.h> | ||
|
|
||
| //-------------------------------------------------------------------------- | ||
| // Init Outer unknown, cache a GIT cookie |
There was a problem hiding this comment.
#include <optsmallperfcritical.h> is used earlier to improve CCW QI performance, but the matching #include <optdefault.h> that reverts optimization settings was removed here. Please restore #include <optdefault.h> after the QI perf-critical region to ensure the remainder of the file is compiled with the intended default optimization settings.
| } | ||
| #endif // DACCESS_COMPILE | ||
|
|
||
| #include <optdefault.h> | ||
|
|
||
| CHECK Module::CheckActivated() | ||
| { |
There was a problem hiding this comment.
Module::EnsureActive is marked as perf-critical via #include <optsmallperfcritical.h>, but the corresponding #include <optdefault.h> to revert optimization settings was removed in this area. Re-add #include <optdefault.h> after EnsureActive so subsequent code in this TU uses default optimization settings.
| RETURN; | ||
| } | ||
|
|
||
| #include <optdefault.h> | ||
| CHECK Assembly::CheckLoadLevel(FileLoadLevel requiredLevel, BOOL deadlockOK) | ||
| { | ||
| CONTRACTL |
There was a problem hiding this comment.
Assembly::EnsureLoadLevel is placed under #include <optsmallperfcritical.h>, but #include <optdefault.h> was removed here. Please restore #include <optdefault.h> after the perf-critical region so the rest of the file is compiled with default optimization settings.
| // This function sets the card table with the granularity of 1 byte, to avoid ghost updates | ||
| // that could occur if multiple threads were trying to set different bits in the same card. | ||
|
|
||
| #include <optsmallperfcritical.h> | ||
| void ErectWriteBarrier(OBJECTREF *dst, OBJECTREF ref) | ||
| { |
There was a problem hiding this comment.
The perf-tuning pragmas around ErectWriteBarrier were removed (optsmallperfcritical.h / optdefault.h). This write barrier is extremely hot, and dropping the perf-critical optimization region is likely a measurable regression. Please keep the perf-critical optimization scope (or an equivalent mechanism) around ErectWriteBarrier and ensure default optimization is restored afterward.
|
|
||
| #ifdef FEATURE_PORTABLE_ENTRYPOINTS | ||
|
|
||
| #include "common.h" | ||
| #include "precode_portable.hpp" | ||
|
|
||
| #ifdef HOST_64BIT |
There was a problem hiding this comment.
common.h was removed, but precode_portable.hpp relies on VM-wide types/macros (e.g., PCODE, MethodDesc, Volatile, contract macros) and does not include them itself. This change is likely to break compilation of this translation unit. Please restore #include "common.h" (usually as the first include) before including precode_portable.hpp.
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (3)
src/coreclr/ildasm/ildasmpch.cpp:6
ildasmpch.cppis the compilation unit used to build the ildasm precompiled header, but it no longer includesildasmpch.h. Without including the PCH header, PCH generation (and builds that rely on it) may fail. Please restore#include "ildasmpch.h"in this file.
//
// This is just to build the PCH for ildasm
src/coreclr/vm/perfmap.cpp:13
perfmap.hdepends on CoreCLR VM types/macros (e.g.,CrstStatic,_ASSERTE,ARRAY_SIZE) that are normally provided viacommon.h. With thecommon.hinclude removed here, this translation unit may fail to compile (or become dependent on forced includes/PCH behavior) when includingclrconfignocache.h/perfmap.h. Re-add#include "common.h"(or include the specific required headers) before these includes to keep the file self-contained across build configurations.
src/coreclr/vm/autotrace.cpp:26- This file uses many CoreCLR/Win32 types and helpers (
CLRConfig,HANDLE,WCHAR,CreateEventW,FormatInteger, etc.). Withcommon.hremoved, it now has no general runtime header included, which can lead to missing declarations unless the build forcibly includescommon.hfor this TU. Please restore thecommon.hinclude (or add the minimal required headers) to avoid build-config-dependent compilation.
- Files reviewed: 128/130 changed files
- Comments generated: 2
| #ifdef FEATURE_PORTABLE_ENTRYPOINTS | ||
|
|
||
| #include "common.h" | ||
| #include "precode_portable.hpp" | ||
|
|
There was a problem hiding this comment.
precode_portable.hpp relies on CoreCLR VM types/macros (e.g., PCODE, MethodDesc, contracts, _ASSERTE) that are typically provided by including common.h first. With common.h removed from this .cpp, the file becomes dependent on forced includes/PCH behavior and may fail to compile in configurations where common.h isn't pre-included. Please restore the common.h include (or add the specific dependencies needed before including precode_portable.hpp).
| #include "stdafx.h" | ||
| #include "sstring.h" | ||
| #include "ex.h" | ||
| #include "holder.h" | ||
| #include <minipal/strings.h> | ||
|
|
There was a problem hiding this comment.
This file calls minipal_toupper_invariant / minipal_tolower_invariant under !SELF_NO_HOST, but the #include <minipal/strings.h> that declares those APIs was removed. Please restore the include (or include another header that declares these functions) to avoid missing-prototype build errors.
|
Could you please update the PR title and description with what you are trying to achieve? |
|
@moyo1997 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement ( “Agreement” ) is agreed to by the party signing below ( “You” ), 1. Definitions. “Code” means the computer software code, whether in human-readable or machine-executable form, “Project” means any of the projects owned or managed by .NET Foundation and offered under a license “Submit” is the act of uploading, submitting, transmitting, or distributing code or other content to any “Submission” means the Code and any other copyrightable material Submitted by You, including any 2. Your Submission. You must agree to the terms of this Agreement before making a Submission to any 3. Originality of Work. You represent that each of Your Submissions is entirely Your 4. Your Employer. References to “employer” in this Agreement include Your employer or anyone else 5. Licenses. a. Copyright License. You grant .NET Foundation, and those who receive the Submission directly b. Patent License. You grant .NET Foundation, and those who receive the Submission directly or c. Other Rights Reserved. Each party reserves all rights not expressly granted in this Agreement. 6. Representations and Warranties. You represent that You are legally entitled to grant the above 7. Notice to .NET Foundation. You agree to notify .NET Foundation in writing of any facts or 8. Information about Submissions. You agree that contributions to Projects and information about 9. Governing Law/Jurisdiction. This Agreement is governed by the laws of the State of Washington, and 10. Entire Agreement/Assignment. This Agreement is the entire agreement between the parties, and .NET Foundation dedicates this Contribution License Agreement to the public domain according to the Creative Commons CC0 1. |
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/coreclr/ildasm/ildasmpch.cpp:6
- This file’s comment says it exists "to build the PCH for ildasm", but after removing the
#include "ildasmpch.h"it no longer does that. Either restore the include (if a PCH is still intended) or remove/update this file and any build references so the comment and build intent stay accurate.
//
// This is just to build the PCH for ildasm
- Files reviewed: 124/126 changed files
- Comments generated: 1
| // This function sets the card table with the granularity of 1 byte, to avoid ghost updates | ||
| // that could occur if multiple threads were trying to set different bits in the same card. | ||
|
|
||
| #include <optsmallperfcritical.h> | ||
| void ErectWriteBarrier(OBJECTREF *dst, OBJECTREF ref) | ||
| { | ||
| STATIC_CONTRACT_MODE_COOPERATIVE; |
There was a problem hiding this comment.
The removed optsmallperfcritical.h / optdefault.h includes are not "unused"—they scope compiler optimization pragmas around ErectWriteBarrier. Dropping them changes how this hot write-barrier helper is optimized and can regress performance (and may also change which optimizations apply to code that follows). Consider restoring the optimization-scope headers (or replacing them with an equivalent, explicit pragma scope) if the intent is to keep this function in the perf-critical optimization mode.
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
src/coreclr/vm/comutilnative.cpp:1734
<optsmallperfcritical.h>is still included for the COMInterlocked FCIMPLs, but the<optdefault.h>include that restores default optimization settings was removed. This causes the optimization pragmas (andFPO_ON) to leak into the rest of the file. Please re-add<optdefault.h>after the last COMInterlocked FCIMPL in this block.
src/coreclr/vm/gchelpers.cpp:1600- This hunk removes the
<optsmallperfcritical.h>/<optdefault.h>pair that previously scoped MSVC optimization pragmas forErectWriteBarrier. Those headers are not “unused” includes; they change codegen (optimize-for-speed and frame-pointer omission in non-debug MSVC builds). If the intent is to keepErectWriteBarrierperf-critical, please restore the scoped optimization includes around it; otherwise, consider updating/removing the nearby comment(s) or adding a justification since this can be a measurable perf regression in a very hot path.
- Files reviewed: 122/124 changed files
- Comments generated: 0 new
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
src/coreclr/vm/autotrace.cpp:25
common.hwas removed, but this file uses VM contracts/macros (e.g.,CLRConfig,HANDLE,LPWSTR,STANDARD_VM_CONTRACT) that are typically provided via the CoreCLR precompiled header. As written, this will fail to compile (especially on MSVC with PCH enabled). Add back the appropriate PCH/header include (likelycommon.h) or include the specific headers that define these types/macros before they are used.
src/coreclr/vm/perfmap.cpp:12common.hwas removed before includingperfmap.h.perfmap.hreferences CoreCLR VM types (e.g.,MethodDesc,PCODE,CrstStatic, contracts macros) that are not declared in this file’s current include set, so this will not compile whenFEATURE_PERFMAPis enabled. Add back#include "common.h"(or otherwise ensure the required forward declarations/includes exist beforeperfmap.h).
- Files reviewed: 119/121 changed files
- Comments generated: 0 new
|
I don't think this can be done in a batch way. The native build of the codebase is very complicated, with various different platforms and include tricks. There are false positives like removing the pch file or functionality toggling headers. This shouldn't be done by who is not very familiar to the codebase. |
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (6)
src/coreclr/vm/runtimehandles.cpp:223
#include <optsmallperfcritical.h>is used just above forRuntimeTypeHandle::GetNextIntroducedMethod, but the corresponding#include <optdefault.h>was removed. This will keep the perf-critical optimization pragmas enabled for subsequent methods in this file; please restoreoptdefault.hafter the intended region.
src/coreclr/vm/comcallablewrapper.cpp:1178- This file enables
#include <optsmallperfcritical.h>for the CCW QI helpers, but the matching#include <optdefault.h>afterQIStandardInterfacewas removed. That leaves the optimization pragmas active for the remainder of the file; please restoreoptdefault.hafter the intended perf-critical section.
src/coreclr/vm/perfmap.cpp:12 common.hwas removed, but this translation unit uses VM infrastructure types/macros (e.g.,Volatile,CrstStatic,CLRConfig, contract macros) that are typically provided viacommon.h/the VM PCH. This is likely to cause compile failures; restore#include "common.h"as the first include in the file.
src/coreclr/vm/gchelpers.cpp:1600ErectWriteBarrieris a hot path and previously had anoptsmallperfcritical.h/optdefault.hoptimization bracket. Removing those includes changes compiler optimization settings for this function (and may regress write barrier performance). If the intent is only to remove unused includes, keep the optimization bracket (or replace it with an equivalent pragma).
src/coreclr/vm/autotrace.cpp:26common.hwas removed, but this file uses many VM types/macros (e.g.,HANDLE,WCHAR,CLRConfig, contract macros) that are normally provided viacommon.h/the VM PCH. This will likely break the build (and may also violate the PCH include requirement). Add#include "common.h"back as the first include for this translation unit.
src/coreclr/ildasm/ildasmpch.cpp:6- This file exists to build the ildasm precompiled header, but it no longer includes
ildasmpch.h. With MSVC/Ycthis will typically fail (or produce an empty/incorrect PCH). Restore the#include "ildasmpch.h"in this TU (or adjust the build to generate the PCH elsewhere).
//
// This is just to build the PCH for ildasm
- Files reviewed: 117/119 changed files
- Comments generated: 2
| #include "excep.h" | ||
| #include "threads.h" | ||
| #include "comsynchronizable.h" | ||
| #include "threads.h" | ||
| #include "dllimport.h" |
There was a problem hiding this comment.
There are duplicate includes (threads.h and also vars.hpp later). Since this change is touching the include list, consider removing duplicates to keep the include set minimal and make include-order dependencies clearer.
| #include "commandline.h" | ||
| #include "errorhandling.h" | ||
| #include "methodcontext.h" | ||
| #include "methodcontextreader.h" | ||
| #include "spmiutil.h" | ||
| #include "fileio.h" | ||
| #include "commandline.h" |
There was a problem hiding this comment.
commandline.h is included twice (at the top include list and again a few lines later). Since this PR is already adjusting includes, remove the duplicate include to keep the header list minimal and avoid confusion about include-order dependencies.
No description provided.