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

[NativeAOT] Emit Align8 flag for thread statics #105931

Merged
merged 9 commits into from
Aug 9, 2024

Conversation

filipnavara
Copy link
Member

Fixes #103234
Contributes to #97729

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 5, 2024
@filipnavara filipnavara marked this pull request as ready for review August 7, 2024 07:29
Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Could you also make it so that we don't have GCStaticEETypes that only differ by RequiresAlign8 bit on platforms that don't have Align8 concept? (we don't set RequiresAlign8 on MethodTables on these platforms either).

@filipnavara
Copy link
Member Author

filipnavara commented Aug 7, 2024

Could you also make it so that we don't have GCStaticEETypes that only differ by RequiresAlign8 bit on platforms that don't have Align8 concept? (we don't set RequiresAlign8 on MethodTables on these platforms either).

The bool RequiresAlign8(this TypeDesc type) always returns false on platforms without the concept, ie. anything but ARM32 and WASM. The deduplication should happen in NodeCache automatically since you get the same key with requiresAlign8 == false.

Nevermind, I forgot that I didn't use RequiresAlign8(...) in the final version. Will fix.

filipnavara and others added 3 commits August 7, 2024 09:44
Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>
Copy link
Contributor

@SingleAccretion SingleAccretion left a comment

Choose a reason for hiding this comment

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

Do dynamic (MakeGenericType) thread statics need any additional attention (e. g. a test)?

@filipnavara
Copy link
Member Author

Do dynamic (MakeGenericType) thread statics need any additional attention (e. g. a test)?

Test would be nice. I am not really sure how these are treated internally in ILC / NAOT runtime, so I am not sure if I am qualified to check this.

@MichalStrehovsky
Copy link
Member

Do dynamic (MakeGenericType) thread statics need any additional attention (e. g. a test)?

Test would be nice. I am not really sure how these are treated internally in ILC / NAOT runtime, so I am not sure if I am qualified to check this.

Something along these lines:

    private class ThreadStaticAlignCheck1<T>
    {
        [ThreadStatic]
        [FixedAddressValueType]
        internal static ReturnArea returnArea = default;
    }

    private class ThreadStaticAlignCheck2<T>
    {
        [ThreadStatic]
        [FixedAddressValueType]
        internal static ReturnArea returnArea = default;
    }

    public static void RunGeneric<T>()
    {
            // Assume that these are allocated sequentially, use a padding object of size 12 (mod 8 is not 0)
            // to move the alignment of the second AddressOfReturnArea in case the first is coincidentally aligned 8.
            var ts1Addr = ThreadStaticAlignCheck1<T>.returnArea.AddressOfReturnArea();
            var p = new Padder();
            var ts2Addr = ThreadStaticAlignCheck2<T>.returnArea.AddressOfReturnArea();

            if (((nint)ts1Addr) % 8 != 0)
                throw new Exception();
            if (((nint)ts2Addr) % 8 != 0)
                throw new Exception();
    }

    class Atom { }

And add this to Run:

typeof(ThreadStaticAlignmentTest).GetMethod("RunGeneric").MakeGenericMethod(GetAtom()).Invoke(null, []);

[MethodImpl(MethodImplOptions.NoInlining)]
static void GetAtom() => typeof(Atom);

@filipnavara
Copy link
Member Author

Something along these lines:

Thanks, I will check it on my Raspberry Pi once I get to office.

@filipnavara
Copy link
Member Author

Test added, it passes on my machine (tm).

@MichalStrehovsky
Copy link
Member

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Thank you!

@MichalStrehovsky MichalStrehovsky merged commit a524db6 into dotnet:main Aug 9, 2024
111 of 115 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NativeAOT/linux-arm32] long and double thread statics are not 8-byte aligned
3 participants