Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cdac][.NET9] Data descriptor changes for GetMethodDescData #106417

Closed
wants to merge 1 commit into from

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented Aug 14, 2024

Extracted from #106413

The main new thing here is a PrecodeMachineDescriptor struct and global: this is used to copy out the enum values and masks that show how to manipulate precode stubs to figure out what kind of a stub we're looking at.

Contributes to #99302

Copy link
Contributor

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

@carlossanlop
Copy link
Member

carlossanlop commented Aug 14, 2024

@jkotas @AaronRobinsonMSFT @davidwrighton @elinor-fung please help reviewing this PR. @lambdageek has requested that I wait for it before the RC1 snap if possible.

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to update any markdown?

@@ -332,7 +332,7 @@ class LoaderAllocator
bool m_fTerminated;
bool m_fMarked;
int m_nGCCount;
bool m_IsCollectible;
BYTE m_IsCollectible;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
BYTE m_IsCollectible;
const BYTE m_IsCollectible;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it turns out we can't make m_IsCollectible const. The reason is because LoaderAllocator is a DAC-ized base class and the VPTR_BASE_VTABLE_CLASS macro adds a LoaderAllocator(TADDR addr, TADDR vtAddr) {} constructor which doesn't include an initializer for m_IsCollectible which results in a compilation error:

 E:\dotnet-runtime\runtime-m\src\coreclr\vm\loaderallocator.hpp(291): error C2789: 'LoaderAllocator::m_IsCollectible'
  : an object of const-qualified type must be initialized
  E:\dotnet-runtime\runtime-m\src\coreclr\vm\loaderallocator.hpp(335): note: see declaration of 'LoaderAllocator::m_Is
  Collectible'

@elinor-fung
Copy link
Member

Do we need to update any markdown?

Not for this portion - this is the data descriptors (and the new struct/global in runtime to better represent needed data), not tied to contracts yet.

@@ -662,4 +662,55 @@ BOOL DoesSlotCallPrestub(PCODE pCode)
return FALSE;
}

PrecodeMachineDescriptor g_PrecodeMachineDescriptor;

void PrecodeMachineDescriptor::Init()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there an Init routine as well as a set of non-static data member initializers here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to delete the initializers, that was an idea that didn't work out

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you doing this before you want this checked in? Given the snap deadline, I think we should probably miss it, and do a backport of the data descriptors when we're ready in reality to declare that .NET 9 might actually be useable with the cDAC and CLRMA. In general, I'm leery of stuff that has a static initializer, and that's what we have here now, and I don't see us fixing this before the snap happens.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, let's miss the snap. I can't fix and validate this in the next couple hours

@lambdageek
Copy link
Member Author

@carlossanlop I'm going to close this and do a back port after the snap, it needs more work

@lambdageek lambdageek closed this Aug 14, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants