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

MethodTable::IsClassInited returns false for <PrivateImplementationDetails> class #104247

Closed
EgorBo opened this issue Jul 1, 2024 · 7 comments · Fixed by #104253
Closed

MethodTable::IsClassInited returns false for <PrivateImplementationDetails> class #104247

EgorBo opened this issue Jul 1, 2024 · 7 comments · Fixed by #104253
Assignees

Comments

@EgorBo
Copy link
Member

EgorBo commented Jul 1, 2024

See #104054 for discussion.

public class Program
{
    static void Main()
    {
        Test();
    }

    static ReadOnlySpan<int> RvaData => [1, 2, 3, 4];

    [MethodImpl(MethodImplOptions.NoInlining)]
    static int Test() => RvaData[2];
}

This code (TieredCompilation=0) used to fold RvaData[2] into 3, it's no longer happening because getStaticFieldContent gives up on <PrivateImplementationDetails> being non-initialized statically (despite the fact it has no cctor)

Previously, it used to return true and everything was correctly folded, it's no longer the case after #99183

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 1, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jul 1, 2024
@EgorBo EgorBo added area-VM-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 1, 2024
Copy link
Contributor

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

@jkotas
Copy link
Member

jkotas commented Jul 1, 2024

I think the problem is more in the JIT/EE interface.

Before the change, CORINFO_FLG_FIELD_INITCLASS means initialization is required.

After the change, CORINFO_FLG_FIELD_INITCLASS means initialization may be required, call initClass to be sure.

We either need to fix JIT/EE interface to have the old behavior; or we need fix the JIT to account for the new behavior.

Alternatively, we can get rid of CORINFO_FLG_FIELD_INITCLASS and require JIT to always call initClass to figure out whether the initialization is required.

@EgorBo
Copy link
Member Author

EgorBo commented Jul 1, 2024

and require JIT to always call initClass to figure out whether the initialization is required.

so who (and when) is going to call initClass in case of CreateSpan(ldtoken) ?

@jkotas
Copy link
Member

jkotas commented Jul 1, 2024

Why does initClass need to be called for CreateSpan(ldtoken) in the first place? The unexpanded implementation of CreateSpan(ldtoken) does not trigger static constructors, so I do not see why we need to call initClass in the intrinsic expansion of CreateSpan(ldtoken).

@EgorBo
Copy link
Member Author

EgorBo commented Jul 1, 2024

Why does initClass need to be called for CreateSpan(ldtoken) in the first place? The unexpanded implementation of CreateSpan(ldtoken) does not trigger static constructors, so I do not see why we need to call initClass in the intrinsic expansion of CreateSpan(ldtoken).

um.. so it means nobody calls initClass for <PrivateImplementationDetails> (since the only way it's referenced is via CreateSpan(ldtoken)) and then getStaticFieldContent always refuses to fold anything since IsClassInited will return false. I feel like I am missing something 😐 how is JIT-EE fixup going to help here?

@jkotas
Copy link
Member

jkotas commented Jul 1, 2024

getStaticFieldContent always refuses to fold anything since IsClassInited will return false.

getStaticFieldContent may want to call AttemptToPreinit() before calling IsClassInitialized.

@davidwrighton
Copy link
Member

I think we would prefer to make a new function... I'm calling it IsClassInitedOrPreinited which will will do the IsClassInited check + the AttemptToPreinit + a final IsClassInited. It looks like there may be several places in the JIT interface which should be calling it and just using the more complete function is a more reliable fix. I'm taking a look at making that change, which seems fairly simple.

davidwrighton added a commit to davidwrighton/runtime that referenced this issue Jul 1, 2024
davidwrighton added a commit that referenced this issue Jul 3, 2024
…04253)

* Fix #104247 by providing a new helper function which means what the old IsClassInited helper meant
- AND a very subtle race condition where we need to set the IsClassInited flag bit on dynamic statics BEFORE setting the MethodTable level one
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Jul 3, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants