Skip to content

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

Merged
stephentoub merged 2 commits into
dotnet:mainfrom
stephentoub:usecreatespan
Jan 13, 2023
Merged

Use Roslyn support for RuntimeHelpers.CreateSpan (or field caching downlevel)#79461
stephentoub merged 2 commits into
dotnet:mainfrom
stephentoub:usecreatespan

Conversation

@stephentoub

Copy link
Copy Markdown
Member

No description provided.

@ghostghost added the area-Meta label Dec 9, 2022
@ghostghost assigned stephentoubDec 9, 2022
@ghost

ghost commented Dec 9, 2022

Copy link
Copy Markdown

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

Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.Date.cs Outdated
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.Date.cs Outdated
@stephentoubstephentoub added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 10, 2022
@stephentoub

Copy link
Copy Markdown
MemberAuthor

Some compiler fixes in dotnet/roslyn#65919.

@jcouv

Copy link
Copy Markdown
Contributor

FYI, resolved the conflict

@stephentoubstephentoub removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 3, 2023
Comment threadsrc/libraries/Common/src/System/Security/IdentityHelper.cs Outdated
@EgorBo

Copy link
Copy Markdown
Member

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

@stephentoub

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

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
Copy Markdown
Contributor

@stephentoub#79449 is the last blocker

@stephentoub

Copy link
Copy Markdown
MemberAuthor

#79449 is the last blocker

Excellent, thanks.

@stephentoub

Copy link
Copy Markdown
MemberAuthor

This is ready for review.

@EgorBoEgorBo left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

@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
Copy Markdown
Member

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

#79449 hasn't merged yet.

@stephentoub

Copy link
Copy Markdown
MemberAuthor

#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 freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@stephentoub@jcouv@EgorBo@marek-safar@MichalStrehovsky@teo-tsirpanis