[cDAC] implement GetRuntimeNameByAddress#127134
Conversation
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR adds cDAC support needed to implement IXCLRDataProcess::GetRuntimeNameByAddress by enabling stub-kind classification in the ExecutionManager contract, exposing additional RangeSection/range-list metadata, and wiring up the corresponding test and native data-descriptor updates.
Changes:
- Implement
GetRuntimeNameByAddressin the managed legacy SOS DAC implementation using cDAC contracts (ExecutionManager, PrecodeStubs, AuxiliarySymbols). - Add stub classification (
StubKind,GetStubKind) and the data plumbing needed for stub range lists (RangeSection.RangeList,CodeRangeMapRangeList). - Extend tests/mocks and native CDAC descriptors/globals to support the new data contract fields and globals.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/tests/MockDescriptors/MockDescriptors.ExecutionManager.cs | Extends mock RangeSection with RangeList and adds mock CodeRangeMapRangeList layout + builder helpers. |
| src/native/managed/cdac/tests/ExecutionManager/ExecutionManagerTests.cs | Adds tests for stub-kind classification and registers the new mock datatype. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.IXCLRDataProcess.cs | Implements GetRuntimeNameByAddress using cDAC contracts and formats CLRStub names. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/RangeSection.cs | Adds RangeList pointer field to the managed RangeSection data model. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/CodeRangeMapRangeList.cs | Introduces managed data model for CodeRangeMapRangeList. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/PrecodeStubs_Common.cs | Adds GetCandidateEntryPoints enumeration support for precode name resolution. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManager_1.cs | Exposes GetStubKind in v1 contract wrapper. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManager_2.cs | Exposes GetStubKind in v2 contract wrapper. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.cs | Implements GetStubKind logic (globals + range-section lookup) and reads new optional globals. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.ReadyToRunJitManager.cs | Adds stub-kind classification for R2R thunk regions. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.EEJitManager.cs | Adds stub-kind classification for range-list stubs and code-header stub code blocks. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs | Adds new global names for well-known stubs/helpers. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/DataType.cs | Adds CodeRangeMapRangeList to the DataType enum. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IPrecodeStubs.cs | Adds GetCandidateEntryPoints API to support precode resolution. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IExecutionManager.cs | Adds StubKind enum and GetStubKind API. |
| src/coreclr/vm/loaderallocator.hpp | Exposes CodeRangeMapRangeList::_rangeListType via cdac_data<>. |
| src/coreclr/vm/datadescriptor/datadescriptor.inc | Adds RangeSection.RangeList, introduces CodeRangeMapRangeList type descriptor, and exports new stub globals. |
| src/coreclr/vm/codeman.h | Adds cdac_data<RangeSection>::RangeList offset. |
| src/coreclr/inc/gfunc_list.h | Adds DACGFN entries for new helper globals (platform-conditional). |
| src/coreclr/debug/daccess/daccess.cpp | Adjusts CLRStub address formatting to fixed-width hex. |
| docs/design/datacontracts/PrecodeStubs.md | Documents new GetCandidateEntryPoints API and its intended behavior. |
| docs/design/datacontracts/ExecutionManager.md | Documents GetStubKind, new RangeList/CodeRangeMapRangeList descriptors, and new globals. |
…rTests.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
||
| ### Stub Kind Classification | ||
|
|
||
| `GetStubKind` classifies a code address as a known stub type or managed code. It first checks the address against well-known global stub pointers (`ThePreStub`, `VarargPInvokeStub`, `VarargPInvokeStub_RetBuffArg`, `GenericPInvokeCalliHelper`, `TailCallJitHelper`). If the address matches one of these, it returns the corresponding `StubKind` immediately. |
There was a problem hiding this comment.
Are we doing this classification just to match the name returned by the legacy DAC?
I think we should depend on coreclr.dll symbols for the names of these assembly helpers (similar to what we have done for JIT helpers).
There was a problem hiding this comment.
Yes, we are matching the DAC. The DAC uses FindStubManager, which does find these helpers. We can modify the DAC to exclude those, and rely on the debugger to fall back to coreclr symbols.
There was a problem hiding this comment.
I assume that this is only about symbols displayed by disassembly windbg. We do not need to match 1:1 what windbg displays in disassembly. We just need to make sure that it displays it is reasonable.
If we do nothing (ie stop recognizing these addresses in GetRuntimeNameByAddress), what is going to break?
There was a problem hiding this comment.
BTW: windbg disassembly experience that involves this API is very poor today. When I run !u on a method that just calls Console.ReadLine, I see this:
00007ff9`c0cb4780 55 push rbp
00007ff9`c0cb4781 4883ec20 sub rsp,20h
00007ff9`c0cb4785 488d6c2420 lea rbp,[rsp+20h]
00007ff9`c0cb478a 48894d10 mov qword ptr [rbp+10h],rcx
>>> 00007ff9`c0cb478e ff155c051d00 call qword ptr [CLRStub[MethodDescPrestub]@00007FF9C0E84CF0 (00007ff9`c0e84cf0)]
00007ff9`c0cb4794 90 nop
00007ff9`c0cb4795 4883c420 add rsp,20h
00007ff9`c0cb4799 5d pop rbp
00007ff9`c0cb479a c3 ret
I assume that MethodDescPrestub in the disassembly comes from this API. It is close to useless information. Raw address with no symbols name would be about as useful.
I would like to see something like call qword ptr [System_Console!System.Console.ReadLine (00007ff9c0e84cf0)]` in the disassembly, similar to what it was the case in .NET Framework. The contract of these DAC APIs is poorly defined and windbg makes a lot of assumptions about what they do. These details changed as the runtime evolved and ended up breaking the experience. Instead of trying to match 1:1 what these APIs do currently, we should focus on what they need to do to produce human friendly disassembly in windbg in common cases.
There was a problem hiding this comment.
@jkotas could you share your repro? I tried
public class Program
{
public static void Main()
{
Console.ReadLine();
}
}in Release with tiered compilation off and got this:
Normal JIT generated code
Program.Main()
ilAddr is 000001DA1E9B2050 pImport is 000002500E7A4970
Begin 00007FF92052BD40, size 10
D:\git\console\Program.cs @ 4309:
>>> 00007ff9`2052bd40 4883ec28 sub rsp,28h
00007ff9`2052bd44 ff152ebf7d00 call qword ptr [CLRStub[MethodDescPrestub]@00007FF920D07C78 (00007ff9`20d07c78)] (System.Console.ReadLine(), mdToken: 0000000006000084)
D:\git\console\Program.cs @ 4310:
00007ff9`2052bd4a 90 nop
00007ff9`2052bd4b 4883c428 add rsp,28h
00007ff9`2052bd4f c3 ret
There was a problem hiding this comment.
Minimal implementation that only deals with cases that are provably useful, and leaving the rest for the later, sounds good to me. It matches the point that I was trying to make - avoid spending time on implementing functionality that is not valuable.
Maybe these callcounting stub annotations
When I am looking at a simple call in the disassembly, I want to know the actual method name that it is going to call. It is not really interesting to know whether that call is going through a call counting stub or not. It is ok to include that information, but it should not be dominating the name, e.g. instead of CLRStub[CallCountingStub]@00007FF8D2160000, it can be just CallCountingStub.
There was a problem hiding this comment.
I wonder if these belong in the JitHelpers
There was a problem hiding this comment.
Are you thinking about AuxiliarySymbols? Auxiliary symbols are meant to be used for a small number of global helpers. Call counting stubs are not like that. Call counting stubs are dynamically annotated per method and there is a lot of them. A good human readable name of the call counting stub would include the name of the method that it is counting. For example, something like:
00007ff8`d186bec0 48b950eaa42765020000 mov rcx,26527A4EA50h ("Hello, World!")
00007ff8`d186beca ff1590bd7b00 call qword ptr [00007FF8D2027C60] (CallCountingStub System.Console.WriteLine ...)
00007ff8`d186bed0 90 nop
There was a problem hiding this comment.
Sorry, GitHub wasn't refreshed for me; I was referring to ThrowIndexOutOfRangeException. As we discussed, I'm not sure why we use this API to resolve this method. Maybe we shouldn't, but we can leave that for another day.
There was a problem hiding this comment.
ThrowIndexOutOfRangeException is a JIT helper implemented in C#, so it is both a managed method and a JIT helper. I think windbg ends up resolving the address of ThrowIndexOutOfRangeException because of it is the first call that recognizes the address and returns a name for it.
| // Compute the address as a string safely. | ||
| WCHAR addrString[Max64BitHexString + 1]; | ||
| FormatInteger(addrString, ARRAY_SIZE(addrString), "%p", stubAddr); | ||
| FormatInteger(addrString, ARRAY_SIZE(addrString), sizeof(void*) == 8 ? "%016llX" : "%08X", stubAddr); |
There was a problem hiding this comment.
FormatInteger is passed stubAddr (a TADDR/uintptr_t) with the format string %016llX on 64-bit. On non-Windows 64-bit platforms, uintptr_t is typically unsigned long, which does not match %llX and can cause undefined behavior in sprintf_s. Use a format specifier that matches uintptr_t (e.g., PRIxPTR) or cast stubAddr to an explicit unsigned long long (and similarly to unsigned int for the 32-bit branch) so the varargs type always matches the format string.
| FormatInteger(addrString, ARRAY_SIZE(addrString), sizeof(void*) == 8 ? "%016llX" : "%08X", stubAddr); | |
| #ifdef HOST_64BIT | |
| FormatInteger(addrString, ARRAY_SIZE(addrString), "%016llX", static_cast<unsigned long long>(stubAddr)); | |
| #else | |
| FormatInteger(addrString, ARRAY_SIZE(addrString), "%08X", static_cast<unsigned int>(stubAddr)); | |
| #endif |
| // Enumerates candidate precode entry points near a given code address. | ||
| // This is used to resolve a code address that falls within a | ||
| // precode stub back to its entry point. | ||
| IEnumerable<TargetCodePointer> GetCandidateEntryPoints(TargetCodePointer address); |
There was a problem hiding this comment.
We should be able to find the precode start address using some address math rather than having to do guess-and-check repeatedly. I think the algorithm would look something like this:
TADDR GetStubStartFromInterior(RangeSection* pRS, TADDR interiorPrecodeAddress)
{
if (pRS == nullptr || !(pRS->_flags & RangeSection::RANGE_SECTION_RANGELIST))
return 0;
size_t stubSize = 0;
switch (pRS->_pRangeList->GetCodeBlockKind())
{
case STUB_CODE_BLOCK_FIXUPPRECODE:
stubSize = FixupPrecode::CodeSize;
break;
case STUB_CODE_BLOCK_STUBPRECODE:
stubSize = StubPrecode::CodeSize;
break;
default:
return 0;
}
const size_t pageMask = GetStubCodePageSize() - 1;
const TADDR pageBase = interiorPrecodeAddress & ~static_cast<TADDR>(pageMask);
const size_t offset = static_cast<size_t>(interiorPrecodeAddress - pageBase);
return pageBase + (offset / stubSize) * stubSize;
}
| if (wcscmp(wszStubManagerName, W("ThePreStub")) != 0 && wcscmp(wszStubManagerName, W("InteropDispatchStub")) != 0 && wcscmp(wszStubManagerName, W("TailCallStub")) != 0) | ||
| { | ||
| // Skip the stubs that are just assembly helpers. | ||
| wcscpy_s(symbolBuf, bufLen, wszStubManagerName); |
There was a problem hiding this comment.
In RawGetMethodName, the stub-manager branch copies into symbolBuf without checking symbolBuf for null and without setting *symbolLen. This can dereference a null optional output buffer and also returns S_OK even when the buffer is too small (since wcscpy_s failure is ignored). Please mirror the existing string-output pattern used by GetFullMethodName/ConvertUtf8: compute/set symbolLen when requested, only write when symbolBuf is non-null, and return S_FALSE on truncation.
| wcscpy_s(symbolBuf, bufLen, wszStubManagerName); | |
| ULONG32 stubManagerNameLen = static_cast<ULONG32>(wcslen(wszStubManagerName) + 1); | |
| if (symbolLen != NULL) | |
| { | |
| *symbolLen = stubManagerNameLen; | |
| } | |
| if (symbolBuf != NULL) | |
| { | |
| if (wcscpy_s(symbolBuf, bufLen, wszStubManagerName) != 0) | |
| { | |
| return S_FALSE; | |
| } | |
| } |
| - **ReadyToRunJitManager**: Checks whether the address falls within a delay-load method call thunk region. | ||
|
|
||
| ```csharp | ||
| string? GetStubSymbol(TargetCodePointer jittedCodeAddress) |
There was a problem hiding this comment.
Formatting of the stub symbol name is a debugger policy decision. I do not think the algorithm to format the stub symbol name should be part of the data contract. I think the data contract level should be about:
- Tell me what kind of code is at this address. This can be an enum.
- Tell me the range of that code (where does it start)
- Tell me the MethodDesc (or other data) associated with this stub if there is one
And the debugger will assemble a name that it likes out of these.
There was a problem hiding this comment.
@noahfalk I think we can make this work and be more robust if we start by using ToString on the enum values
There was a problem hiding this comment.
"Symbol" probably wasn't the right term, name is more what I was thinking. For example names like "FixupPrecode", not "FixupPrecide(Foo.Bar)@0xdeadbeef". The reason I was resistant to an enum is because I fear it won't be stable over time and enumerations often imply a fixed set of options. If we want to be very explicit that the enum is open-ended and we won't consider it a breaking change to add new values then I'm fine with it.
| => LegacyFallbackHelper.CanFallback() && _legacyProcess is not null ? _legacyProcess.GetRuntimeNameByAddress(address, flags, bufLen, nameLen, nameBuf, displacement) : HResults.E_NOTIMPL; | ||
| { | ||
| if (displacement is not null) | ||
| *displacement = default; |
There was a problem hiding this comment.
Can you check the windbg disassembly for some stubs with multiple instructions? I suspect that setting the displacement to 0 may produce a named label at each instruction that would be an experience regression.
There was a problem hiding this comment.
With (top) and without (bottom) setting displacement to 0:
There was a problem hiding this comment.
There is a regular windbg u and .NET custom !u. The two resolve symbols differently. u symbol resolution is more messy and complicated. Can you try regular windbg u as well?
I have done a quick ad-hoc test:
.NET 11 P3:
0:000> u 00007ff9`6d220960
System.Console.WriteLine():
00007ff9`6d220960 ff25fa3f0000 jmp qword ptr [CLRStub[MethodDescPrestub]@00007FF96D224960 (00007ff9`6d224960)]
00007ff9`6d220966 4c8b15fb3f0000 mov r10,qword ptr [CLRStub[MethodDescPrestub]@00007FF96D224968 (00007ff9`6d224968)]
00007ff9`6d22096d ff25fd3f0000 jmp qword ptr [CLRStub[MethodDescPrestub]@00007FF96D224970 (00007ff9`6d224970)]
With if (displacement != nullptr) *displacement = 0; at the end of ClrDataAccess::GetRuntimeNameByAddress - it assigns wrong symbol to the first instruction and then gives a label to every single instruction after that:
0:000> u 00007ff9`6d7f2534
System.Threading.Thread.Sleep(Int32):
00007ff9`6d7f2534 66666666ff25fa3f0000 jmp word ptr [CLRStub[MethodDescPrestub]@00007FF96D7F6538 (00007ff9`6d7f6538)]
Console.WriteLine():
00007ff9`6d7f253e 4c8b15fb3f0000 mov r10,qword ptr [CLRStub[MethodDescPrestub]@00007FF96D7F6540 (00007ff9`6d7f6540)]
Console.WriteLine():
00007ff9`6d7f2545 ff25fd3f0000 jmp qword ptr [CLRStub[MethodDescPrestub]@00007FF96D7F6548 (00007ff9`6d7f6548)]
Console.WriteLine():
I have not tried with your full delta. I expect that it is going to be the same.
There was a problem hiding this comment.
I see, I'll revert back to only setting it when we succeed in resolving a symbol.
| // Classify a code address as a known stub kind (precode, jump stub, VSD stub, etc.) | ||
| // or as managed code. Returns CodeBlockUnknown if the address is not recognized. | ||
| StubKind GetStubKind(TargetCodePointer jittedCodeAddress); |
There was a problem hiding this comment.
| // Classify a code address as a known stub kind (precode, jump stub, VSD stub, etc.) | |
| // or as managed code. Returns CodeBlockUnknown if the address is not recognized. | |
| StubKind GetStubKind(TargetCodePointer jittedCodeAddress); | |
| // Classify a code address as a known stub kind (precode, jump stub, VSD stub, etc.) | |
| // or as managed code. Returns CodeBlockUnknown if the address is not recognized. | |
| CodeKind GetCodeKind(TargetCodePointer codeAddress); |
Can we generalize to deal with code in general? Here is a code address, tell me whether it is code owned by the runtime and what kind of code is that.
There was a problem hiding this comment.
What classifications do you imagine CodeKind would contain beyond the stubs we are classifying now? My worry is that any of the classifications I can think of that go deeper than "not a stub" start being facts that are also true of some stubs. This would make CodeKind more like CodeFlags.
For me this method does raise a question about whether it should classify stubs implemented via IL too. I think it would be fine if it didn't initially because we are just trying to hit parity with current experiences, but that seems like a natural evolution.
There was a problem hiding this comment.
I think this API should describe what the code is, not how it was generated (e.g. whether it was generated from static IL or runtime generated IL).
Managed code kinds (JITed, R2R, Interpreted) have GCInfo, UnwindInfo, MethodDesc, ... . Once you know that it is managed code, you can get more information about it from different *infos or MethodDesc.
The other code kinds do not have any of that.
Once we do the range section lookup, we know the code kind at the given IP. We can certainly map the managed code kinds to unknown. It looks wasteful to throw away the information since we have it.
There was a problem hiding this comment.
Is the suggestion to add a CodeKind.Managed to the enum? If a stub that currently isn't jitted from IL became jitted in the future would we stop reporting it as a specific kind of stub and report it as Managed instead?
There was a problem hiding this comment.
It looks wasteful to throw away the information since we have it.
One of my original suggestions is that this API should consume a CodeBlockHandle parameter rather than a TargetPointer so that all the work we do to lookup code information (range section, code start, code header, etc) gets cached. Regardless whether we put more info in one API or spread it over multiple I think that would help ensure the info stays cached and quickly accessible.
There was a problem hiding this comment.
Is the suggestion to add a CodeKind.Managed to the enum?
Yes. I would make it more fine-grained and split it into JIT/R2R/Interpreter.
If a stub that currently isn't jitted from IL became jitted in the future would we stop reporting it as a specific kind of stub and report it as Managed instead?
StubLink stubs are like that today. Whether or not we generate StubLink stub or IL stub in a given situation depends on architecture and the stub signature in some cases.
One of my original suggestions is that this API should consume a CodeBlockHandle parameter rather than a TargetPointer so that all the work we do to lookup code information (range section, code start, code header, etc) gets cached.
CodeBlockHandle is a strong-types wrapper around IP. I do not think it is really needed for caching in the current setup.
CodeBlockHandle seems to be coupled with managed code kinds currently. We would have to fix that to generalized it cover the other code kinds.
There was a problem hiding this comment.
StubLink stubs are like that today. Whether or not we generate StubLink stub or IL stub in a given situation depends on architecture and the stub signature in some cases.
I think there are two directions we can go with this API that seem mutually exclusive:
- We can have it focus on identifying stub identity regardless of how that stub's code was generated (StubLinker, Jit, R2R). On this path we would expand the API to include various forms of ILStubs or TransientIL stubs.
- We can have it focus on RangeSection/HeapList distinctions. In this model it only classifies stubs to the degree that RangeSection/HeapList already do so. In order to get a full stub classification the consumer would need to be aware of code-generation distinctions and chain together multiple APIs to further refine the Jitted/R2R categories.
I was imagining this API as being (1), @jkotas it appears you are imagining it being (2). Ultimately both seem useful so I'm fine if this API serves role (2) and we create some other API in the future when we care about IL stub classification for (1). I do think if this API becomes option (2) then we should either get rid of GetJITType or re-implement it in terms of this API.
CodeBlockHandle seems to be coupled with managed code kinds currently. We would have to fix that to generalized it cover the other code kinds.
Yep, I expected thats what we'd do.
There was a problem hiding this comment.
classifies stubs to the degree that RangeSection/HeapList already do so
Most stubs are in codeblocks with a code header. To classify a code address, we need to find a range section and then find the code block within the range section using a nibble map (or the R2R equivalent). The code block is going to have a code header that may or may not point to a methoddesc. If it has methoddesc, it is regular managed code and you can find more about it from methoddesc. If it does not have a method desc, it is some other code kind (STUB_CODE_BLOCK... enum). It is where I would stop.
If we keep going and interrogate MethodDesc in this API too, we are going to be duplicating existing APIs (IRuntimeTypeSystem.IsILStub, IRuntimeTypeSystem.IsWrapperStub, IRuntimeTypeSystem.IsAsyncThunkMethod, and we are likely going to have more). It seems useful for MethodDesc classification to live in separate API(s).
There was a problem hiding this comment.
I think we are in agreement that this method will do option (2) which would not look at MethodDesc info.
| CallCountingStub = 9, | ||
| StubLinkStub = 10, | ||
| MethodCallThunk = 11, | ||
| NoCodeStub = 12, |
There was a problem hiding this comment.
How is this different from UnknownStub?
There was a problem hiding this comment.
Good point, in practice they mean the same thing, either no code range found or no stub found or both. Maybe the intent was to separate "this stub doesn't make sense" from "we couldn't find a code range". I'm not sure we really need this distinction, maybe we should just consolidate the two.
| StubLinkStub = 10, | ||
| MethodCallThunk = 11, | ||
| NoCodeStub = 12, | ||
| Managed = 13, |
There was a problem hiding this comment.
We may want to return more granularity here: Jitted, ReadyToRun, Interpreted
There was a problem hiding this comment.
I'd suggest that we remove 'Managed' and probably 'NoCodeStub' too, returning 'Unknown' for those instead. We have the GetJITType() API that can provide Jitted/R2R/Interpretted classification.
I'm suspicious that at some point we may want to classify IL-backed stub implementations here too or perhaps some of the stubs already classified by this method might become IL backed in the future. 'Managed' or 'Jitted' appear more like orthogonal attributes rather than a specific kind of stub.
There was a problem hiding this comment.
I agree with Noah, we already have an API for the type of managed code.
There was a problem hiding this comment.
I think @jkotas is effectively proposing to turn this method into a super-set of what GetJITType() is. Its not the design I originally had in mind, but I can see it would make a different useful API and it would handle the immediate scenario needs so I'm fine to do that. Assuming we go that way I'd suggest we either remove GetJiTType() or re-implement it as a small helper over this API depending on how heavily its already used.
#127134 (comment)
| if (_legacyProcess is not null) | ||
| { | ||
| uint nameLenLocal = 0; | ||
| char[] nameBufLocal = new char[bufLen > 0 ? bufLen : 1]; |
There was a problem hiding this comment.
The DEBUG validation block allocates nameBufLocal using a uint length expression (new char[bufLen > 0 ? bufLen : 1]). Array lengths must be int, so this will not compile as written. Convert bufLen to an int (with bounds checking) before using it as an array length (and consider handling very large bufLen defensively).
| char[] nameBufLocal = new char[bufLen > 0 ? bufLen : 1]; | |
| int nameBufLocalLength = bufLen == 0 ? 1 : bufLen > int.MaxValue ? int.MaxValue : (int)bufLen; | |
| char[] nameBufLocal = new char[nameBufLocalLength]; |
| { | ||
| hrLocal = _legacyProcess.GetRuntimeNameByAddress( | ||
| address, flags, bufLen, | ||
| &nameLenLocal, |
There was a problem hiding this comment.
In the DEBUG cross-check call to _legacyProcess.GetRuntimeNameByAddress, nameLenLocal is always passed as a non-null pointer. To accurately validate behavior, this should mirror the current call's nameLen parameter (i.e., pass null when nameLen is null). Otherwise debug builds can hit mismatched HRESULT/outputs for callers that omit nameLen.
| &nameLenLocal, | |
| nameLen is null ? null : &nameLenLocal, |
| LPCWSTR wszStubManagerName = pStubManager->GetStubManagerName(TO_TADDR(address)); | ||
| _ASSERTE(wszStubManagerName != NULL); | ||
| if (u16_strcmp(wszStubManagerName, W("ThePreStub")) != 0 && u16_strcmp(wszStubManagerName, W("InteropDispatchStub")) != 0 && u16_strcmp(wszStubManagerName, W("TailCallStub")) != 0) | ||
| { | ||
| PCODE alignedAddress = AlignDown(TO_TADDR(address), PRECODE_ALIGNMENT); | ||
|
|
||
| #ifdef TARGET_ARM | ||
| alignedAddress += THUMB_CODE; | ||
| #endif | ||
|
|
||
| SIZE_T maxPrecodeSize = sizeof(StubPrecode); | ||
|
|
||
| #ifdef HAS_THISPTR_RETBUF_PRECODE | ||
| maxPrecodeSize = max((size_t)maxPrecodeSize, sizeof(ThisPtrRetBufPrecode)); | ||
| #endif | ||
|
|
||
| for (SIZE_T i = 0; i < maxPrecodeSize / PRECODE_ALIGNMENT; i++) | ||
| // Skip the stubs that are just assembly helpers. | ||
| wcscpy_s(symbolBuf, bufLen, wszStubManagerName); | ||
| if (displacement) | ||
| { | ||
| EX_TRY | ||
| { | ||
| // Try to find matching precode entrypoint | ||
| Precode* pPrecode = Precode::GetPrecodeFromEntryPoint(alignedAddress, TRUE); | ||
| if (pPrecode != NULL && pPrecode->GetType() != PRECODE_UMENTRY_THUNK) | ||
| { | ||
| methodDesc = pPrecode->GetMethodDesc(); | ||
| if (methodDesc != NULL) | ||
| { | ||
| if (DacValidateMD(methodDesc)) | ||
| { | ||
| if (displacement) | ||
| { | ||
| *displacement = TO_TADDR(address) - PCODEToPINSTR(alignedAddress); | ||
| } | ||
| goto NameFromMethodDesc; | ||
| } | ||
| } | ||
| } | ||
| alignedAddress -= PRECODE_ALIGNMENT; | ||
| } | ||
| EX_CATCH | ||
| { | ||
| } | ||
| EX_END_CATCH | ||
| *displacement = 0; | ||
| } | ||
| return S_OK; | ||
| } |
There was a problem hiding this comment.
The stub-manager path copies wszStubManagerName into symbolBuf unconditionally and returns S_OK, but it doesn't (1) handle the common "size query" pattern where symbolBuf is null / bufLen is 0, (2) set *symbolLen, or (3) return S_FALSE when the buffer is too small. This can lead to null dereferences, missing length output, and incorrect HRESULTs. Please follow the same output contract as GetFullMethodName/ConvertUtf8: compute required length, set symbolLen when non-null, write only when symbolBuf is non-null and bufLen is sufficient (or truncate + S_FALSE as appropriate).
| #endif | ||
|
|
||
| for (SIZE_T i = 0; i < maxPrecodeSize / PRECODE_ALIGNMENT; i++) | ||
| // Skip the stubs that are just assembly helpers. |
There was a problem hiding this comment.
The comment "Skip the stubs that are just assembly helpers." doesn't match the logic: the code returns the stub manager name when it is not one of the listed assembly-helper names. Please update the comment to reflect the actual behavior (or invert the condition if the comment is what was intended).
| // Skip the stubs that are just assembly helpers. | |
| // Return the stub manager name for stubs other than these assembly-helper stubs. |
| // Placeholders returned by code:GetStubCodeBlockKind | ||
| STUB_CODE_BLOCK_NOCODE = 0x10, | ||
| STUB_CODE_BLOCK_MANAGED = 0x11, | ||
| STUB_CODE_BLOCK_STUBLINK = 0x12, |
There was a problem hiding this comment.
Is this actually used anywhere? I see it used in a few case statements, but not actually passed in anywhere.
This method will annotate stubs with the stub type name: for example "CallCountingStub" "VSD_DispatchStub", as well as providing auxiliary symbol names.
The native implementation currently attempts to follow PrecodeStubs and to resolve normal managed code addresses. The former only appears to yield meaningful, human-readable names on some managed JitHelpers. I am not sure why we consult or require this API for some JitHelpers (but not others), and when we would call this API with a normal managed code address (have never seen it happen). In the interest of time, both of these issues will be left for a follow-up.