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/8.0-staging] Call the Copy Constructor for stack arguments in C++/CLI on x86 #100221

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Mar 25, 2024

This is a port of #100050 to .NET 8.

/cc @jeffschwMSFT @agocke @jkotas

Customer Impact

  • Customer reported
  • Found internally

See user reported issue #100033.

The DTS bug contains the repro. Essentially, a copy ctor of std::vector<>::iterator is not called when pushing the argument onto the stack for the call to the native method, which takes an argument of type iterator. This leads to STL's iterator debugging book-keeping code to later assert.

This happens only for x86 and does not repro in the desktop framework where the copy ctor is correctly called.

The workaround is to disable debug iterators or change the legacy code to avoid the problematic pattern. Both of these are unacceptable long-term in-production fixes.

Regression

  • Yes
  • No

Regression introduced from .NET Framework. Support was added in .NET Core 3.1 and included the regression - dotnet/coreclr#22805.

Testing

A test was added that validates the issue in a C++/CLI scenario.

Risk

Low

…86 (dotnet#100050)

* Add repro test case

* Directly load the argument address using ldarga to avoid making a copy

* Reimplement the "Copy Constructor Cookie" logic in a more modern and maintainable style to get the test passing again

* Narrow support to Windows only

---------

Co-authored-by: Aaron R Robinson <arobins@microsoft.com>
@AaronRobinsonMSFT AaronRobinsonMSFT added Servicing-consider Issue for next servicing release review area-Interop-coreclr labels Mar 25, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 8.0.x milestone Mar 25, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT marked this pull request as ready for review March 25, 2024 02:59
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

I would avoid the additional cleanup in the servicing backport.

src/coreclr/vm/ilmarshalers.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/ilmarshalers.cpp Show resolved Hide resolved
src/coreclr/vm/ilmarshalers.cpp Show resolved Hide resolved
src/coreclr/vm/ilmarshalers.cpp Show resolved Hide resolved
src/coreclr/vm/ilmarshalers.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/mlinfo.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/mlinfo.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/mlinfo.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/mtypes.h Outdated Show resolved Hide resolved
src/coreclr/vm/mtypes.h Outdated Show resolved Hide resolved
@AaronRobinsonMSFT
Copy link
Member Author

I would avoid the additional cleanup in the servicing backport.

Sounds good. I wasn't sure how much effort we wanted to narrow the changes. I just tried applying them without a local build. If there is any fallout, I will look again tomorrow.

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. we will take for consideration in 8.0.x

@rbhanda rbhanda modified the milestones: 8.0.x, 8.0.5 Mar 26, 2024
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Mar 26, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT merged commit ba9df1e into dotnet:release/8.0-staging Mar 30, 2024
115 of 122 checks passed
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the port_100050_net8 branch March 30, 2024 15:20
@github-actions github-actions bot locked and limited conversation to collaborators Apr 30, 2024
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.

6 participants