[Mono] Fix offset calculation for nested struct, when pinvoke is enabled #91292
Conversation
fanyang-mono
requested review from
SamMonoRT,
lambdageek and
vargaz
as code owners
ghost
assigned
|
This should be backported to .NET8/7/6. |
lambdageek
left a comment
lambdageek
left a comment
There was a problem hiding this comment.
The test can be simplified - you don't need any methods in the struct definitions - just fields.
The test can also go into src/tests/Interop/StructMarshalling/PInvoke/ and can be updated like the other .csproj files in that directory to use the .cpp file to implement a C++ function that takes the right kind of argument struct.
|
Newly added test failed on wasm with interpreter. Investigating it now. |
|
The wasm apps created for runtime tests weren't including c++ file/library needed by the test. See details in #64127. Disabled newly added test on wasm. |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Library test failures were related to #91410. |
|
/backport to release/8.0 |
|
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6041888778 |
|
/backport to release/7.0-staging |
|
/backport to release/6.0-staging |
|
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6041900557 |
|
Started backporting to release/6.0-staging: https://github.com/dotnet/runtime/actions/runs/6041901510 |
|
@fanyang-mono backporting to release/7.0-staging failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fix offset calculation for nested struct
Applying: Add a test for nested struct with pinvoke
Applying: Move and update test with real c++ implementation
Applying: Exclude newly added test from wasm
Using index info to reconstruct a base tree...
M src/tests/issues.targets
Falling back to patching base and 3-way merge...
Auto-merging src/tests/issues.targets
CONFLICT (content): Merge conflict in src/tests/issues.targets
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0004 Exclude newly added test from wasm
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! |
|
@fanyang-mono an error occurred while backporting to release/7.0-staging, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
|
@fanyang-mono backporting to release/6.0-staging failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fix offset calculation for nested struct
Using index info to reconstruct a base tree...
M src/mono/mono/mini/mini-amd64.c
Falling back to patching base and 3-way merge...
Auto-merging src/mono/mono/mini/mini-amd64.c
Applying: Add a test for nested struct with pinvoke
Applying: Move and update test with real c++ implementation
Applying: Exclude newly added test from wasm
Using index info to reconstruct a base tree...
M src/tests/issues.targets
Falling back to patching base and 3-way merge...
Auto-merging src/tests/issues.targets
CONFLICT (content): Merge conflict in src/tests/issues.targets
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0004 Exclude newly added test from wasm
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! |
|
@fanyang-mono an error occurred while backporting to release/6.0-staging, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
2 participants
Fixes: #90955
This is an existing bug for both JIT and AOT. Basically, the issue was that the offset from one layer above wasn't carried on when calculating offset for nested struct fields. I've added a test to lock down this behavior and verified that the test failed without this change.