Restore symbol-free decoding for CORINFO_HELP_NEW helpers - #131554
Conversation
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 5 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. |
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/coreclr/vm/jitinterface.h:1011
- MAX_AUXILIARY_SYMBOLS is now exactly the current maximum number of auxiliary symbols on x86 (10 allocation helpers + 6 register write barriers + JIT_WriteBarrier = 17). Since SetAuxiliarySymbol only guards with _ASSERTE, any future addition (or unexpected extra registration in some config) would become an immediate out-of-bounds write in retail builds. Consider increasing this constant to leave some headroom.
#define MAX_AUXILIARY_SYMBOLS 17
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (3)
src/native/managed/cdac/tests/DumpTests/AuxiliarySymbolsDumpTests.cs:63
- After switching to a name-keyed map, update the validation loop to iterate by (expectedName, address) so each expected helper name is asserted against the contract.
foreach ((TargetCodePointer address, string expectedName) in helpersByAddress)
{
Assert.True(Target.Contracts.AuxiliarySymbols.TryGetAuxiliarySymbolName(address.AsTargetPointer, out string? name));
Assert.Equal(expectedName, name);
}
src/coreclr/vm/jitinterfacegen.cpp:22
- In FEATURE_PORTABLE_ENTRYPOINTS builds, newobj helpers are invoked via
CEECodeGenInfo::getHelperFtnStatic, which can return aPortableEntryPoint(stored inhlpFuncEntryPoints[ftnNum]). This helper registers the underlyingpfnHelperinstead, soClrDataAccess::GetJitHelperName(which matches againsthlpAuxiliarySymbolTable[i].pfnAuxiliarySymbol) can fail to resolve names for the actual call targets used by the JIT/interpreter in those configurations. Consider registering the portable entrypoint address (or registering at the point wherehlpFuncEntryPoints[ftnNum]is initialized) so the auxiliary symbol table matches what the runtime actually emits/calls.
VMHELPDEF const& helperDef = hlpFuncTable[ftnNum];
PCODE pfnHelper = helperDef.pfnHelper;
DynamicCorInfoHelpFunc dynamicFtnNum;
if (helperDef.IsDynamicHelper(&dynamicFtnNum))
{
src/native/managed/cdac/tests/DumpTests/AuxiliarySymbolsDumpTests.cs:55
- The test currently stores helper mappings keyed by address (
Dictionary<TargetCodePointer, string>+TryAdd), which can drop validation for additional expected helper names that share the same entrypoint address. Keying by helper name instead ensures each expected helper name is validated independently.
This issue also appears on line 59 of the same file.
Dictionary<TargetCodePointer, string> helpersByAddress = [];
TargetPointer table = Target.ReadGlobalPointer(Constants.Globals.AuxiliarySymbols);
uint count = Target.Read<uint>(Target.ReadGlobalPointer(Constants.Globals.AuxiliarySymbolCount));
Target.TypeInfo typeInfo = Target.GetTypeInfo(DataType.AuxiliarySymbolInfo);
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
Co-authored-by: Noah Falk <noahfalk@users.noreply.github.com>
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (3)
src/native/managed/cdac/tests/DumpTests/AuxiliarySymbolsDumpTests.cs:55
- helpersByAddress.TryAdd silently drops duplicate addresses. If two auxiliary symbols end up sharing the same code pointer, this test can still pass while failing to validate one of the expected helper names. This should be treated as a test failure since address→name resolution can’t be one-to-many.
TargetCodePointer address = Target.ReadCodePointer(entry + (ulong)addressOffset);
Assert.NotEqual(TargetCodePointer.Null, address);
helpersByAddress.TryAdd(address, name);
}
src/coreclr/vm/jitinterface.h:1011
- MAX_AUXILIARY_SYMBOLS is set exactly to the current total (10 allocation helpers + 7 x86 write-barrier entries). Because SetAuxiliarySymbol only uses _ASSERTE for bounds checking, any future addition (or a platform-specific extra entry) would be a retail out-of-bounds write. Consider keeping some headroom to avoid fragile exact sizing.
#define MAX_AUXILIARY_SYMBOLS 17
docs/design/datacontracts/AuxiliarySymbols.md:5
- The updated contract description is a bit hard to read/grammatically awkward ("at minimum it has functions") and ends with "new." without formatting. Clarifying this text makes the contract scope easier to understand.
This contract provides name resolution for helper functions. It may include other functions in the future but at minimum it has functions:
* whose executing code resides at dynamically-determined addresses or
* that are native helpers used to implement new.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (3)
src/coreclr/vm/jitinterface.h:1011
- MAX_AUXILIARY_SYMBOLS is set to the exact current population size (7 x86 write-barrier symbols + 10 allocation helpers). Since SetAuxiliarySymbol only has a debug _ASSERTE and will write out of bounds in release if the count ever grows, it would be safer to leave some slack and document why this constant exists.
#define MAX_AUXILIARY_SYMBOLS 17
src/native/managed/cdac/tests/DumpTests/AuxiliarySymbolsDumpTests.cs:55
- helpersByAddress.TryAdd(address, name) drops duplicate addresses silently. If two expected helpers accidentally map to the same address (or the table has duplicates), the test can still pass while skipping validation for one of the helpers. Consider asserting that the address is unique so the test actually covers every expected helper entry.
TargetCodePointer address = Target.ReadCodePointer(entry + (ulong)addressOffset);
Assert.NotEqual(TargetCodePointer.Null, address);
helpersByAddress.TryAdd(address, name);
}
docs/design/datacontracts/AuxiliarySymbols.md:5
- The updated intro is a bit hard to parse (run-on sentence) and uses the C# keyword "new" without formatting. Rephrasing improves readability and makes it clear this is about allocation helpers / the
newoperator.
This contract provides name resolution for helper functions. It may include other functions in the future but at minimum it has functions:
* whose executing code resides at dynamically-determined addresses or
* that are native helpers used to implement new.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (5)
src/native/managed/cdac/tests/DumpTests/AuxiliarySymbolsDumpTests.cs:54
helpersByAddress.TryAdd(address, name)can silently drop an expected helper if two helper names share the same target address (which can happen when multiple helpers map to the same underlying implementation). In that case the test would still passAssert.Empty(expectedHelpers)but would not actually validateTryGetAuxiliarySymbolNamefor the dropped helper name/address pair. Consider asserting that the expected helper addresses are unique by failing whenTryAddreturns false.
TargetCodePointer address = Target.ReadCodePointer(entry + (ulong)addressOffset);
Assert.NotEqual(TargetCodePointer.Null, address);
helpersByAddress.TryAdd(address, name);
src/coreclr/vm/jitinterfacegen.cpp:90
- The ALIGN8 helper variants are registered unconditionally, but
FEATURE_64BIT_ALIGNMENTis only defined on TARGET_ARM/TARGET_WASM (seesrc/coreclr/inc/switches.h). On x86/x64 these helpers will typically still point at the genericRhpNew/RhpNewVariableSizeObjectimplementations (seesrc/coreclr/inc/jithelpers.h), so adding them here can create duplicate addresses with different names in the auxiliary symbol table (and consumes scarce table slots). Consider registering these only whenFEATURE_64BIT_ALIGNMENTis enabled.
SET_NEW_HELPER_AUXILIARY_SYMBOL(CORINFO_HELP_NEWSFAST_ALIGN8)
SET_NEW_HELPER_AUXILIARY_SYMBOL(CORINFO_HELP_NEWSFAST_ALIGN8_VC)
src/coreclr/vm/jitinterface.h:1011
MAX_AUXILIARY_SYMBOLSis now exactly the current worst-case symbol count on x86 (7 write barriers + 10 allocation helpers). SinceSetAuxiliarySymbolonly has a debug_ASSERTE(no release guard), any future addition risks a silent out-of-bounds write in release builds. Consider giving this table some headroom (e.g., 32) so small future additions don't require revisiting this constant.
#define MAX_AUXILIARY_SYMBOLS 17
src/coreclr/vm/jitinterfacegen.cpp:19
SetJitHelperAuxiliarySymbolindexeshlpFuncTable[ftnNum]without a bounds assertion. While current call sites use constants, adding_ASSERTE(ftnNum < CORINFO_HELP_COUNT)makes the helper more robust against accidental misuse and matches the surrounding pattern in this file (e.g.,_SetJitHelperFunction).
LIMITED_METHOD_CONTRACT;
VMHELPDEF const& helperDef = hlpFuncTable[ftnNum];
PCODE pfnHelper = helperDef.pfnHelper;
docs/design/datacontracts/AuxiliarySymbols.md:5
- The updated contract description is a bit unclear/grammatically incomplete: "native helpers used to implement new." It would be clearer to refer to the C#
newkeyword explicitly and to distinguishnewobjvsnewarrallocation helpers.
This contract provides name resolution for helper functions. It may include other functions in the future but at minimum it has functions:
* whose executing code resides at dynamically-determined addresses or
* that are native helpers used to implement new.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
|
/ba-g random test crash |
Restores native DAC and managed cDAC address-to-name resolution for reachable
CORINFO_HELP_NEW*JIT helpers removed by #126281. Fixes an issue found in VS testing, where Visual Studio depends on being able to recognize these methods without native symbols.Changes