Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Feb 9, 2026

  • Remove INodeWithSize interface and always use IMAGE_REL_SYMBOL_SIZE relocation
  • Remove HasEndPointer flag and ModuleInfoFlags enum
  • Update ModuleInfoRow to have SectionId, Length, Start fields
  • Remove GetLength() method and use Length field directly
  • Remove Size properties from ProxyTypeMapObjectNode and ExternalTypeMapObjectNode
  • Remove double blank lines left by mechanical removal in ~30 files
  • Restore single blank lines between methods and logical code sections

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

…SIZE reloc

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
@dotnet-policy-service
Copy link
Contributor

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

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Copilot AI changed the title [WIP] Remove INodeWithSize interface and update size emission Replace INodeWithSize.Size property with IMAGE_REL_SYMBOL_SIZE relocation Feb 9, 2026
Copilot AI requested a review from jkoritzinsky February 9, 2026 22:27
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Copilot AI changed the title Replace INodeWithSize.Size property with IMAGE_REL_SYMBOL_SIZE relocation Remove INodeWithSize interface and use IMAGE_REL_SYMBOL_SIZE relocation Feb 9, 2026
Copilot AI requested a review from jkoritzinsky February 9, 2026 22:56
@jkoritzinsky
Copy link
Member

@copilot please remove the HasEndPointer flag and the Flags field it goes in. Then, update the native ModuleInfoRow type to have a Start field and a Length field instead of Start and End and remove the Flags type there as well.

Finally, update the code in ReadyToRunHeaderNode to require alignment before emitting each entry.

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 10, 2026

@copilot please remove the HasEndPointer flag and the Flags field it goes in. Then, update the native ModuleInfoRow type to have a Start field and a Length field instead of Start and End and remove th...

Done. Removed HasEndPointer flag and ModuleInfoFlags enum. Updated ModuleInfoRow to have Start and Length fields instead of Start, End, and Flags. Added alignment requirement before emitting each entry. (5b50167)

Copilot AI changed the title Remove INodeWithSize interface and use IMAGE_REL_SYMBOL_SIZE relocation Remove INodeWithSize interface, use IMAGE_REL_SYMBOL_SIZE relocation, and restructure ModuleInfoRow Feb 10, 2026
Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address this feedback

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 10, 2026

@copilot please address this feedback

All feedback addressed. (cc44d34)

…otnet/runtime into copilot/remove-inodewithsize-interface
@jkoritzinsky jkoritzinsky marked this pull request as ready for review February 10, 2026 22:14
Copilot AI review requested due to automatic review settings February 10, 2026 22:14
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the INodeWithSize mechanism for tracking emitted node sizes and switches NativeAOT module section size reporting to use RelocType.IMAGE_REL_SYMBOL_SIZE, alongside restructuring the NativeAOT ModuleInfoRow ABI to store a resolved Length directly.

Changes:

  • Removed INodeWithSize implementations and per-node manual size tracking (_size/Size properties) across many dependency analysis nodes.
  • Updated ReadyToRunHeaderNode to emit section entries as { SectionId, Length (IMAGE_REL_SYMBOL_SIZE), Start } and adjusted EntrySize accordingly.
  • Updated the NativeAOT runtime to consume the new ModuleInfoRow layout and removed the old flags/end-pointer length computation.

Reviewed changes

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

Show a summary per file
File Description
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeMetadataMapNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/StructMarshallingStubMapNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/StaticsInfoHashtableNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/StackTraceMethodMappingNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/StackTraceLineNumbersNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/StackTraceDocumentsNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ResourceIndexNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ResourceDataNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionVirtualInvokeMapNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeMapNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionFieldMapNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReadyToRunHeaderNode.cs Emits IMAGE_REL_SYMBOL_SIZE for section lengths; updates EntrySize; removes INodeWithSize definition and old end-pointer logic.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ProxyTypeMapObjectNode.cs Removes INodeWithSize/Size property and assignment.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/PointerTypeMapNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NativeLayoutInfoNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ModuleInitializerListNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/MetadataNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InterfaceGenericVirtualMethodTableNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericVirtualMethodTableNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericTypesTemplateMap.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericTypesHashtableNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericMethodsTemplateMap.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericMethodsHashtableNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/FunctionPointerMapNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ExternalTypeMapObjectNode.cs Removes INodeWithSize/Size property and assignment.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ExternalReferencesTableNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ExactMethodInstantiationsNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/DelegateMarshallingStubMapNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/DehydratedDataNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ClassConstructorContextMap.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ByRefTypeMapNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ArrayOfFrozenObjectsNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ArrayOfEmbeddedDataNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ArrayMapNode.cs Removes INodeWithSize and manual _size tracking.
src/coreclr/tools/Common/Internal/Runtime/ModuleHeaders.cs Removes managed ModuleInfoFlags enum.
src/coreclr/tools/Common/Compiler/ObjectWriter/ObjectWriter.cs Always records symbol sizes for offset-0 symbols (enables IMAGE_REL_SYMBOL_SIZE resolution in NativeAOT builds).
src/coreclr/nativeaot/Runtime/inc/ModuleHeaders.h Removes native ModuleInfoFlags enum.
src/coreclr/nativeaot/Runtime/TypeManager.h Updates ModuleInfoRow layout to { SectionId, Length, Start }.
src/coreclr/nativeaot/Runtime/TypeManager.cpp Consumes ModuleInfoRow.Length directly; removes old flag/end-pointer length computation.

@stephentoub
Copy link
Member

@copilot please invoke the code-review skill and post your analysis/results as comment on this PR

Copy link
Contributor Author

Copilot AI commented Feb 11, 2026

@copilot please invoke the code-review skill and post your analysis/results as comment on this PR

🤖 Copilot Code Review — PR #124218

Holistic Assessment

Motivation: The change removes boilerplate INodeWithSize tracking from ~33 node classes and unifies size resolution through the existing IMAGE_REL_SYMBOL_SIZE relocation mechanism already used by R2R. The restructured ModuleInfoRow ABI ({SectionId, Length, Start} instead of {SectionId, Flags, Start, End}) is a clean simplification. This is justified cleanup driven by the component maintainer.

Approach: Correct. The IMAGE_REL_SYMBOL_SIZE reloc is resolved by EmitOrResolveRelocation in ObjectWriter.cs using SymbolDefinition.Size, which is populated when each node's symbol is emitted. The ReadyToRunHeaderNode is Phase.Late, so referenced symbols are already defined when the header relocs are resolved. The companion ObjectWriter.cs change (by @jkoritzinsky) to record sizes for all NAOT symbols — not just methods — ensures the definedSymbol.Size values are populated.

Summary: ⚠️ Needs Human Review. The core logic is correct — the ABI layout matches between emitter (EmitInt + EmitReloc(IMAGE_REL_SYMBOL_SIZE) + EmitPointerReloc) and consumer ({int32_t SectionId, int32_t Length, void* Start}), and the EntrySize calculation (8 + PointerSize) is correct for both 32-bit (12) and 64-bit (16). The size resolution through ObjectWriter is verified. There is one cosmetic issue to note below.


Detailed Findings

✅ ABI layout correctness verified

The managed emitter writes SectionId (4 bytes)Length via IMAGE_REL_SYMBOL_SIZE (4 bytes)Start pointer (PointerSize bytes). The native ModuleInfoRow struct reads SectionId (int32_t)Length (int32_t)Start (void*). The EntrySize byte emitted (= 8 + PointerSize) matches sizeof(ModuleInfoRow) on both 32-bit (12) and 64-bit (16) targets. The runtime's ASSERT(m_pHeader->EntrySize == sizeof(ModuleInfoRow)) will pass.

✅ Size resolution flow verified

ObjectWriter.EmitObject now records nodeContents.Data.Length as the size for all symbols with n.Offset == 0 (previously gated on recordSize for NAOT, which only covered methods). The IMAGE_REL_SYMBOL_SIZE reloc in EmitOrResolveRelocation resolves in-place via Relocation.WriteValue(relocType, pData, definedSymbol.Size). Since ReadyToRunHeaderNode is Phase.Late, all referenced node symbols are already emitted and their sizes populated in _definedSymbols by the time the header is processed.

✅ Complete removal of INodeWithSize

Grepped the full tree — no references to INodeWithSize remain. The ModuleInfoFlags enum is gone from both ModuleHeaders.cs and ModuleHeaders.h. The HasEndPointer() and GetLength() methods are removed from TypeManager.cpp/h.

✅ Mach-O / ELF / PE compatibility

The IMAGE_REL_SYMBOL_SIZE reloc was already supported by all object writers (MachObjectWriter handles it via SUBTRACTOR+UNSIGNED reloc pairs; ElfObjectWriter and PEObjectWriter go through the base EmitOrResolveRelocation path). No new format-specific handling is needed.

💡 Double blank lines left by mechanical removal — cosmetic

The removal of _size fields, INodeWithSize.Size properties, and _size = ... assignments left consecutive blank lines in ~30 files (e.g., ArrayMapNode.cs lines 23-24 and 66-67). This is a minor cosmetic issue from the automated removal — not a blocker, but the maintainer may want a follow-up cleanup pass to collapse the double blank lines. This affects: ArrayMapNode, ArrayOfEmbeddedDataNode, ByRefTypeMapNode, ClassConstructorContextMap, DehydratedDataNode, DelegateMarshallingStubMapNode, ExactMethodInstantiationsNode, ExternalReferencesTableNode, FunctionPointerMapNode, GenericMethodsHashtableNode, GenericMethodsTemplateMap, GenericTypesHashtableNode, GenericTypesTemplateMap, GenericVirtualMethodTableNode, InterfaceGenericVirtualMethodTableNode, MetadataNode, ModuleInitializerListNode, NativeLayoutInfoNode, PointerTypeMapNode, ReflectionFieldMapNode, ReflectionInvokeMapNode, ReflectionVirtualInvokeMapNode, ResourceDataNode, ResourceIndexNode, StackTraceDocumentsNode, StackTraceLineNumbersNode, StackTraceMethodMappingNode, StaticsInfoHashtableNode, StructMarshallingStubMapNode, and TypeMetadataMapNode.

@jkoritzinsky
Copy link
Member

@copilot please remove the double blank lines mentioned in #124202 (comment)

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 11, 2026

@copilot please remove the double blank lines mentioned in #124202 (comment)

Collapsed all double blank lines across the 30 affected files. (76c42d9)

@MichalStrehovsky
Copy link
Member

@copilot go over the blank lines deleted in this pull request. Make sure a blank line still separates individual method blocks. Make sure a blank line still separates logically different parts of method bodies.

@MichalStrehovsky
Copy link
Member

The test failures look related. Pulled down a random crashdump with runfo and looks like GC statics are broken. GC statics are one of the things referenced from R2R header.

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 11, 2026

@copilot go over the blank lines deleted in this pull request. Make sure a blank line still separates individual method blocks. Make sure a blank line still separates logically different parts of meth...

Restored single blank lines at all 47 locations across the 30 files. Each location now has exactly one blank line separating method blocks and logical code sections (e.g., between constructor } and next method, between Save() and return, between loop } and next logical section). No double blank lines remain. Build verified clean. (9b06c1e)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants