Call the Copy Constructor for stack arguments in C++/CLI on x86 #100050
Conversation
…maintainable style to get the test passing again
I do not think we want to be enabling any of this on Linux x86. This is Windows-only feature to support IJW. |
If we're looking at backporting this to .NET 8 (which I believe we are given where this issue originated, I really don't want to break back-compat here as we already know that customers do strange things and are prone to using features we never thought they would. I'd rather write the 5 lines of code to get this to compile equally everywhere than remove what currently exists. |
|
[like] Aaron Robinson reacted to your message:
…________________________________
From: Jan Kotas ***@***.***>
Sent: Thursday, March 21, 2024 9:20:20 PM
To: dotnet/runtime ***@***.***>
Cc: Aaron Robinson ***@***.***>; Review requested ***@***.***>
Subject: Re: [dotnet/runtime] Call the Copy Constructor for stack arguments in C++/CLI on x86 (PR #100050)
I'll look at writing the assembly stub for Linux_x86
I do not think we want to be enabling any of this on Linux x86. This is Windows-only feature to support IJW.
—
Reply to this email directly, view it on GitHub<#100050 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AHJXMLJ5EPGNUNRKOP7WMYDYZNFJJAVCNFSM6AAAAABFAQOQROVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJTG43DGMZQHA>.
You are receiving this because your review was requested.Message ID: ***@***.***>
|
|
@jkotas I am good with this change based on Jeremy's response in #100050 (comment). Do you have concerns moving forward, for now, as is or is removing the non-Windows support a hard requirement? |
|
The change includes Linux x86 specific code. What kind of testing have we done on this change on Linux x86? If we have not tested it, we should not be pushing it to servicing and limit it to Windows x86 instead (e.g. find&replace the ifdefs around the changes with I am fine with cleaning up IJW-specific features that leaked to non-Windows later. It may be nice to introduce a FEATURE_ define for it (we used to have one in the old days). |
There is a test case that runs on all platforms. I'm not convinced it should be there, but it does exist and runs/passes. |
The CI does not run any tests on Linux x86. |
Shoot. I saw Linux x86 and assumed it was testing too, the leg is just build. Well this sucks. Let me see what I can do here. |
|
@jkotas PTAL |
|
/backport to release/8.0-staging |
|
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8412095170 |
|
@AaronRobinsonMSFT backporting to release/8.0-staging failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Add repro test case
Applying: Directly load the argument address using ldarga to avoid making a copy
Using index info to reconstruct a base tree...
M src/coreclr/vm/ilmarshalers.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/vm/ilmarshalers.cpp
Applying: Reimplement the "Copy Constructor Cookie" logic in a more modern and maintainable style to get the test passing again
error: sha1 information is lacking or useless (src/tests/Interop/IJW/CopyConstructorMarshaler/CopyConstructorMarshaler.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 Reimplement the "Copy Constructor Cookie" logic in a more modern and maintainable style to get the test passing again
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@AaronRobinsonMSFT an error occurred while backporting to release/8.0-staging, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
…n C++/CLI on x86 (#100221) * Call the Copy Constructor for stack arguments in C++/CLI on Windows-x86 (#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: Jeremy Koritzinsky <jekoritz@microsoft.com>
Fixes #100033