Use enum for frametype not v table #112166

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.

src/coreclr/vm/frames.h

…ForFrametypeNotVTable

src/coreclr/vm/FrameTypes.h

src/coreclr/vm/frames.cpp

src/coreclr/vm/frames.h

src/coreclr/vm/arm/ehhelpers.S

src/coreclr/vm/comtoclrcall.cpp

src/coreclr/vm/frames.h

src/coreclr/vm/frames.cpp

src/coreclr/vm/amd64/redirectedhandledjitcase.S

src/coreclr/vm/amd64/ComCallPreStub.asm

src/coreclr/vm/amd64/GenericComCallStubs.asm

src/coreclr/vm/arm64/asmhelpers.S

src/coreclr/vm/riscv64/asmhelpers.S

src/coreclr/vm/frames.cpp

jkotas

LGTM otherwise. Thank you!

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

src/coreclr/vm/frames.cpp

@@ -269,77 +512,30 @@ void Frame::LogFrameChain(

 #ifndef DACCESS_COMPILE

-// 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

src/coreclr/vm/frames.cpp

@@ -198,19 +445,19 @@ const size_t FRAME_TYPES_COUNT =
 //-----------------------------------------------------------------------
 // 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

src/coreclr/vm/frames.cpp

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


FRAME_TYPES_COUNT above can be deleted

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

src/coreclr/vm/frames.cpp

 } // void Frame::Init()

 #endif // DACCESS_COMPILE

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

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


Suggested change

bool Frame::HasFrameIdentifier(Frame * pFrame)

bool Frame::HasValidFrameIdentifier(Frame * pFrame)

?

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

src/coreclr/vm/frames.cpp

-#include "frames.h"
-
+    LIMITED_METHOD_CONTRACT;
+    _frameIdentifier = frameIdentifier;
 } // void Frame::Init()

 #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

src/coreclr/vm/frames.cpp

+
+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

src/coreclr/vm/frames.cpp

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


Suggested change

}

}

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

src/coreclr/vm/frames.cpp

+
+#endif


Suggested change

#endif

#endif

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

src/coreclr/vm/frames.cpp

+    }
+}
+
+


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' of https://github.com/dotnet/runtime into UseEnum…

93f81b4

…ForFrametypeNotVTable

Fix issues on more architectures

496472e

dotnet-issue-labeler bot added the area-VM-coreclr label Feb 5, 2025

dotnet-policy-service bot assigned davidwrighton Feb 5, 2025

jkotas reviewed Feb 5, 2025

View reviewed changes

src/coreclr/vm/frames.h Outdated Show resolved Hide resolved

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' of https://github.com/dotnet/runtime into UseEnum…

b34284f

…ForFrametypeNotVTable

Fix build errors

dc2464d

Fix windows arm64 build failure

7fe3e81

Fix issues noted in X86 testing

ba80e02

Merge branch 'main' of https://github.com/dotnet/runtime into UseEnum…

e20b775

…ForFrametypeNotVTable

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

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

davidwrighton requested review from max-charlamb and jkotas 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

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

src/coreclr/vm/frames.cpp Outdated Show resolved Hide resolved

Address more feedback

0cbc419

jkotas approved these changes Feb 8, 2025

View reviewed changes

jkotas reviewed Feb 8, 2025

View reviewed changes

src/coreclr/vm/frames.cpp

FRAME_POLYMORPHIC_DISPATCH_UNREACHABLE();

return NULL;

}

}

Copy link

Member

jkotas Feb 8, 2025

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

src/coreclr/vm/frames.cpp Outdated

Comment on lines 83 to 84

#endif

Copy link

Member

jkotas Feb 8, 2025

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

src/coreclr/vm/frames.cpp Outdated

}

}

Copy link

Member

jkotas Feb 8, 2025

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] Contract implementations needed for managed stack walking #110758

Open

38 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:main Feb 11, 2025
93 of 97 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use enum for frametype not v table #112166

Use enum for frametype not v table #112166

davidwrighton commented Feb 5, 2025 •

edited

Loading

dotnet-policy-service bot commented Feb 5, 2025

jkotas left a comment

jkotas Feb 8, 2025

jkotas Feb 8, 2025

jkotas Feb 8, 2025

jkotas Feb 8, 2025

jkotas Feb 8, 2025

jkotas Feb 8, 2025

jkotas Feb 8, 2025

jkotas Feb 8, 2025

jkotas Feb 8, 2025

max-charlamb commented Feb 10, 2025

davidwrighton commented Feb 11, 2025

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

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

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

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

Use enum for frametype not v table #112166

Use enum for frametype not v table #112166

Conversation

davidwrighton commented Feb 5, 2025 • edited Loading

dotnet-policy-service bot commented Feb 5, 2025

jkotas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-charlamb commented Feb 10, 2025

davidwrighton commented Feb 11, 2025

davidwrighton commented Feb 5, 2025 •

edited

Loading