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

Use Roslyn support for RuntimeHelpers.CreateSpan (or field caching downlevel) #79461

Merged
merged 2 commits into from
Jan 13, 2023

Conversation

stephentoub
Copy link
Member

No description provided.

@ghost
Copy link

ghost commented Dec 9, 2022

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

Issue Details

null

Author: stephentoub
Assignees: stephentoub
Labels:

area-Meta

Milestone: -

@stephentoub stephentoub added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 10, 2022
@stephentoub
Copy link
Member Author

Some compiler fixes in dotnet/roslyn#65919.

@jcouv
Copy link
Member

jcouv commented Dec 16, 2022

FYI, resolved the conflict

@stephentoub stephentoub removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 3, 2023
@EgorBo
Copy link
Member

EgorBo commented Jan 10, 2023

Just curious - is it blocked by some other roslyn/illink bug currently? Can't wait to see it merged 🙂

@stephentoub
Copy link
Member Author

is it blocked by some other roslyn/illink bug currently?

Yes, it's blocked by a cecil / illink bug that's resulting in some fields being misaligned.

@stephentoub
Copy link
Member Author

is it blocked by some other roslyn/illink bug currently?

Yes, it's blocked by a cecil / illink bug that's resulting in some fields being misaligned.

@sbomer, any updated ETA on when you think a fix might land? Thanks!

@marek-safar
Copy link
Contributor

@stephentoub #79449 is the last blocker

@stephentoub
Copy link
Member Author

#79449 is the last blocker

Excellent, thanks.

@stephentoub
Copy link
Member Author

This is ready for review.

Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

It seems that DaysToMonth365 and DaysToMonth366 are duplicated in several files but I assume it's not a big deal

@stephentoub
Copy link
Member Author

It seems that DaysToMonth365 and DaysToMonth366 are duplicated in several files but I assume it's not a big deal

This PR isn't adding any duplication. It removes some of the duplication already there, and then what's left remains duplicated because they're either defined in different assemblies or using different types. We could obviously reduce that source duplication by putting them in a shared file, but I chose not to do so as part of this PR.

@stephentoub
Copy link
Member Author

@sbomer, this is still hitting an alignment issue, on both x86 and arm64 legs 😦

      System.ArgumentException : Value does not fall within the expected range.
      Stack Trace:
           at System.Runtime.CompilerServices.RuntimeHelpers.GetSpanDataFrom(RuntimeFieldHandle fldHandle, RuntimeTypeHandle targetTypeHandle, Int32& count)
        /_/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs(100,0): at System.Runtime.CompilerServices.RuntimeHelpers.CreateSpan[T](RuntimeFieldHandle fldHandle)
        /_/src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs(140,0): at System.Decimal.DecCalc.get_DoublePowers10()
        /_/src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs(1732,0): at System.Decimal.DecCalc.VarDecFromR8(Double input, DecCalc& result)
        /_/src/libraries/System.Private.CoreLib/src/System/Decimal.cs(195,0): at System.Decimal..ctor(Double value)
        /_/src/libraries/System.Private.CoreLib/src/System/Decimal.cs(935,0): at System.Decimal.op_Explicit(Double value)
    Microsoft.Extensions.Configuration.FileExtensions.Test.FileConfigurationBuilderExtensionsTest.GetFileProvider_ReturnPhysica

Is this somehow not picking up your fixes? Are there more issues lurking?

@MichalStrehovsky
Copy link
Member

Is this somehow not picking up your fixes? Are there more issues lurking?

#79449 hasn't merged yet.

@stephentoub
Copy link
Member Author

#79449 hasn't merged yet

Thanks. 🤦‍♂️ Somehow I saw the link as the larger dependency update that did merge; too many tabs open I guess. Will try again now that it has merged.

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.

6 participants