Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix GC holes in array marshallers in the field scenario #27562

Merged
jkoritzinsky merged 6 commits into from
Nov 1, 2019
Merged

Fix GC holes in array marshallers in the field scenario #27562

jkoritzinsky merged 6 commits into from
Nov 1, 2019

Conversation

@jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Oct 30, 2019

Correctly protect references to objects in manually-managed code called by the array IL marshalers in the field scenarios. Unlike the non-field scenarios where the "managed home" address is a local variable on the stack, the "managed home" address for field scenarios is a byref into the managed object. If the managed object is a layout-class, it can be relocated/collected.

Also, use RuntimeHelpers.GetRawData to simplify layout class marshalling and reduce weird IL patterns in the marshalers when there are C# methods with better readability that we can reuse.

Fixes #27496
Fixes #27491
Fixes dotnet/corefx#42201

@jkoritzinsky
Correctly protect references to objects in manually-managed code call…
4d9a3a6 
…ed by IL marshalers in the field scenarios. Use RuntimeHelpers.GetRawData to simplify layout class marshalling.
@jkoritzinsky
Add additional protects for SAFEARRAY marshalling when used in the fi…
3b5c09e 
…eld scenario.
@jkoritzinsky
Remove dead code.
9e1ec91
@jkoritzinsky jkoritzinsky added this to the 5.0 milestone Oct 30, 2019
@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Oct 30, 2019

@jkoritzinsky Is there a test we can add that would have caught this issue when running under GC stress?

@jkoritzinsky
Copy link
Member Author

jkoritzinsky commented Oct 30, 2019

Well the two fixed-array marshalling test projects did catch this under GCStress. That's what #27496 and #27491 are from. This might not show up on tier0 code since (at least the specific failure in #27496) only shows up when the gcinfo for an object is as strict (shortest lifetime) as possible.

@jkoritzinsky
Copy link
Member Author

jkoritzinsky commented Oct 30, 2019

I initially missed this since I copied the pattern of the array marshalers from the ones already in the code base and didn't realize that the differences in the scenario (param/retval vs field) were enough to possibly cause GC holes.

@jkoritzinsky
Copy link
Member Author

jkoritzinsky commented Oct 30, 2019

@trylek I can't run the GCStress pipeline to validate this in CI. I'm getting the error:

Job run_test_p1__Linux_x64_checked depends on unknown job build_test_p1_OSX_x64_checked.

Looks like your recent changes to the pipeline may have broken the jobs that don't include an OSX leg.

@trylek
Copy link
Member

trylek commented Oct 30, 2019

Hmm, that's unfortunate, apologies about that. I think this should be fixable by tweaking the conditions in platform-matrix - it's true that with my change any Linux test execution looks for managed tests components built on OSX. I have a meeting in 5 minutes, I'll look into fixing that right after we finish. Is this the gcstress-extra pipeline where you're hitting this? I definitely need to know how to test the fix.

jkotas
jkotas reviewed Oct 30, 2019
View reviewed changes
@jkoritzinsky
Copy link
Member Author

jkoritzinsky commented Oct 30, 2019

@trylek It's the gcstress_0x3_0xc pipeline.

@jkotas
Copy link
Member

jkotas commented Oct 31, 2019 

Assert failure(PID 3592 [0x00000e08], Thread: 4268 [0x10ac]): Consistency check failed: AV in clr at this callstack:
------
CORECLR! OBJECTREF::OBJECTREF + 0x10 (0x00007ffb`8ccb0460)
CORECLR! MngdFixedArrayMarshaler::ClearNativeContents + 0xCA (0x00007ffb`8d09185a)

@jkoritzinsky
Copy link
Member Author

jkoritzinsky commented Oct 31, 2019

I'll take a look when I get into the office tomorrow.

AaronRobinsonMSFT
AaronRobinsonMSFT reviewed Oct 31, 2019
View reviewed changes
@jkoritzinsky
Copy link
Member Author

jkoritzinsky commented Nov 1, 2019

Failures in the GCStress pipeline are due to the following issues:

  • Linux_arm timeouts which have been happening for a few weeks on and off.
  • crossgen2smoke, which is being tracked by #27497
  • pinvoke_examples, which is being tracked by #27602

@jkoritzinsky jkoritzinsky merged commit e658b97 into dotnet:master Nov 1, 2019
@jkoritzinsky jkoritzinsky deleted the gcprotect-fixed-array-marshaler branch November 1, 2019 20:21
@jkoritzinsky jkoritzinsky mentioned this pull request Oct 5, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@jkotas jkotas jkotas left review comments

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-Interop

Projects

None yet

Milestone

5.0

4 participants

@jkoritzinsky @AaronRobinsonMSFT @trylek @jkotas