Improvements to native AOT method debug information

[MichalStrehovsky]

Fixes#117309

• Make sure we have type information for the method and parameters show up in the stack
• Format method names like the VS debugger does instead of using symbol name
• Previously we'd skip emitting method debug information if there's no sequence points but I'm not clear on why so deleting that filter too
• this needs to be named this or the debugger will treat it as a regular parameter that is visible in the signature. AFAIK the only reason why we didn't use this originally was CppCodegen.

Before (notice no int n parameter):

After:

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR improves NativeAOT-generated debug information (primarily CodeView/PDB) so debuggers and tooling can show richer method/parameter type details and more user-friendly method names.

Changes:

• Emit a non-zero CodeView procedure type index (methodTypeIndex) and pass it through to the S_GPROC32_ID record.
• Introduce CSharpTypeNameFormatter and use it to provide a C#-style method display name in CodeView symbols instead of the mangled symbol name.
• Rename the implicit instance parameter from "___this" to "this" in ECMA PDB-backed parameter name enumeration.

Reviewed changes

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

Show a summary per file

File	Description
src/coreclr/tools/Common/TypeSystem/IL/EcmaMethodIL.Symbols.cs	Changes implicit instance parameter name emitted by PDB-backed parameter enumeration to "this".
src/coreclr/tools/Common/TypeSystem/Common/Utilities/CSharpTypeNameFormatter.cs	Adds a new formatter for producing C#-style type (and method owning-type) names.
src/coreclr/tools/aot/ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj	Links the new formatter into the ILCompiler.TypeSystem build.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs	Always computes and supplies method type ID and method display name during debug info emission.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs	Updates debug function info signature and sequence-point gating logic for DWARF emission.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs	Passes the new display name through to CodeView symbol emission; preserves line info gating.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CodeView/CodeViewSymbolsBuilder.cs	Writes methodTypeIndex and the new display name into the CodeView S_GPROC32_ID record.

[Copilot]

Pull request overview

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

Suppressed comments (3)

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs:129

• methodDisplayName is constructed for every IMethodNode during debug-info emission, but it’s not used by the DWARF path (e.g., UnixObjectWriter.Aot.cs ignores it) and the Wasm writer also ignores it. This adds per-method StringBuilder/string/UTF8 allocations in ObjectWritingOptions.GenerateDebugInfo builds on non-Windows targets. Consider computing the display name only in the CodeView/Coff path (or lazily in the override when needed) to avoid unnecessary work on other object writers.

 uint methodTypeIndex = _userDefinedTypeDescriptor.GetMethodFunctionIdTypeIndex(methodNode.Method);
Utf8String methodDisplayName = new Utf8String(CSharpTypeNameFormatter.Instance.FormatName(methodNode.Method));
EmitDebugFunctionInfo(methodTypeIndex, methodDisplayName, methodName, methodSymbol, debugNode);
}

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs:276

• debugNode.GetNativeSequencePoints() is enumerated for .Any() here, and then enumerated again later when passing sequence points into EmitLineInfo. If this enumerable is not a cheap materialized collection, this can add avoidable overhead. Consider capturing/materializing the sequence points once and reusing them for both the emptiness check and emission.

 if (debugNode.GetNativeSequencePoints().Any())
{
_dwarfBuilder.EmitLineInfo(
methodSymbol.SectionIndex,
section.SymbolName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs:228

• debugNode.GetNativeSequencePoints() is enumerated for .Any() here, and then enumerated again later when passing sequence points into EmitLineInfo. If this enumerable is not a cheap materialized collection, this can add avoidable overhead. Consider capturing/materializing the sequence points once and reusing them for both the emptiness check and emission.

 if (debugNode.GetNativeSequencePoints().Any())
{
debugSymbolsBuilder.EmitLineInfo(
_debugFileTableBuilder,
methodName,

[MichalStrehovsky]

Previously we'd skip emitting method debug information if there's no sequence points but I'm not clear on why so deleting that filter too

So this was used to paper over issues when no debug information for parameters is available. "No sequence points" was used to approximate "no parameter debug info", basically. DWARF debug info emission wasn't ready to deal with that. We now just emit no parameter debug information. This mostly affects compiler-generated IL.

Alternatively, we could name these parameters, but that sounds a bit problematic.

[Copilot]

Pull request overview

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

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs:228

• GetNativeSequencePoints() is implemented as an iterator on MethodCodeNode and does non-trivial work per enumeration. The current pattern enumerates it twice (once for Any(), again when emitting line info). Materializing the sequence points once avoids doing all that work twice per method.

 if (debugNode.GetNativeSequencePoints().Any())
{
debugSymbolsBuilder.EmitLineInfo(
_debugFileTableBuilder,
methodName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs:276

• GetNativeSequencePoints() is implemented as an iterator on MethodCodeNode and does non-trivial work per enumeration. The current pattern enumerates it twice (once for Any(), again when emitting line info). Materializing the sequence points once avoids doing all that work twice per method.

 if (debugNode.GetNativeSequencePoints().Any())
{
_dwarfBuilder.EmitLineInfo(
methodSymbol.SectionIndex,
section.SymbolName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs:128

• methodDisplayName is computed for every method node unconditionally, but the derived emitters for DWARF and Wasm don’t use it. This adds avoidable per-method allocations/work on non-Windows NativeAOT builds. Consider only formatting the C# display name when targeting Windows (CodeView/PDB), and otherwise just pass methodName (or default).

 if (node is IMethodNode methodNode)
{
uint methodTypeIndex = _userDefinedTypeDescriptor.GetMethodFunctionIdTypeIndex(methodNode.Method);
Utf8String methodDisplayName = new Utf8String(CSharpTypeNameFormatter.Instance.FormatName(methodNode.Method));
EmitDebugFunctionInfo(methodTypeIndex, methodDisplayName, methodName, methodSymbol, debugNode);
}

[max-charlamb]

It looks like this changes the scheme for method name symbols. Can we rev the SymbolName contract to n2 so tooling can handle both formats?

runtime/src/coreclr/nativeaot/Runtime/datadescriptor/datadescriptor.inc

Line 259 in 714432e

CDAC_GLOBAL_CONTRACT(SymbolName, n1)

[MichalStrehovsky]

It looks like this changes the scheme for method name symbols. Can we rev the SymbolName contract to n2 so tooling can handle both formats?

It does not change COFF symbol names, the symbol name is still the same. It changes the CodeView S_GPROC32_ID name of the method. This is same as C++ where:

classFoo
{
voidBar(int x);
};
voidFoo::Bar(int x) { }

The COFF symbol name for Bar is ?Bar@Foo@@AEAAXH@Z but S_GPROC32_ID symbol record assigns it name Foo::Bar.

Do we still need to rev SymboName contract?

[max-charlamb]

I see, I misunderstood the change. However, I would still like to bump the version. This should give us flexibility in the future to support previous versions.

/c @noahfalk

[noahfalk]

I see, I misunderstood the change. However, I would still like to bump the version. This should give us flexibility in the future to support previous versions.

I think it would be fine to do and right now the consequences are minimal. One issue that I do think we still need to address is documenting an explicit data contract for this so everyone agrees what behavior is or isn't covered by the contract.

[MichalStrehovsky]

I think it would be fine to do and right now the consequences are minimal. One issue that I do think we still need to address is documenting an explicit data contract for this so everyone agrees what behavior is or isn't covered by the contract.

Does cDAC interpret the symbol names in any way? The symbol names are supposed to be unique and deterministic for the exact same app, but e.g. two different apps might already assign different names to the same thing and there can be various butterfly effects when something in an assembly changes. They are not meant to be stable or reversible like the C++ mangled names. We can't use a reversible/stable C++-like name mangling because .NET naming is ridiculously long (std in C++ vs System.Collections.Generics in .NET).

[Copilot]

Pull request overview

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

[jkotas]

Does cDAC interpret the symbol names in any way?

Yes, for limited set of symbols in CoreLib where the ambiguity should not be a problem. As @noahfalk mentioned this needs to be documented for clarity.

[Copilot]

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/coreclr/nativeaot/Runtime/datadescriptor/datadescriptor.inc:262

• Bumping the NativeAOT cDAC contract version for SymbolName from n1 to n2 has compatibility impact for dump readers, but this PR doesn't appear to include any corresponding contract shape/algorithm changes or managed reader updates. If this wasn't intentional, please revert the version bump; if it was intentional, the contract implementation + docs need to be updated in the same PR so readers can negotiate v2 successfully.

CDAC_GLOBAL_CONTRACT(StressLog, c2)
CDAC_GLOBAL_CONTRACT(Loader, n1)
CDAC_GLOBAL_CONTRACT(ExecutionManager, n1)
CDAC_GLOBAL_CONTRACT(SyncBlock, n1)
CDAC_GLOBAL_CONTRACT(SymbolName, n2)
CDAC_GLOBAL_CONTRACT(CodeVersions, n1)
CDAC_GLOBAL_CONTRACT(ReJIT, n1)
CDAC_GLOBAL_CONTRACT(PrecodeStubs, n1)

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Lite

[MichalStrehovsky]

Fixes#117309

• Make sure we have type information for the method and parameters show up in the stack
• Format method names like the VS debugger does instead of using symbol name
• Previously we'd skip emitting method debug information if there's no sequence points but I'm not clear on why so deleting that filter too
• this needs to be named this or the debugger will treat it as a regular parameter that is visible in the signature. AFAIK the only reason why we didn't use this originally was CppCodegen.

Before (notice no int n parameter):

After:

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR improves NativeAOT-generated debug information (primarily CodeView/PDB) so debuggers and tooling can show richer method/parameter type details and more user-friendly method names.

Changes:

• Emit a non-zero CodeView procedure type index (methodTypeIndex) and pass it through to the S_GPROC32_ID record.
• Introduce CSharpTypeNameFormatter and use it to provide a C#-style method display name in CodeView symbols instead of the mangled symbol name.
• Rename the implicit instance parameter from "___this" to "this" in ECMA PDB-backed parameter name enumeration.

Reviewed changes

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

Show a summary per file

File	Description
src/coreclr/tools/Common/TypeSystem/IL/EcmaMethodIL.Symbols.cs	Changes implicit instance parameter name emitted by PDB-backed parameter enumeration to "this".
src/coreclr/tools/Common/TypeSystem/Common/Utilities/CSharpTypeNameFormatter.cs	Adds a new formatter for producing C#-style type (and method owning-type) names.
src/coreclr/tools/aot/ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj	Links the new formatter into the ILCompiler.TypeSystem build.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs	Always computes and supplies method type ID and method display name during debug info emission.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs	Updates debug function info signature and sequence-point gating logic for DWARF emission.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs	Passes the new display name through to CodeView symbol emission; preserves line info gating.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CodeView/CodeViewSymbolsBuilder.cs	Writes methodTypeIndex and the new display name into the CodeView S_GPROC32_ID record.

[Copilot]

Pull request overview

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

Suppressed comments (3)

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs:129

• methodDisplayName is constructed for every IMethodNode during debug-info emission, but it’s not used by the DWARF path (e.g., UnixObjectWriter.Aot.cs ignores it) and the Wasm writer also ignores it. This adds per-method StringBuilder/string/UTF8 allocations in ObjectWritingOptions.GenerateDebugInfo builds on non-Windows targets. Consider computing the display name only in the CodeView/Coff path (or lazily in the override when needed) to avoid unnecessary work on other object writers.

 uint methodTypeIndex = _userDefinedTypeDescriptor.GetMethodFunctionIdTypeIndex(methodNode.Method);
Utf8String methodDisplayName = new Utf8String(CSharpTypeNameFormatter.Instance.FormatName(methodNode.Method));
EmitDebugFunctionInfo(methodTypeIndex, methodDisplayName, methodName, methodSymbol, debugNode);
}

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs:276

• debugNode.GetNativeSequencePoints() is enumerated for .Any() here, and then enumerated again later when passing sequence points into EmitLineInfo. If this enumerable is not a cheap materialized collection, this can add avoidable overhead. Consider capturing/materializing the sequence points once and reusing them for both the emptiness check and emission.

 if (debugNode.GetNativeSequencePoints().Any())
{
_dwarfBuilder.EmitLineInfo(
methodSymbol.SectionIndex,
section.SymbolName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs:228

• debugNode.GetNativeSequencePoints() is enumerated for .Any() here, and then enumerated again later when passing sequence points into EmitLineInfo. If this enumerable is not a cheap materialized collection, this can add avoidable overhead. Consider capturing/materializing the sequence points once and reusing them for both the emptiness check and emission.

 if (debugNode.GetNativeSequencePoints().Any())
{
debugSymbolsBuilder.EmitLineInfo(
_debugFileTableBuilder,
methodName,

[MichalStrehovsky]

Previously we'd skip emitting method debug information if there's no sequence points but I'm not clear on why so deleting that filter too

So this was used to paper over issues when no debug information for parameters is available. "No sequence points" was used to approximate "no parameter debug info", basically. DWARF debug info emission wasn't ready to deal with that. We now just emit no parameter debug information. This mostly affects compiler-generated IL.

Alternatively, we could name these parameters, but that sounds a bit problematic.

[Copilot]

Pull request overview

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

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs:228

• GetNativeSequencePoints() is implemented as an iterator on MethodCodeNode and does non-trivial work per enumeration. The current pattern enumerates it twice (once for Any(), again when emitting line info). Materializing the sequence points once avoids doing all that work twice per method.

 if (debugNode.GetNativeSequencePoints().Any())
{
debugSymbolsBuilder.EmitLineInfo(
_debugFileTableBuilder,
methodName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs:276

• GetNativeSequencePoints() is implemented as an iterator on MethodCodeNode and does non-trivial work per enumeration. The current pattern enumerates it twice (once for Any(), again when emitting line info). Materializing the sequence points once avoids doing all that work twice per method.

 if (debugNode.GetNativeSequencePoints().Any())
{
_dwarfBuilder.EmitLineInfo(
methodSymbol.SectionIndex,
section.SymbolName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs:128

• methodDisplayName is computed for every method node unconditionally, but the derived emitters for DWARF and Wasm don’t use it. This adds avoidable per-method allocations/work on non-Windows NativeAOT builds. Consider only formatting the C# display name when targeting Windows (CodeView/PDB), and otherwise just pass methodName (or default).

 if (node is IMethodNode methodNode)
{
uint methodTypeIndex = _userDefinedTypeDescriptor.GetMethodFunctionIdTypeIndex(methodNode.Method);
Utf8String methodDisplayName = new Utf8String(CSharpTypeNameFormatter.Instance.FormatName(methodNode.Method));
EmitDebugFunctionInfo(methodTypeIndex, methodDisplayName, methodName, methodSymbol, debugNode);
}

[max-charlamb]

It looks like this changes the scheme for method name symbols. Can we rev the SymbolName contract to n2 so tooling can handle both formats?

runtime/src/coreclr/nativeaot/Runtime/datadescriptor/datadescriptor.inc

Line 259 in 714432e

CDAC_GLOBAL_CONTRACT(SymbolName, n1)

[MichalStrehovsky]

It looks like this changes the scheme for method name symbols. Can we rev the SymbolName contract to n2 so tooling can handle both formats?

It does not change COFF symbol names, the symbol name is still the same. It changes the CodeView S_GPROC32_ID name of the method. This is same as C++ where:

classFoo
{
voidBar(int x);
};
voidFoo::Bar(int x) { }

The COFF symbol name for Bar is ?Bar@Foo@@AEAAXH@Z but S_GPROC32_ID symbol record assigns it name Foo::Bar.

Do we still need to rev SymboName contract?

[max-charlamb]

I see, I misunderstood the change. However, I would still like to bump the version. This should give us flexibility in the future to support previous versions.

/c @noahfalk

[noahfalk]

I see, I misunderstood the change. However, I would still like to bump the version. This should give us flexibility in the future to support previous versions.

I think it would be fine to do and right now the consequences are minimal. One issue that I do think we still need to address is documenting an explicit data contract for this so everyone agrees what behavior is or isn't covered by the contract.

[MichalStrehovsky]

I think it would be fine to do and right now the consequences are minimal. One issue that I do think we still need to address is documenting an explicit data contract for this so everyone agrees what behavior is or isn't covered by the contract.

Does cDAC interpret the symbol names in any way? The symbol names are supposed to be unique and deterministic for the exact same app, but e.g. two different apps might already assign different names to the same thing and there can be various butterfly effects when something in an assembly changes. They are not meant to be stable or reversible like the C++ mangled names. We can't use a reversible/stable C++-like name mangling because .NET naming is ridiculously long (std in C++ vs System.Collections.Generics in .NET).

[Copilot]

Pull request overview

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

[jkotas]

Does cDAC interpret the symbol names in any way?

Yes, for limited set of symbols in CoreLib where the ambiguity should not be a problem. As @noahfalk mentioned this needs to be documented for clarity.

[Copilot]

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/coreclr/nativeaot/Runtime/datadescriptor/datadescriptor.inc:262

• Bumping the NativeAOT cDAC contract version for SymbolName from n1 to n2 has compatibility impact for dump readers, but this PR doesn't appear to include any corresponding contract shape/algorithm changes or managed reader updates. If this wasn't intentional, please revert the version bump; if it was intentional, the contract implementation + docs need to be updated in the same PR so readers can negotiate v2 successfully.

CDAC_GLOBAL_CONTRACT(StressLog, c2)
CDAC_GLOBAL_CONTRACT(Loader, n1)
CDAC_GLOBAL_CONTRACT(ExecutionManager, n1)
CDAC_GLOBAL_CONTRACT(SyncBlock, n1)
CDAC_GLOBAL_CONTRACT(SymbolName, n2)
CDAC_GLOBAL_CONTRACT(CodeVersions, n1)
CDAC_GLOBAL_CONTRACT(ReJIT, n1)
CDAC_GLOBAL_CONTRACT(PrecodeStubs, n1)

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Lite

[MichalStrehovsky]

Fixes#117309

• Make sure we have type information for the method and parameters show up in the stack
• Format method names like the VS debugger does instead of using symbol name
• Previously we'd skip emitting method debug information if there's no sequence points but I'm not clear on why so deleting that filter too
• this needs to be named this or the debugger will treat it as a regular parameter that is visible in the signature. AFAIK the only reason why we didn't use this originally was CppCodegen.

Before (notice no int n parameter):

After:

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR improves NativeAOT-generated debug information (primarily CodeView/PDB) so debuggers and tooling can show richer method/parameter type details and more user-friendly method names.

Changes:

• Emit a non-zero CodeView procedure type index (methodTypeIndex) and pass it through to the S_GPROC32_ID record.
• Introduce CSharpTypeNameFormatter and use it to provide a C#-style method display name in CodeView symbols instead of the mangled symbol name.
• Rename the implicit instance parameter from "___this" to "this" in ECMA PDB-backed parameter name enumeration.

Reviewed changes

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

Show a summary per file

File	Description
src/coreclr/tools/Common/TypeSystem/IL/EcmaMethodIL.Symbols.cs	Changes implicit instance parameter name emitted by PDB-backed parameter enumeration to "this".
src/coreclr/tools/Common/TypeSystem/Common/Utilities/CSharpTypeNameFormatter.cs	Adds a new formatter for producing C#-style type (and method owning-type) names.
src/coreclr/tools/aot/ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj	Links the new formatter into the ILCompiler.TypeSystem build.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs	Always computes and supplies method type ID and method display name during debug info emission.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs	Updates debug function info signature and sequence-point gating logic for DWARF emission.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs	Passes the new display name through to CodeView symbol emission; preserves line info gating.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CodeView/CodeViewSymbolsBuilder.cs	Writes methodTypeIndex and the new display name into the CodeView S_GPROC32_ID record.

[Copilot]

Pull request overview

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

Suppressed comments (3)

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs:129

• methodDisplayName is constructed for every IMethodNode during debug-info emission, but it’s not used by the DWARF path (e.g., UnixObjectWriter.Aot.cs ignores it) and the Wasm writer also ignores it. This adds per-method StringBuilder/string/UTF8 allocations in ObjectWritingOptions.GenerateDebugInfo builds on non-Windows targets. Consider computing the display name only in the CodeView/Coff path (or lazily in the override when needed) to avoid unnecessary work on other object writers.

 uint methodTypeIndex = _userDefinedTypeDescriptor.GetMethodFunctionIdTypeIndex(methodNode.Method);
Utf8String methodDisplayName = new Utf8String(CSharpTypeNameFormatter.Instance.FormatName(methodNode.Method));
EmitDebugFunctionInfo(methodTypeIndex, methodDisplayName, methodName, methodSymbol, debugNode);
}

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs:276

• debugNode.GetNativeSequencePoints() is enumerated for .Any() here, and then enumerated again later when passing sequence points into EmitLineInfo. If this enumerable is not a cheap materialized collection, this can add avoidable overhead. Consider capturing/materializing the sequence points once and reusing them for both the emptiness check and emission.

 if (debugNode.GetNativeSequencePoints().Any())
{
_dwarfBuilder.EmitLineInfo(
methodSymbol.SectionIndex,
section.SymbolName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs:228

• debugNode.GetNativeSequencePoints() is enumerated for .Any() here, and then enumerated again later when passing sequence points into EmitLineInfo. If this enumerable is not a cheap materialized collection, this can add avoidable overhead. Consider capturing/materializing the sequence points once and reusing them for both the emptiness check and emission.

 if (debugNode.GetNativeSequencePoints().Any())
{
debugSymbolsBuilder.EmitLineInfo(
_debugFileTableBuilder,
methodName,

[MichalStrehovsky]

Previously we'd skip emitting method debug information if there's no sequence points but I'm not clear on why so deleting that filter too

So this was used to paper over issues when no debug information for parameters is available. "No sequence points" was used to approximate "no parameter debug info", basically. DWARF debug info emission wasn't ready to deal with that. We now just emit no parameter debug information. This mostly affects compiler-generated IL.

Alternatively, we could name these parameters, but that sounds a bit problematic.

[Copilot]

Pull request overview

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

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs:228

• GetNativeSequencePoints() is implemented as an iterator on MethodCodeNode and does non-trivial work per enumeration. The current pattern enumerates it twice (once for Any(), again when emitting line info). Materializing the sequence points once avoids doing all that work twice per method.

 if (debugNode.GetNativeSequencePoints().Any())
{
debugSymbolsBuilder.EmitLineInfo(
_debugFileTableBuilder,
methodName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs:276

• GetNativeSequencePoints() is implemented as an iterator on MethodCodeNode and does non-trivial work per enumeration. The current pattern enumerates it twice (once for Any(), again when emitting line info). Materializing the sequence points once avoids doing all that work twice per method.

 if (debugNode.GetNativeSequencePoints().Any())
{
_dwarfBuilder.EmitLineInfo(
methodSymbol.SectionIndex,
section.SymbolName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs:128

• methodDisplayName is computed for every method node unconditionally, but the derived emitters for DWARF and Wasm don’t use it. This adds avoidable per-method allocations/work on non-Windows NativeAOT builds. Consider only formatting the C# display name when targeting Windows (CodeView/PDB), and otherwise just pass methodName (or default).

 if (node is IMethodNode methodNode)
{
uint methodTypeIndex = _userDefinedTypeDescriptor.GetMethodFunctionIdTypeIndex(methodNode.Method);
Utf8String methodDisplayName = new Utf8String(CSharpTypeNameFormatter.Instance.FormatName(methodNode.Method));
EmitDebugFunctionInfo(methodTypeIndex, methodDisplayName, methodName, methodSymbol, debugNode);
}

[max-charlamb]

It looks like this changes the scheme for method name symbols. Can we rev the SymbolName contract to n2 so tooling can handle both formats?

runtime/src/coreclr/nativeaot/Runtime/datadescriptor/datadescriptor.inc

Line 259 in 714432e

CDAC_GLOBAL_CONTRACT(SymbolName, n1)

[MichalStrehovsky]

It looks like this changes the scheme for method name symbols. Can we rev the SymbolName contract to n2 so tooling can handle both formats?

It does not change COFF symbol names, the symbol name is still the same. It changes the CodeView S_GPROC32_ID name of the method. This is same as C++ where:

classFoo
{
voidBar(int x);
};
voidFoo::Bar(int x) { }

The COFF symbol name for Bar is ?Bar@Foo@@AEAAXH@Z but S_GPROC32_ID symbol record assigns it name Foo::Bar.

Do we still need to rev SymboName contract?

[max-charlamb]

I see, I misunderstood the change. However, I would still like to bump the version. This should give us flexibility in the future to support previous versions.

/c @noahfalk

[noahfalk]

I see, I misunderstood the change. However, I would still like to bump the version. This should give us flexibility in the future to support previous versions.

I think it would be fine to do and right now the consequences are minimal. One issue that I do think we still need to address is documenting an explicit data contract for this so everyone agrees what behavior is or isn't covered by the contract.

[MichalStrehovsky]

I think it would be fine to do and right now the consequences are minimal. One issue that I do think we still need to address is documenting an explicit data contract for this so everyone agrees what behavior is or isn't covered by the contract.

Does cDAC interpret the symbol names in any way? The symbol names are supposed to be unique and deterministic for the exact same app, but e.g. two different apps might already assign different names to the same thing and there can be various butterfly effects when something in an assembly changes. They are not meant to be stable or reversible like the C++ mangled names. We can't use a reversible/stable C++-like name mangling because .NET naming is ridiculously long (std in C++ vs System.Collections.Generics in .NET).

[Copilot]

Pull request overview

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

[jkotas]

Does cDAC interpret the symbol names in any way?

Yes, for limited set of symbols in CoreLib where the ambiguity should not be a problem. As @noahfalk mentioned this needs to be documented for clarity.

[Copilot]

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/coreclr/nativeaot/Runtime/datadescriptor/datadescriptor.inc:262

• Bumping the NativeAOT cDAC contract version for SymbolName from n1 to n2 has compatibility impact for dump readers, but this PR doesn't appear to include any corresponding contract shape/algorithm changes or managed reader updates. If this wasn't intentional, please revert the version bump; if it was intentional, the contract implementation + docs need to be updated in the same PR so readers can negotiate v2 successfully.

CDAC_GLOBAL_CONTRACT(StressLog, c2)
CDAC_GLOBAL_CONTRACT(Loader, n1)
CDAC_GLOBAL_CONTRACT(ExecutionManager, n1)
CDAC_GLOBAL_CONTRACT(SyncBlock, n1)
CDAC_GLOBAL_CONTRACT(SymbolName, n2)
CDAC_GLOBAL_CONTRACT(CodeVersions, n1)
CDAC_GLOBAL_CONTRACT(ReJIT, n1)
CDAC_GLOBAL_CONTRACT(PrecodeStubs, n1)

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Lite

[MichalStrehovsky]

Fixes#117309

• Make sure we have type information for the method and parameters show up in the stack
• Format method names like the VS debugger does instead of using symbol name
• Previously we'd skip emitting method debug information if there's no sequence points but I'm not clear on why so deleting that filter too
• this needs to be named this or the debugger will treat it as a regular parameter that is visible in the signature. AFAIK the only reason why we didn't use this originally was CppCodegen.

Before (notice no int n parameter):

After:

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR improves NativeAOT-generated debug information (primarily CodeView/PDB) so debuggers and tooling can show richer method/parameter type details and more user-friendly method names.

Changes:

• Emit a non-zero CodeView procedure type index (methodTypeIndex) and pass it through to the S_GPROC32_ID record.
• Introduce CSharpTypeNameFormatter and use it to provide a C#-style method display name in CodeView symbols instead of the mangled symbol name.
• Rename the implicit instance parameter from "___this" to "this" in ECMA PDB-backed parameter name enumeration.

Reviewed changes

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

Show a summary per file

File	Description
src/coreclr/tools/Common/TypeSystem/IL/EcmaMethodIL.Symbols.cs	Changes implicit instance parameter name emitted by PDB-backed parameter enumeration to "this".
src/coreclr/tools/Common/TypeSystem/Common/Utilities/CSharpTypeNameFormatter.cs	Adds a new formatter for producing C#-style type (and method owning-type) names.
src/coreclr/tools/aot/ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj	Links the new formatter into the ILCompiler.TypeSystem build.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs	Always computes and supplies method type ID and method display name during debug info emission.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs	Updates debug function info signature and sequence-point gating logic for DWARF emission.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs	Passes the new display name through to CodeView symbol emission; preserves line info gating.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CodeView/CodeViewSymbolsBuilder.cs	Writes methodTypeIndex and the new display name into the CodeView S_GPROC32_ID record.

[Copilot]

Pull request overview

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

Suppressed comments (3)

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs:129

• methodDisplayName is constructed for every IMethodNode during debug-info emission, but it’s not used by the DWARF path (e.g., UnixObjectWriter.Aot.cs ignores it) and the Wasm writer also ignores it. This adds per-method StringBuilder/string/UTF8 allocations in ObjectWritingOptions.GenerateDebugInfo builds on non-Windows targets. Consider computing the display name only in the CodeView/Coff path (or lazily in the override when needed) to avoid unnecessary work on other object writers.

 uint methodTypeIndex = _userDefinedTypeDescriptor.GetMethodFunctionIdTypeIndex(methodNode.Method);
Utf8String methodDisplayName = new Utf8String(CSharpTypeNameFormatter.Instance.FormatName(methodNode.Method));
EmitDebugFunctionInfo(methodTypeIndex, methodDisplayName, methodName, methodSymbol, debugNode);
}

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs:276

• debugNode.GetNativeSequencePoints() is enumerated for .Any() here, and then enumerated again later when passing sequence points into EmitLineInfo. If this enumerable is not a cheap materialized collection, this can add avoidable overhead. Consider capturing/materializing the sequence points once and reusing them for both the emptiness check and emission.

 if (debugNode.GetNativeSequencePoints().Any())
{
_dwarfBuilder.EmitLineInfo(
methodSymbol.SectionIndex,
section.SymbolName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs:228

• debugNode.GetNativeSequencePoints() is enumerated for .Any() here, and then enumerated again later when passing sequence points into EmitLineInfo. If this enumerable is not a cheap materialized collection, this can add avoidable overhead. Consider capturing/materializing the sequence points once and reusing them for both the emptiness check and emission.

 if (debugNode.GetNativeSequencePoints().Any())
{
debugSymbolsBuilder.EmitLineInfo(
_debugFileTableBuilder,
methodName,

[MichalStrehovsky]

Previously we'd skip emitting method debug information if there's no sequence points but I'm not clear on why so deleting that filter too

So this was used to paper over issues when no debug information for parameters is available. "No sequence points" was used to approximate "no parameter debug info", basically. DWARF debug info emission wasn't ready to deal with that. We now just emit no parameter debug information. This mostly affects compiler-generated IL.

Alternatively, we could name these parameters, but that sounds a bit problematic.

[Copilot]

Pull request overview

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

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs:228

• GetNativeSequencePoints() is implemented as an iterator on MethodCodeNode and does non-trivial work per enumeration. The current pattern enumerates it twice (once for Any(), again when emitting line info). Materializing the sequence points once avoids doing all that work twice per method.

 if (debugNode.GetNativeSequencePoints().Any())
{
debugSymbolsBuilder.EmitLineInfo(
_debugFileTableBuilder,
methodName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs:276

• GetNativeSequencePoints() is implemented as an iterator on MethodCodeNode and does non-trivial work per enumeration. The current pattern enumerates it twice (once for Any(), again when emitting line info). Materializing the sequence points once avoids doing all that work twice per method.

 if (debugNode.GetNativeSequencePoints().Any())
{
_dwarfBuilder.EmitLineInfo(
methodSymbol.SectionIndex,
section.SymbolName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs:128

• methodDisplayName is computed for every method node unconditionally, but the derived emitters for DWARF and Wasm don’t use it. This adds avoidable per-method allocations/work on non-Windows NativeAOT builds. Consider only formatting the C# display name when targeting Windows (CodeView/PDB), and otherwise just pass methodName (or default).

 if (node is IMethodNode methodNode)
{
uint methodTypeIndex = _userDefinedTypeDescriptor.GetMethodFunctionIdTypeIndex(methodNode.Method);
Utf8String methodDisplayName = new Utf8String(CSharpTypeNameFormatter.Instance.FormatName(methodNode.Method));
EmitDebugFunctionInfo(methodTypeIndex, methodDisplayName, methodName, methodSymbol, debugNode);
}

[max-charlamb]

It looks like this changes the scheme for method name symbols. Can we rev the SymbolName contract to n2 so tooling can handle both formats?

runtime/src/coreclr/nativeaot/Runtime/datadescriptor/datadescriptor.inc

Line 259 in 714432e

CDAC_GLOBAL_CONTRACT(SymbolName, n1)

[MichalStrehovsky]

It looks like this changes the scheme for method name symbols. Can we rev the SymbolName contract to n2 so tooling can handle both formats?

It does not change COFF symbol names, the symbol name is still the same. It changes the CodeView S_GPROC32_ID name of the method. This is same as C++ where:

classFoo
{
voidBar(int x);
};
voidFoo::Bar(int x) { }

The COFF symbol name for Bar is ?Bar@Foo@@AEAAXH@Z but S_GPROC32_ID symbol record assigns it name Foo::Bar.

Do we still need to rev SymboName contract?

[max-charlamb]

I see, I misunderstood the change. However, I would still like to bump the version. This should give us flexibility in the future to support previous versions.

/c @noahfalk

[noahfalk]

I see, I misunderstood the change. However, I would still like to bump the version. This should give us flexibility in the future to support previous versions.

I think it would be fine to do and right now the consequences are minimal. One issue that I do think we still need to address is documenting an explicit data contract for this so everyone agrees what behavior is or isn't covered by the contract.

[MichalStrehovsky]

I think it would be fine to do and right now the consequences are minimal. One issue that I do think we still need to address is documenting an explicit data contract for this so everyone agrees what behavior is or isn't covered by the contract.

Does cDAC interpret the symbol names in any way? The symbol names are supposed to be unique and deterministic for the exact same app, but e.g. two different apps might already assign different names to the same thing and there can be various butterfly effects when something in an assembly changes. They are not meant to be stable or reversible like the C++ mangled names. We can't use a reversible/stable C++-like name mangling because .NET naming is ridiculously long (std in C++ vs System.Collections.Generics in .NET).

[Copilot]

Pull request overview

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

[jkotas]

Does cDAC interpret the symbol names in any way?

Yes, for limited set of symbols in CoreLib where the ambiguity should not be a problem. As @noahfalk mentioned this needs to be documented for clarity.

[Copilot]

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/coreclr/nativeaot/Runtime/datadescriptor/datadescriptor.inc:262

• Bumping the NativeAOT cDAC contract version for SymbolName from n1 to n2 has compatibility impact for dump readers, but this PR doesn't appear to include any corresponding contract shape/algorithm changes or managed reader updates. If this wasn't intentional, please revert the version bump; if it was intentional, the contract implementation + docs need to be updated in the same PR so readers can negotiate v2 successfully.

CDAC_GLOBAL_CONTRACT(StressLog, c2)
CDAC_GLOBAL_CONTRACT(Loader, n1)
CDAC_GLOBAL_CONTRACT(ExecutionManager, n1)
CDAC_GLOBAL_CONTRACT(SyncBlock, n1)
CDAC_GLOBAL_CONTRACT(SymbolName, n2)
CDAC_GLOBAL_CONTRACT(CodeVersions, n1)
CDAC_GLOBAL_CONTRACT(ReJIT, n1)
CDAC_GLOBAL_CONTRACT(PrecodeStubs, n1)

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Lite

[MichalStrehovsky]

Fixes#117309

• Make sure we have type information for the method and parameters show up in the stack
• Format method names like the VS debugger does instead of using symbol name
• Previously we'd skip emitting method debug information if there's no sequence points but I'm not clear on why so deleting that filter too
• this needs to be named this or the debugger will treat it as a regular parameter that is visible in the signature. AFAIK the only reason why we didn't use this originally was CppCodegen.

Before (notice no int n parameter):

After:

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR improves NativeAOT-generated debug information (primarily CodeView/PDB) so debuggers and tooling can show richer method/parameter type details and more user-friendly method names.

Changes:

• Emit a non-zero CodeView procedure type index (methodTypeIndex) and pass it through to the S_GPROC32_ID record.
• Introduce CSharpTypeNameFormatter and use it to provide a C#-style method display name in CodeView symbols instead of the mangled symbol name.
• Rename the implicit instance parameter from "___this" to "this" in ECMA PDB-backed parameter name enumeration.

Reviewed changes

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

Show a summary per file

File	Description
src/coreclr/tools/Common/TypeSystem/IL/EcmaMethodIL.Symbols.cs	Changes implicit instance parameter name emitted by PDB-backed parameter enumeration to "this".
src/coreclr/tools/Common/TypeSystem/Common/Utilities/CSharpTypeNameFormatter.cs	Adds a new formatter for producing C#-style type (and method owning-type) names.
src/coreclr/tools/aot/ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj	Links the new formatter into the ILCompiler.TypeSystem build.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs	Always computes and supplies method type ID and method display name during debug info emission.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs	Updates debug function info signature and sequence-point gating logic for DWARF emission.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs	Passes the new display name through to CodeView symbol emission; preserves line info gating.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CodeView/CodeViewSymbolsBuilder.cs	Writes methodTypeIndex and the new display name into the CodeView S_GPROC32_ID record.

[Copilot]

Pull request overview

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

Suppressed comments (3)

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs:129

• methodDisplayName is constructed for every IMethodNode during debug-info emission, but it’s not used by the DWARF path (e.g., UnixObjectWriter.Aot.cs ignores it) and the Wasm writer also ignores it. This adds per-method StringBuilder/string/UTF8 allocations in ObjectWritingOptions.GenerateDebugInfo builds on non-Windows targets. Consider computing the display name only in the CodeView/Coff path (or lazily in the override when needed) to avoid unnecessary work on other object writers.

 uint methodTypeIndex = _userDefinedTypeDescriptor.GetMethodFunctionIdTypeIndex(methodNode.Method);
Utf8String methodDisplayName = new Utf8String(CSharpTypeNameFormatter.Instance.FormatName(methodNode.Method));
EmitDebugFunctionInfo(methodTypeIndex, methodDisplayName, methodName, methodSymbol, debugNode);
}

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs:276

• debugNode.GetNativeSequencePoints() is enumerated for .Any() here, and then enumerated again later when passing sequence points into EmitLineInfo. If this enumerable is not a cheap materialized collection, this can add avoidable overhead. Consider capturing/materializing the sequence points once and reusing them for both the emptiness check and emission.

 if (debugNode.GetNativeSequencePoints().Any())
{
_dwarfBuilder.EmitLineInfo(
methodSymbol.SectionIndex,
section.SymbolName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs:228

• debugNode.GetNativeSequencePoints() is enumerated for .Any() here, and then enumerated again later when passing sequence points into EmitLineInfo. If this enumerable is not a cheap materialized collection, this can add avoidable overhead. Consider capturing/materializing the sequence points once and reusing them for both the emptiness check and emission.

 if (debugNode.GetNativeSequencePoints().Any())
{
debugSymbolsBuilder.EmitLineInfo(
_debugFileTableBuilder,
methodName,

[MichalStrehovsky]

Previously we'd skip emitting method debug information if there's no sequence points but I'm not clear on why so deleting that filter too

So this was used to paper over issues when no debug information for parameters is available. "No sequence points" was used to approximate "no parameter debug info", basically. DWARF debug info emission wasn't ready to deal with that. We now just emit no parameter debug information. This mostly affects compiler-generated IL.

Alternatively, we could name these parameters, but that sounds a bit problematic.

[Copilot]

Pull request overview

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

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs:228

• GetNativeSequencePoints() is implemented as an iterator on MethodCodeNode and does non-trivial work per enumeration. The current pattern enumerates it twice (once for Any(), again when emitting line info). Materializing the sequence points once avoids doing all that work twice per method.

 if (debugNode.GetNativeSequencePoints().Any())
{
debugSymbolsBuilder.EmitLineInfo(
_debugFileTableBuilder,
methodName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs:276

• GetNativeSequencePoints() is implemented as an iterator on MethodCodeNode and does non-trivial work per enumeration. The current pattern enumerates it twice (once for Any(), again when emitting line info). Materializing the sequence points once avoids doing all that work twice per method.

 if (debugNode.GetNativeSequencePoints().Any())
{
_dwarfBuilder.EmitLineInfo(
methodSymbol.SectionIndex,
section.SymbolName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs:128

• methodDisplayName is computed for every method node unconditionally, but the derived emitters for DWARF and Wasm don’t use it. This adds avoidable per-method allocations/work on non-Windows NativeAOT builds. Consider only formatting the C# display name when targeting Windows (CodeView/PDB), and otherwise just pass methodName (or default).

 if (node is IMethodNode methodNode)
{
uint methodTypeIndex = _userDefinedTypeDescriptor.GetMethodFunctionIdTypeIndex(methodNode.Method);
Utf8String methodDisplayName = new Utf8String(CSharpTypeNameFormatter.Instance.FormatName(methodNode.Method));
EmitDebugFunctionInfo(methodTypeIndex, methodDisplayName, methodName, methodSymbol, debugNode);
}

[max-charlamb]

It looks like this changes the scheme for method name symbols. Can we rev the SymbolName contract to n2 so tooling can handle both formats?

runtime/src/coreclr/nativeaot/Runtime/datadescriptor/datadescriptor.inc

Line 259 in 714432e

CDAC_GLOBAL_CONTRACT(SymbolName, n1)

[MichalStrehovsky]

It looks like this changes the scheme for method name symbols. Can we rev the SymbolName contract to n2 so tooling can handle both formats?

It does not change COFF symbol names, the symbol name is still the same. It changes the CodeView S_GPROC32_ID name of the method. This is same as C++ where:

classFoo
{
voidBar(int x);
};
voidFoo::Bar(int x) { }

The COFF symbol name for Bar is ?Bar@Foo@@AEAAXH@Z but S_GPROC32_ID symbol record assigns it name Foo::Bar.

Do we still need to rev SymboName contract?

[max-charlamb]

I see, I misunderstood the change. However, I would still like to bump the version. This should give us flexibility in the future to support previous versions.

/c @noahfalk

[noahfalk]

I see, I misunderstood the change. However, I would still like to bump the version. This should give us flexibility in the future to support previous versions.

I think it would be fine to do and right now the consequences are minimal. One issue that I do think we still need to address is documenting an explicit data contract for this so everyone agrees what behavior is or isn't covered by the contract.

[MichalStrehovsky]

I think it would be fine to do and right now the consequences are minimal. One issue that I do think we still need to address is documenting an explicit data contract for this so everyone agrees what behavior is or isn't covered by the contract.

Does cDAC interpret the symbol names in any way? The symbol names are supposed to be unique and deterministic for the exact same app, but e.g. two different apps might already assign different names to the same thing and there can be various butterfly effects when something in an assembly changes. They are not meant to be stable or reversible like the C++ mangled names. We can't use a reversible/stable C++-like name mangling because .NET naming is ridiculously long (std in C++ vs System.Collections.Generics in .NET).

[Copilot]

Pull request overview

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

[jkotas]

Does cDAC interpret the symbol names in any way?

Yes, for limited set of symbols in CoreLib where the ambiguity should not be a problem. As @noahfalk mentioned this needs to be documented for clarity.

[Copilot]

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/coreclr/nativeaot/Runtime/datadescriptor/datadescriptor.inc:262

• Bumping the NativeAOT cDAC contract version for SymbolName from n1 to n2 has compatibility impact for dump readers, but this PR doesn't appear to include any corresponding contract shape/algorithm changes or managed reader updates. If this wasn't intentional, please revert the version bump; if it was intentional, the contract implementation + docs need to be updated in the same PR so readers can negotiate v2 successfully.

CDAC_GLOBAL_CONTRACT(StressLog, c2)
CDAC_GLOBAL_CONTRACT(Loader, n1)
CDAC_GLOBAL_CONTRACT(ExecutionManager, n1)
CDAC_GLOBAL_CONTRACT(SyncBlock, n1)
CDAC_GLOBAL_CONTRACT(SymbolName, n2)
CDAC_GLOBAL_CONTRACT(CodeVersions, n1)
CDAC_GLOBAL_CONTRACT(ReJIT, n1)
CDAC_GLOBAL_CONTRACT(PrecodeStubs, n1)

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Lite

[MichalStrehovsky]

Fixes#117309

• Make sure we have type information for the method and parameters show up in the stack
• Format method names like the VS debugger does instead of using symbol name
• Previously we'd skip emitting method debug information if there's no sequence points but I'm not clear on why so deleting that filter too
• this needs to be named this or the debugger will treat it as a regular parameter that is visible in the signature. AFAIK the only reason why we didn't use this originally was CppCodegen.

Before (notice no int n parameter):

After:

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR improves NativeAOT-generated debug information (primarily CodeView/PDB) so debuggers and tooling can show richer method/parameter type details and more user-friendly method names.

Changes:

• Emit a non-zero CodeView procedure type index (methodTypeIndex) and pass it through to the S_GPROC32_ID record.
• Introduce CSharpTypeNameFormatter and use it to provide a C#-style method display name in CodeView symbols instead of the mangled symbol name.
• Rename the implicit instance parameter from "___this" to "this" in ECMA PDB-backed parameter name enumeration.

Reviewed changes

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

Show a summary per file

File	Description
src/coreclr/tools/Common/TypeSystem/IL/EcmaMethodIL.Symbols.cs	Changes implicit instance parameter name emitted by PDB-backed parameter enumeration to "this".
src/coreclr/tools/Common/TypeSystem/Common/Utilities/CSharpTypeNameFormatter.cs	Adds a new formatter for producing C#-style type (and method owning-type) names.
src/coreclr/tools/aot/ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj	Links the new formatter into the ILCompiler.TypeSystem build.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs	Always computes and supplies method type ID and method display name during debug info emission.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs	Updates debug function info signature and sequence-point gating logic for DWARF emission.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs	Passes the new display name through to CodeView symbol emission; preserves line info gating.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CodeView/CodeViewSymbolsBuilder.cs	Writes methodTypeIndex and the new display name into the CodeView S_GPROC32_ID record.

[Copilot]

Pull request overview

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

Suppressed comments (3)

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs:129

• methodDisplayName is constructed for every IMethodNode during debug-info emission, but it’s not used by the DWARF path (e.g., UnixObjectWriter.Aot.cs ignores it) and the Wasm writer also ignores it. This adds per-method StringBuilder/string/UTF8 allocations in ObjectWritingOptions.GenerateDebugInfo builds on non-Windows targets. Consider computing the display name only in the CodeView/Coff path (or lazily in the override when needed) to avoid unnecessary work on other object writers.

 uint methodTypeIndex = _userDefinedTypeDescriptor.GetMethodFunctionIdTypeIndex(methodNode.Method);
Utf8String methodDisplayName = new Utf8String(CSharpTypeNameFormatter.Instance.FormatName(methodNode.Method));
EmitDebugFunctionInfo(methodTypeIndex, methodDisplayName, methodName, methodSymbol, debugNode);
}

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs:276

• debugNode.GetNativeSequencePoints() is enumerated for .Any() here, and then enumerated again later when passing sequence points into EmitLineInfo. If this enumerable is not a cheap materialized collection, this can add avoidable overhead. Consider capturing/materializing the sequence points once and reusing them for both the emptiness check and emission.

 if (debugNode.GetNativeSequencePoints().Any())
{
_dwarfBuilder.EmitLineInfo(
methodSymbol.SectionIndex,
section.SymbolName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs:228

• debugNode.GetNativeSequencePoints() is enumerated for .Any() here, and then enumerated again later when passing sequence points into EmitLineInfo. If this enumerable is not a cheap materialized collection, this can add avoidable overhead. Consider capturing/materializing the sequence points once and reusing them for both the emptiness check and emission.

 if (debugNode.GetNativeSequencePoints().Any())
{
debugSymbolsBuilder.EmitLineInfo(
_debugFileTableBuilder,
methodName,

[MichalStrehovsky]

Previously we'd skip emitting method debug information if there's no sequence points but I'm not clear on why so deleting that filter too

So this was used to paper over issues when no debug information for parameters is available. "No sequence points" was used to approximate "no parameter debug info", basically. DWARF debug info emission wasn't ready to deal with that. We now just emit no parameter debug information. This mostly affects compiler-generated IL.

Alternatively, we could name these parameters, but that sounds a bit problematic.

[Copilot]

Pull request overview

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

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs:228

• GetNativeSequencePoints() is implemented as an iterator on MethodCodeNode and does non-trivial work per enumeration. The current pattern enumerates it twice (once for Any(), again when emitting line info). Materializing the sequence points once avoids doing all that work twice per method.

 if (debugNode.GetNativeSequencePoints().Any())
{
debugSymbolsBuilder.EmitLineInfo(
_debugFileTableBuilder,
methodName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs:276

• GetNativeSequencePoints() is implemented as an iterator on MethodCodeNode and does non-trivial work per enumeration. The current pattern enumerates it twice (once for Any(), again when emitting line info). Materializing the sequence points once avoids doing all that work twice per method.

 if (debugNode.GetNativeSequencePoints().Any())
{
_dwarfBuilder.EmitLineInfo(
methodSymbol.SectionIndex,
section.SymbolName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs:128

• methodDisplayName is computed for every method node unconditionally, but the derived emitters for DWARF and Wasm don’t use it. This adds avoidable per-method allocations/work on non-Windows NativeAOT builds. Consider only formatting the C# display name when targeting Windows (CodeView/PDB), and otherwise just pass methodName (or default).

 if (node is IMethodNode methodNode)
{
uint methodTypeIndex = _userDefinedTypeDescriptor.GetMethodFunctionIdTypeIndex(methodNode.Method);
Utf8String methodDisplayName = new Utf8String(CSharpTypeNameFormatter.Instance.FormatName(methodNode.Method));
EmitDebugFunctionInfo(methodTypeIndex, methodDisplayName, methodName, methodSymbol, debugNode);
}

[max-charlamb]

It looks like this changes the scheme for method name symbols. Can we rev the SymbolName contract to n2 so tooling can handle both formats?

runtime/src/coreclr/nativeaot/Runtime/datadescriptor/datadescriptor.inc

Line 259 in 714432e

CDAC_GLOBAL_CONTRACT(SymbolName, n1)

[MichalStrehovsky]

It looks like this changes the scheme for method name symbols. Can we rev the SymbolName contract to n2 so tooling can handle both formats?

It does not change COFF symbol names, the symbol name is still the same. It changes the CodeView S_GPROC32_ID name of the method. This is same as C++ where:

classFoo
{
voidBar(int x);
};
voidFoo::Bar(int x) { }

The COFF symbol name for Bar is ?Bar@Foo@@AEAAXH@Z but S_GPROC32_ID symbol record assigns it name Foo::Bar.

Do we still need to rev SymboName contract?

[max-charlamb]

I see, I misunderstood the change. However, I would still like to bump the version. This should give us flexibility in the future to support previous versions.

/c @noahfalk

[noahfalk]

I see, I misunderstood the change. However, I would still like to bump the version. This should give us flexibility in the future to support previous versions.

I think it would be fine to do and right now the consequences are minimal. One issue that I do think we still need to address is documenting an explicit data contract for this so everyone agrees what behavior is or isn't covered by the contract.

[MichalStrehovsky]

I think it would be fine to do and right now the consequences are minimal. One issue that I do think we still need to address is documenting an explicit data contract for this so everyone agrees what behavior is or isn't covered by the contract.

Does cDAC interpret the symbol names in any way? The symbol names are supposed to be unique and deterministic for the exact same app, but e.g. two different apps might already assign different names to the same thing and there can be various butterfly effects when something in an assembly changes. They are not meant to be stable or reversible like the C++ mangled names. We can't use a reversible/stable C++-like name mangling because .NET naming is ridiculously long (std in C++ vs System.Collections.Generics in .NET).

[Copilot]

Pull request overview

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

[jkotas]

Does cDAC interpret the symbol names in any way?

Yes, for limited set of symbols in CoreLib where the ambiguity should not be a problem. As @noahfalk mentioned this needs to be documented for clarity.

[Copilot]

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/coreclr/nativeaot/Runtime/datadescriptor/datadescriptor.inc:262

• Bumping the NativeAOT cDAC contract version for SymbolName from n1 to n2 has compatibility impact for dump readers, but this PR doesn't appear to include any corresponding contract shape/algorithm changes or managed reader updates. If this wasn't intentional, please revert the version bump; if it was intentional, the contract implementation + docs need to be updated in the same PR so readers can negotiate v2 successfully.

CDAC_GLOBAL_CONTRACT(StressLog, c2)
CDAC_GLOBAL_CONTRACT(Loader, n1)
CDAC_GLOBAL_CONTRACT(ExecutionManager, n1)
CDAC_GLOBAL_CONTRACT(SyncBlock, n1)
CDAC_GLOBAL_CONTRACT(SymbolName, n2)
CDAC_GLOBAL_CONTRACT(CodeVersions, n1)
CDAC_GLOBAL_CONTRACT(ReJIT, n1)
CDAC_GLOBAL_CONTRACT(PrecodeStubs, n1)

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Lite

[MichalStrehovsky]

Fixes#117309

• Make sure we have type information for the method and parameters show up in the stack
• Format method names like the VS debugger does instead of using symbol name
• Previously we'd skip emitting method debug information if there's no sequence points but I'm not clear on why so deleting that filter too
• this needs to be named this or the debugger will treat it as a regular parameter that is visible in the signature. AFAIK the only reason why we didn't use this originally was CppCodegen.

Before (notice no int n parameter):

After:

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR improves NativeAOT-generated debug information (primarily CodeView/PDB) so debuggers and tooling can show richer method/parameter type details and more user-friendly method names.

Changes:

• Emit a non-zero CodeView procedure type index (methodTypeIndex) and pass it through to the S_GPROC32_ID record.
• Introduce CSharpTypeNameFormatter and use it to provide a C#-style method display name in CodeView symbols instead of the mangled symbol name.
• Rename the implicit instance parameter from "___this" to "this" in ECMA PDB-backed parameter name enumeration.

Reviewed changes

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

Show a summary per file

File	Description
src/coreclr/tools/Common/TypeSystem/IL/EcmaMethodIL.Symbols.cs	Changes implicit instance parameter name emitted by PDB-backed parameter enumeration to "this".
src/coreclr/tools/Common/TypeSystem/Common/Utilities/CSharpTypeNameFormatter.cs	Adds a new formatter for producing C#-style type (and method owning-type) names.
src/coreclr/tools/aot/ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj	Links the new formatter into the ILCompiler.TypeSystem build.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs	Always computes and supplies method type ID and method display name during debug info emission.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs	Updates debug function info signature and sequence-point gating logic for DWARF emission.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs	Passes the new display name through to CodeView symbol emission; preserves line info gating.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CodeView/CodeViewSymbolsBuilder.cs	Writes methodTypeIndex and the new display name into the CodeView S_GPROC32_ID record.

[Copilot]

Pull request overview

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

Suppressed comments (3)

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs:129

• methodDisplayName is constructed for every IMethodNode during debug-info emission, but it’s not used by the DWARF path (e.g., UnixObjectWriter.Aot.cs ignores it) and the Wasm writer also ignores it. This adds per-method StringBuilder/string/UTF8 allocations in ObjectWritingOptions.GenerateDebugInfo builds on non-Windows targets. Consider computing the display name only in the CodeView/Coff path (or lazily in the override when needed) to avoid unnecessary work on other object writers.

 uint methodTypeIndex = _userDefinedTypeDescriptor.GetMethodFunctionIdTypeIndex(methodNode.Method);
Utf8String methodDisplayName = new Utf8String(CSharpTypeNameFormatter.Instance.FormatName(methodNode.Method));
EmitDebugFunctionInfo(methodTypeIndex, methodDisplayName, methodName, methodSymbol, debugNode);
}

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs:276

• debugNode.GetNativeSequencePoints() is enumerated for .Any() here, and then enumerated again later when passing sequence points into EmitLineInfo. If this enumerable is not a cheap materialized collection, this can add avoidable overhead. Consider capturing/materializing the sequence points once and reusing them for both the emptiness check and emission.

 if (debugNode.GetNativeSequencePoints().Any())
{
_dwarfBuilder.EmitLineInfo(
methodSymbol.SectionIndex,
section.SymbolName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs:228

• debugNode.GetNativeSequencePoints() is enumerated for .Any() here, and then enumerated again later when passing sequence points into EmitLineInfo. If this enumerable is not a cheap materialized collection, this can add avoidable overhead. Consider capturing/materializing the sequence points once and reusing them for both the emptiness check and emission.

 if (debugNode.GetNativeSequencePoints().Any())
{
debugSymbolsBuilder.EmitLineInfo(
_debugFileTableBuilder,
methodName,

[MichalStrehovsky]

Previously we'd skip emitting method debug information if there's no sequence points but I'm not clear on why so deleting that filter too

So this was used to paper over issues when no debug information for parameters is available. "No sequence points" was used to approximate "no parameter debug info", basically. DWARF debug info emission wasn't ready to deal with that. We now just emit no parameter debug information. This mostly affects compiler-generated IL.

Alternatively, we could name these parameters, but that sounds a bit problematic.

[Copilot]

Pull request overview

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

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs:228

• GetNativeSequencePoints() is implemented as an iterator on MethodCodeNode and does non-trivial work per enumeration. The current pattern enumerates it twice (once for Any(), again when emitting line info). Materializing the sequence points once avoids doing all that work twice per method.

 if (debugNode.GetNativeSequencePoints().Any())
{
debugSymbolsBuilder.EmitLineInfo(
_debugFileTableBuilder,
methodName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs:276

• GetNativeSequencePoints() is implemented as an iterator on MethodCodeNode and does non-trivial work per enumeration. The current pattern enumerates it twice (once for Any(), again when emitting line info). Materializing the sequence points once avoids doing all that work twice per method.

 if (debugNode.GetNativeSequencePoints().Any())
{
_dwarfBuilder.EmitLineInfo(
methodSymbol.SectionIndex,
section.SymbolName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs:128

• methodDisplayName is computed for every method node unconditionally, but the derived emitters for DWARF and Wasm don’t use it. This adds avoidable per-method allocations/work on non-Windows NativeAOT builds. Consider only formatting the C# display name when targeting Windows (CodeView/PDB), and otherwise just pass methodName (or default).

 if (node is IMethodNode methodNode)
{
uint methodTypeIndex = _userDefinedTypeDescriptor.GetMethodFunctionIdTypeIndex(methodNode.Method);
Utf8String methodDisplayName = new Utf8String(CSharpTypeNameFormatter.Instance.FormatName(methodNode.Method));
EmitDebugFunctionInfo(methodTypeIndex, methodDisplayName, methodName, methodSymbol, debugNode);
}

[max-charlamb]

It looks like this changes the scheme for method name symbols. Can we rev the SymbolName contract to n2 so tooling can handle both formats?

runtime/src/coreclr/nativeaot/Runtime/datadescriptor/datadescriptor.inc

Line 259 in 714432e

CDAC_GLOBAL_CONTRACT(SymbolName, n1)

[MichalStrehovsky]

It looks like this changes the scheme for method name symbols. Can we rev the SymbolName contract to n2 so tooling can handle both formats?

It does not change COFF symbol names, the symbol name is still the same. It changes the CodeView S_GPROC32_ID name of the method. This is same as C++ where:

classFoo
{
voidBar(int x);
};
voidFoo::Bar(int x) { }

The COFF symbol name for Bar is ?Bar@Foo@@AEAAXH@Z but S_GPROC32_ID symbol record assigns it name Foo::Bar.

Do we still need to rev SymboName contract?

[max-charlamb]

I see, I misunderstood the change. However, I would still like to bump the version. This should give us flexibility in the future to support previous versions.

/c @noahfalk

[noahfalk]

I see, I misunderstood the change. However, I would still like to bump the version. This should give us flexibility in the future to support previous versions.

I think it would be fine to do and right now the consequences are minimal. One issue that I do think we still need to address is documenting an explicit data contract for this so everyone agrees what behavior is or isn't covered by the contract.

[MichalStrehovsky]

I think it would be fine to do and right now the consequences are minimal. One issue that I do think we still need to address is documenting an explicit data contract for this so everyone agrees what behavior is or isn't covered by the contract.

Does cDAC interpret the symbol names in any way? The symbol names are supposed to be unique and deterministic for the exact same app, but e.g. two different apps might already assign different names to the same thing and there can be various butterfly effects when something in an assembly changes. They are not meant to be stable or reversible like the C++ mangled names. We can't use a reversible/stable C++-like name mangling because .NET naming is ridiculously long (std in C++ vs System.Collections.Generics in .NET).

[Copilot]

Pull request overview

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

[jkotas]

Does cDAC interpret the symbol names in any way?

Yes, for limited set of symbols in CoreLib where the ambiguity should not be a problem. As @noahfalk mentioned this needs to be documented for clarity.

[Copilot]

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/coreclr/nativeaot/Runtime/datadescriptor/datadescriptor.inc:262

• Bumping the NativeAOT cDAC contract version for SymbolName from n1 to n2 has compatibility impact for dump readers, but this PR doesn't appear to include any corresponding contract shape/algorithm changes or managed reader updates. If this wasn't intentional, please revert the version bump; if it was intentional, the contract implementation + docs need to be updated in the same PR so readers can negotiate v2 successfully.

CDAC_GLOBAL_CONTRACT(StressLog, c2)
CDAC_GLOBAL_CONTRACT(Loader, n1)
CDAC_GLOBAL_CONTRACT(ExecutionManager, n1)
CDAC_GLOBAL_CONTRACT(SyncBlock, n1)
CDAC_GLOBAL_CONTRACT(SymbolName, n2)
CDAC_GLOBAL_CONTRACT(CodeVersions, n1)
CDAC_GLOBAL_CONTRACT(ReJIT, n1)
CDAC_GLOBAL_CONTRACT(PrecodeStubs, n1)

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Lite

[MichalStrehovsky]

Fixes#117309

• Make sure we have type information for the method and parameters show up in the stack
• Format method names like the VS debugger does instead of using symbol name
• Previously we'd skip emitting method debug information if there's no sequence points but I'm not clear on why so deleting that filter too
• this needs to be named this or the debugger will treat it as a regular parameter that is visible in the signature. AFAIK the only reason why we didn't use this originally was CppCodegen.

Before (notice no int n parameter):

After:

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR improves NativeAOT-generated debug information (primarily CodeView/PDB) so debuggers and tooling can show richer method/parameter type details and more user-friendly method names.

Changes:

• Emit a non-zero CodeView procedure type index (methodTypeIndex) and pass it through to the S_GPROC32_ID record.
• Introduce CSharpTypeNameFormatter and use it to provide a C#-style method display name in CodeView symbols instead of the mangled symbol name.
• Rename the implicit instance parameter from "___this" to "this" in ECMA PDB-backed parameter name enumeration.

Reviewed changes

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

Show a summary per file

File	Description
src/coreclr/tools/Common/TypeSystem/IL/EcmaMethodIL.Symbols.cs	Changes implicit instance parameter name emitted by PDB-backed parameter enumeration to "this".
src/coreclr/tools/Common/TypeSystem/Common/Utilities/CSharpTypeNameFormatter.cs	Adds a new formatter for producing C#-style type (and method owning-type) names.
src/coreclr/tools/aot/ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj	Links the new formatter into the ILCompiler.TypeSystem build.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs	Always computes and supplies method type ID and method display name during debug info emission.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs	Updates debug function info signature and sequence-point gating logic for DWARF emission.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs	Passes the new display name through to CodeView symbol emission; preserves line info gating.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CodeView/CodeViewSymbolsBuilder.cs	Writes methodTypeIndex and the new display name into the CodeView S_GPROC32_ID record.

[Copilot]

Pull request overview

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

Suppressed comments (3)

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs:129

• methodDisplayName is constructed for every IMethodNode during debug-info emission, but it’s not used by the DWARF path (e.g., UnixObjectWriter.Aot.cs ignores it) and the Wasm writer also ignores it. This adds per-method StringBuilder/string/UTF8 allocations in ObjectWritingOptions.GenerateDebugInfo builds on non-Windows targets. Consider computing the display name only in the CodeView/Coff path (or lazily in the override when needed) to avoid unnecessary work on other object writers.

 uint methodTypeIndex = _userDefinedTypeDescriptor.GetMethodFunctionIdTypeIndex(methodNode.Method);
Utf8String methodDisplayName = new Utf8String(CSharpTypeNameFormatter.Instance.FormatName(methodNode.Method));
EmitDebugFunctionInfo(methodTypeIndex, methodDisplayName, methodName, methodSymbol, debugNode);
}

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs:276

• debugNode.GetNativeSequencePoints() is enumerated for .Any() here, and then enumerated again later when passing sequence points into EmitLineInfo. If this enumerable is not a cheap materialized collection, this can add avoidable overhead. Consider capturing/materializing the sequence points once and reusing them for both the emptiness check and emission.

 if (debugNode.GetNativeSequencePoints().Any())
{
_dwarfBuilder.EmitLineInfo(
methodSymbol.SectionIndex,
section.SymbolName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs:228

• debugNode.GetNativeSequencePoints() is enumerated for .Any() here, and then enumerated again later when passing sequence points into EmitLineInfo. If this enumerable is not a cheap materialized collection, this can add avoidable overhead. Consider capturing/materializing the sequence points once and reusing them for both the emptiness check and emission.

 if (debugNode.GetNativeSequencePoints().Any())
{
debugSymbolsBuilder.EmitLineInfo(
_debugFileTableBuilder,
methodName,

[MichalStrehovsky]

Previously we'd skip emitting method debug information if there's no sequence points but I'm not clear on why so deleting that filter too

So this was used to paper over issues when no debug information for parameters is available. "No sequence points" was used to approximate "no parameter debug info", basically. DWARF debug info emission wasn't ready to deal with that. We now just emit no parameter debug information. This mostly affects compiler-generated IL.

Alternatively, we could name these parameters, but that sounds a bit problematic.

[Copilot]

Pull request overview

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

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CoffObjectWriter.Aot.cs:228

• GetNativeSequencePoints() is implemented as an iterator on MethodCodeNode and does non-trivial work per enumeration. The current pattern enumerates it twice (once for Any(), again when emitting line info). Materializing the sequence points once avoids doing all that work twice per method.

 if (debugNode.GetNativeSequencePoints().Any())
{
debugSymbolsBuilder.EmitLineInfo(
_debugFileTableBuilder,
methodName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/UnixObjectWriter.Aot.cs:276

• GetNativeSequencePoints() is implemented as an iterator on MethodCodeNode and does non-trivial work per enumeration. The current pattern enumerates it twice (once for Any(), again when emitting line info). Materializing the sequence points once avoids doing all that work twice per method.

 if (debugNode.GetNativeSequencePoints().Any())
{
_dwarfBuilder.EmitLineInfo(
methodSymbol.SectionIndex,
section.SymbolName,

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/ObjectWriter.Aot.cs:128

• methodDisplayName is computed for every method node unconditionally, but the derived emitters for DWARF and Wasm don’t use it. This adds avoidable per-method allocations/work on non-Windows NativeAOT builds. Consider only formatting the C# display name when targeting Windows (CodeView/PDB), and otherwise just pass methodName (or default).

 if (node is IMethodNode methodNode)
{
uint methodTypeIndex = _userDefinedTypeDescriptor.GetMethodFunctionIdTypeIndex(methodNode.Method);
Utf8String methodDisplayName = new Utf8String(CSharpTypeNameFormatter.Instance.FormatName(methodNode.Method));
EmitDebugFunctionInfo(methodTypeIndex, methodDisplayName, methodName, methodSymbol, debugNode);
}

[max-charlamb]

It looks like this changes the scheme for method name symbols. Can we rev the SymbolName contract to n2 so tooling can handle both formats?

runtime/src/coreclr/nativeaot/Runtime/datadescriptor/datadescriptor.inc

Line 259 in 714432e

CDAC_GLOBAL_CONTRACT(SymbolName, n1)

[MichalStrehovsky]

It looks like this changes the scheme for method name symbols. Can we rev the SymbolName contract to n2 so tooling can handle both formats?

It does not change COFF symbol names, the symbol name is still the same. It changes the CodeView S_GPROC32_ID name of the method. This is same as C++ where:

classFoo
{
voidBar(int x);
};
voidFoo::Bar(int x) { }

The COFF symbol name for Bar is ?Bar@Foo@@AEAAXH@Z but S_GPROC32_ID symbol record assigns it name Foo::Bar.

Do we still need to rev SymboName contract?

[max-charlamb]

I see, I misunderstood the change. However, I would still like to bump the version. This should give us flexibility in the future to support previous versions.

/c @noahfalk

[noahfalk]

I see, I misunderstood the change. However, I would still like to bump the version. This should give us flexibility in the future to support previous versions.

I think it would be fine to do and right now the consequences are minimal. One issue that I do think we still need to address is documenting an explicit data contract for this so everyone agrees what behavior is or isn't covered by the contract.

[MichalStrehovsky]

I think it would be fine to do and right now the consequences are minimal. One issue that I do think we still need to address is documenting an explicit data contract for this so everyone agrees what behavior is or isn't covered by the contract.

Does cDAC interpret the symbol names in any way? The symbol names are supposed to be unique and deterministic for the exact same app, but e.g. two different apps might already assign different names to the same thing and there can be various butterfly effects when something in an assembly changes. They are not meant to be stable or reversible like the C++ mangled names. We can't use a reversible/stable C++-like name mangling because .NET naming is ridiculously long (std in C++ vs System.Collections.Generics in .NET).

[Copilot]

Pull request overview

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

[jkotas]

Does cDAC interpret the symbol names in any way?

Yes, for limited set of symbols in CoreLib where the ambiguity should not be a problem. As @noahfalk mentioned this needs to be documented for clarity.

[Copilot]

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/coreclr/nativeaot/Runtime/datadescriptor/datadescriptor.inc:262

• Bumping the NativeAOT cDAC contract version for SymbolName from n1 to n2 has compatibility impact for dump readers, but this PR doesn't appear to include any corresponding contract shape/algorithm changes or managed reader updates. If this wasn't intentional, please revert the version bump; if it was intentional, the contract implementation + docs need to be updated in the same PR so readers can negotiate v2 successfully.

CDAC_GLOBAL_CONTRACT(StressLog, c2)
CDAC_GLOBAL_CONTRACT(Loader, n1)
CDAC_GLOBAL_CONTRACT(ExecutionManager, n1)
CDAC_GLOBAL_CONTRACT(SyncBlock, n1)
CDAC_GLOBAL_CONTRACT(SymbolName, n2)
CDAC_GLOBAL_CONTRACT(CodeVersions, n1)
CDAC_GLOBAL_CONTRACT(ReJIT, n1)
CDAC_GLOBAL_CONTRACT(PrecodeStubs, n1)

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Lite