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

[release/5.0] When marshalling a layout class, fall-back to dynamically marshalling the type if it doesn't match the static type in the signature. #50138

Merged

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Mar 23, 2021

Port of #50137, #50655, and #50735 to release/5.0

When we switched the struct marshalling system to be IL based, we accidentally stopped checking if the passed in object matched the type in the signature in the "layout class" case (where we have a sequential or explicit layout class type). This causes failures to marshal the derived types correctly since only the base type ends up being marshalled.

Customer Impact

Without this change, developers need to manually marshal their types. An internal team (Halo) has worked around this by manually marshalling their types, but would prefer to remove this workaround.

Regression?

This was a regression from .NET Core 3.1.

See #49857

Testing

Tests for both the fix and backwards compatibility are included with this change.

Risk

Medium. This change affects marshalling of layout classes.

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Approved. I will take for consideration in 5.0.x.

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Mar 25, 2021
@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Mar 25, 2021
@leecow leecow modified the milestones: 5.0.x, 5.0.6 Mar 25, 2021
@jkotas
Copy link
Member

jkotas commented Apr 2, 2021

No merge until #50410 is fixed

@jkotas jkotas added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 2, 2021
@elinor-fung elinor-fung removed the Servicing-approved Approved for servicing release label Apr 3, 2021
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Approved. Please complete cr.

@jeffschwMSFT
Copy link
Member

@leecow we had approved this issue. Then in the time since a customer came back with an issue in master that applies to this change. We have updated the PR (the original intent of the change remains the same). I suggest that we consider this issue approved based on our original discussion. Though happy to bring it back if further discussion is warranted.

@jeffschwMSFT
Copy link
Member

@jkoritzinsky is this PR now in a mergeable state?

@jkoritzinsky
Copy link
Member Author

Yes, this PR is in a mergable state. Just waiting on an updated code review @AaronRobinsonMSFT

@jeffschwMSFT
Copy link
Member

@Anipik once we have @AaronRobinsonMSFT cr, this pr will be ready for merge. We found an issue and have sense included an updated commit to address it.

@jeffschwMSFT jeffschwMSFT removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 6, 2021
@jeffschwMSFT jeffschwMSFT added the Servicing-approved Approved for servicing release label Apr 6, 2021
@jeffschwMSFT
Copy link
Member

@Anipik this pr is ready to merge. Thanks

@Anipik
Copy link
Contributor

Anipik commented Apr 6, 2021

@Anipik this pr is ready to merge. Thanks

i will merge the pr after the ci is complete.

@Anipik Anipik merged commit 718c0ed into dotnet:release/5.0 Apr 6, 2021
jkoritzinsky added a commit that referenced this pull request Apr 7, 2021
…ynamically marshalling the type if it doesn't match the static type in the signature. (#50138)"

This reverts commit 718c0ed.
Anipik pushed a commit that referenced this pull request Apr 9, 2021
…ynamically marshalling the type if it doesn't match the static type in the signature. (#50138)" (#50883)

This reverts commit 718c0ed.
jkoritzinsky added a commit that referenced this pull request Apr 9, 2021
…ack to dynamically marshalling the type if it doesn't match the static type in the signature. (#50138)" (#50883)"

This reverts commit b71bb59.
jkoritzinsky added a commit that referenced this pull request Apr 9, 2021
…ynamically marshalling the type if it doesn't match the static type in the signature. (#50138)" (#50883) (#51021)

This reverts commit b71bb59.
Anipik pushed a commit that referenced this pull request May 5, 2021
* [release/5.0] Use adjusted parent layout size to handle a non-zero-sized class inheriting from a zero-sized class that inherits from another zero-sized class.

Fixes handling of zero-sized parent types that have zero-sized parent types.

* Revert "[release/5.0] When marshalling a layout class, fall-back to dynamically marshalling the type if it doesn't match the static type in the signature. (#50138)" (#50883) (#51021)

This reverts commit b71bb59.
@ghost ghost locked as resolved and limited conversation to collaborators May 6, 2021
@jkoritzinsky jkoritzinsky deleted the derived-layouttype-marshalling-5.0 branch September 28, 2021 00:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Interop-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants