Use enum for frametype not v table by davidwrighton · Pull Request #112166 · dotnet/runtime

davidwrighton · 2025-02-05T00:38:49Z

Frames are currently identified by VTable, which is convenient for writing C++ code, but less so for writing asm, or out of process inspection. This change changes that to a FrameIdentifier enum, make the polymorphic dispatch occur using if statements, and removes GS cookies for Frame's as we no longer need to protect the vtable.

…ize, and make it reviewable

…s easier to search for

…ForFrametypeNotVTable

dotnet-policy-service · 2025-02-05T00:39:30Z

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

…ForFrametypeNotVTable

jkotas

LGTM otherwise. Thank you!

jkotas · 2025-02-08T19:07:21Z

-// This hashtable contains the vtable value of every Frame type.
-static PtrHashMap* s_pFrameVTables = NULL;
-
 // static


Suggested change
// static

jkotas · 2025-02-08T19:14:27Z

 // Implementation of the global table of names. On the DAC side, just the global pointer.
 // On the runtime side, the array of names.
- #define FRAME_TYPE_NAME(x) {x::GetMethodFrameVPtr(), #x} ,
+ #define FRAME_TYPE_NAME(x) {FrameIdentifier::x, #x} ,


FrameIdentifier does not need to be in the table anymore. We can just index into the table.

jkotas · 2025-02-08T19:15:50Z

@@ -198,19 +445,19 @@ const size_t FRAME_TYPES_COUNT =
 //-----------------------------------------------------------------------


FRAME_TYPES_COUNT above can be deleted

jkotas · 2025-02-08T19:17:05Z


 // static
-bool Frame::HasValidVTablePtr(Frame * pFrame)
+bool Frame::HasFrameIdentifier(Frame * pFrame)


Suggested change
boolFrame::HasFrameIdentifier(Frame * pFrame)
boolFrame::HasValidFrameIdentifier(Frame * pFrame)
?

jkotas · 2025-02-08T19:36:42Z


 #endif // DACCESS_COMPILE

 // Returns true if the Frame's VTablePtr is valid


Suggested change
// Returns true if the Frame's VTablePtr is valid
// Returns true if the Frame has a valid FrameIdentifier

jkotas · 2025-02-08T19:38:42Z

+
+void Frame::GcScanRoots(promote_func *fn, ScanContext* sc)
+{
+ switch (this->GetFrameIdentifier())


Suggested change
switch (this->GetFrameIdentifier())
switch (GetFrameIdentifier())
Nit: this-> should not be needed

jkotas · 2025-02-08T19:39:32Z

+ FRAME_POLYMORPHIC_DISPATCH_UNREACHABLE();
+ return NULL;
+ }
+}


Suggested change
}
}

jkotas · 2025-02-08T19:39:56Z

+
+#endif


Suggested change
#endif
#endif

jkotas · 2025-02-08T19:40:36Z

+ }
+}
+
+


Suggested change

max-charlamb · 2025-02-10T18:17:05Z

Resolves #111875

davidwrighton · 2025-02-11T18:04:55Z

This change refactors the frame identification mechanism in the runtime by replacing the use of the VTable with an enum FrameIdentifier. The VTable-based approach more suited to C++ code, but presented challenges for assembly code and out-of-process inspections. By using FrameIdentifier, polymorphic dispatch is now handled via if-statements. This change also eliminates the need for GS cookies on Frame objects, as it is no longer necessary to protect the VTable.

Key changes include:

Renaming FrameType to FrameIdentifier
Introducing a GetFrameIdentifier method
This update streamlines frame management while improving performance and flexibility for various platforms.

* main: [Android] Run CoreCLR functional tests on Android (dotnet#112283) [LoongArch64] Fix some assertion failures for Debug ILC building Debug NativeAOT testcases. (dotnet#112229) Fix suspicious code fragments (dotnet#112384) `__ComObject` doesn't support dynamic interface map (dotnet#112375) Native DLLs: only load imported DLLs from System32 (dotnet#112359) [main] Update dependencies from dotnet/roslyn (dotnet#112314) Update SVE instructions that writes to GC regs (dotnet#112389) Bring up android+coreclr windows build. (dotnet#112256) Never use heap for return buffers (dotnet#112060) Wait to complete the test before releasing the agile reference. (dotnet#112387) Prevent returning disposed HTTP/1.1 connections to the pool (dotnet#112383) Fingerprint dotnet.js if writing import map to html is enabled (dotnet#112407) Remove duplicate definition of CORECLR_HOSTING_API_LINKAGE (dotnet#112096) Update the exception message to reflect current behavior. (dotnet#112355) Use enum for frametype not v table (dotnet#112166) Enable AltJits build for LoongArch64 and RiscV64 (dotnet#110282) Guard members of MonoType union & fix related bugs (dotnet#111645) Add optional hooks for debugging OpenSSL memory allocations (dotnet#111539) JIT: Optimize struct parameter register accesses in the backend (dotnet#110819) NativeAOT: Cover more opcodes in type preinitializer (dotnet#112073)

davidwrighton added 7 commits February 4, 2025 14:35

Initial pass where names all changed
4c37642

Convert back to using member functions on Frame to reduce the delta s…
53cf3f4
…ize, and make it reviewable

Handle more multi-arch scenarios
7fb5c3a

Rename FrameType to FrameIdentifier
8fedbb6

Rename the GetType api I added on Frame to GetFrameIdentifier which i…
589263c
…s easier to search for

Merge branch 'main' ofhttps://github.com/dotnet/runtimeinto UseEnum…
93f81b4
…ForFrametypeNotVTable

Fix issues on more architectures
496472e

ghost added the area-VM-coreclr label Feb 5, 2025

dotnet-policy-serviceBot assigned davidwrighton Feb 5, 2025

jkotas reviewed Feb 5, 2025
View reviewed changes

Comment threadsrc/coreclr/vm/frames.h Outdated

davidwrighton added 8 commits February 5, 2025 11:32

Fix issues found in testing
115d235

Remove GS Cookies
e3cfc5a

Get rid of FrameBase
967934c

Merge branch 'main' ofhttps://github.com/dotnet/runtimeinto UseEnum…
b34284f
…ForFrametypeNotVTable

Fix build errors
dc2464d

Fix windows arm64 build failure
7fe3e81

Fix issues noted in X86 testing
ba80e02

Merge branch 'main' ofhttps://github.com/dotnet/runtimeinto UseEnum…
e20b775
…ForFrametypeNotVTable

davidwrighton changed the title ~~[DRAFT] Use enum for frametype not v table~~Use enum for frametype not v tableFeb 6, 2025

davidwrighton marked this pull request as ready for review February 6, 2025 23:01

davidwrighton requested review from jkotas and max-charlamb February 6, 2025 23:01

jkotas reviewed Feb 7, 2025
View reviewed changes

Most of Jan's feedback
8577534

max-charlamb reviewed Feb 7, 2025
View reviewed changes

Comment threadsrc/coreclr/vm/amd64/redirectedhandledjitcase.S Outdated
Comment threadsrc/coreclr/vm/amd64/ComCallPreStub.asm Outdated
Comment threadsrc/coreclr/vm/amd64/GenericComCallStubs.asm Outdated
Comment threadsrc/coreclr/vm/arm64/asmhelpers.S
Comment threadsrc/coreclr/vm/riscv64/asmhelpers.S

davidwrighton added 2 commits February 7, 2025 12:30

Convert to switch statements
a7d8057

Use UNREACHABLE to potentially make stuff faster
c01630f

davidwrighton requested a review from MichalStrehovsky as a code owner February 7, 2025 20:43

jkotas reviewed Feb 7, 2025
View reviewed changes

Comment threadsrc/coreclr/vm/frames.cpp Outdated

Address more feedback
0cbc419

jkotas approved these changes Feb 8, 2025
View reviewed changes

jkotas reviewed Feb 8, 2025
View reviewed changes

Comment threadsrc/coreclr/vm/frames.cpp
FRAME_POLYMORPHIC_DISPATCH_UNREACHABLE();
return NULL;
}
}
jkotasFeb 8, 2025
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change
}
}

jkotas reviewed Feb 8, 2025
View reviewed changes

Comment threadsrc/coreclr/vm/frames.cpp Outdated
Comment on lines +83 to +84

#endif
jkotasFeb 8, 2025
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change
#endif
#endif

jkotas reviewed Feb 8, 2025
View reviewed changes

Comment threadsrc/coreclr/vm/frames.cpp Outdated
}
}

jkotasFeb 8, 2025
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change

max-charlamb mentioned this pull request Feb 7, 2025
[cDAC] Stack Walking #110758
Closed
24 tasks

Hopefully final nits
cc5d9ed

This was referenced Feb 11, 2025
slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883
Open
The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008
Open

davidwrighton merged commit 44b7251 into dotnet:mainFeb 11, 2025

max-charlamb mentioned this pull request Feb 11, 2025
Covert Frames to use an Enum intead of vptr #111875
Closed

max-charlamb mentioned this pull request Feb 12, 2025
[DAC] ClrDataAccess::GetFrameName support Frames using identifier instead of vptr #112488
Merged

LuckyXu-HF mentioned this pull request Feb 19, 2025
[LoongArch64] Fix a typo within PR#112166. #112672
Merged

github-actionsBot locked and limited conversation to collaborators Mar 14, 2025

		@@ -198,19 +445,19 @@ const size_t FRAME_TYPES_COUNT =
		//-----------------------------------------------------------------------

	boolFrame::HasFrameIdentifier(Frame * pFrame)
	boolFrame::HasValidFrameIdentifier(Frame * pFrame)


		#endif // DACCESS_COMPILE

		// Returns true if the Frame's VTablePtr is valid

	// Returns true if the Frame's VTablePtr is valid
	// Returns true if the Frame has a valid FrameIdentifier

	switch (this->GetFrameIdentifier())
	switch (GetFrameIdentifier())

Uh oh!

Conversation

davidwrighton commented Feb 5, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-serviceBot commented Feb 5, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-charlamb commented Feb 10, 2025

Uh oh!

davidwrighton commented Feb 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davidwrighton commented Feb 5, 2025•
edited
Loading