Report by-ref-like types to the GC (SpanOfT) #9034

kouvel · 2017-01-21T00:56:34Z

Fixes #8517

kouvel · 2017-01-21T00:57:11Z

jkotas · 2017-01-21T01:06:54Z

src/vm/siginfo.cpp

+
+    _ASSERTE(pMT->IsByRefLike());
+
+    if (pMT == g_TypedReferenceMT)


This should do the same thing as g_pByReferenceClass:

if (pMT->HasSameTypeDefAs(g_pByReferenceClass) || pMT == g_TypedReferenceMT)

I don't understand, could you please elaborate?

Once the rules for byref-like type in C# compiler are relaxed, it should be possible to write:

struct MyStruct { TypedReference field1; Span<byte> field2; TypedReference field3; }

field1 and field3 won't be reported correctly because of the early out.

CEEInfo::getClassGClayout has the same bug

It looks to me like CEEInfo::getClassGClayout would only return early if the topmost object is a TypedReference, which seems acceptable. If a by-ref-like object contains a TypedReference as in your example, it would call ComputeGCLayout, which would iterate over all fields in the by-ref-like type. I'm not seeing an issue there either.

The first field of TypedReference is actually byref, but it is not recorded as byref in metadata. It has been special cased for GC reporting in multiple places. This is yet another places that should be special cased for it.

Ideally, we would change the implementation of TypedReference to:

struct TypedReference { ByReference<byte> _value; IntPtr _type; ... }

and get rid of the special casing. But until/if that happens, we should continue special casing it.

If TypedReference is not recorded as byref, then it seems to me like it would have already had an issue before this change - it seems then that TypedRefernece would not have been reported properly to the GC. Are you suggesting that there is an issue with the reporting for TypedReference as well? If so, I'll need to test with TypedReference as well to see the problem before fixing it.

The TypedReference has not been allowed as a field and so there is no problem today. They have been only allowed as regular arguments. We should not come for regular TypedReference arguments at all because of they are special cased above ReportPointersFromValueType.

Since you have added the special case for TypedReference here, I thought you want to prepare this code for case where TypedReference are allowed as fields of other byref-like types. There is a good chance that TypedReferences are going to be allowed as fields as natural fallout from the relaxations of rules for byref-like types in C#.

Either of these two options would sound reasonable to me:

Do not prepare this for TypedReference fields - do not handle TypedReference at all here

Prepare this for TypedReference fields - handle TypedReference same way as ByReference<T> here

Ok how does 797ec39 look? I kept the check considering TypedReference is currently not implemented the way in which you suggest, but the new code treats it similarly to ByReferece in the sense the first field of both is the pointer, which should conform to the way in which TypedReference is currently implemented.

jkotas · 2017-01-21T01:11:57Z

src/vm/siginfo.cpp

-    if (!pMT->ContainsPointers())
-        return;
+
+    if(pMT->ContainsPointers())


Why can't this be just

if (!pMT->ContainsPointers() && !pMT->IsByRefLike()) return; ... existing code...

?

(I would be useful to add some comments if it needs to be what you have written.)

As I currently do not have a complete understanding of GC holes with respect to Unix and structs passed in registers, I'm conservatively preserving the previous behavior by calling ReportPointersFromStructInRegisters only under the previous conditions. #9033 is intended to track potential issues on Unix, a fix for which should hopefully resolve the question.

jkotas · 2017-01-21T05:14:05Z

src/vm/siginfo.cpp

+
+    _ASSERTE(pMT->IsByRefLike());
+
+    // TODO: TypedReference should ideally be implemneted as a by-ref-like struct containing a ByReference<T> field,


Nit: typo implemneted

jkotas · 2017-01-21T05:15:47Z

It would be nice to make a matching change in CEEInfo::getClassGClayout in vm\jitinterface.cpp so that it is sync. Move the special case for g_TypedReferenceMT from CEEInfo::getClassGClayout to ComputeGCLayout. Something like:

static unsigned ComputeGCLayout(MethodTable * pMT, BYTE* gcPtrs)
{
    STANDARD_VM_CONTRACT;

    unsigned result = 0;

    _ASSERTE(pMT->IsValueType());

    // TODO: TypedReference should ideally be implemented as a by-ref-like struct containing a ByReference<T> field,  
    // in which case the check for g_TypedReferenceMT below would not be necessary
    if (pMT == g_TypedReferenceMT)
    {
        if (gcPtrs[0] == TYPE_GC_NONE)
        {
            gcPtrs[0] = TYPE_GC_BYREF;
            result++;
        }
        else if (gcPtrs[fieldStartIndex] != TYPE_GC_BYREF)
        {
            COMPlusThrowHR(COR_E_BADIMAGEFORMAT);
        }
        return result;
    }

    ... existing code ...
}

jkotas · 2017-01-21T05:15:54Z

LGTM otherwise. Thanks!

kouvel · 2017-01-21T05:53:36Z

if (gcPtrs[0] == TYPE_GC_NONE)

It looks like getClassGClayout memsets it to TYPE_GC_NONE but the recursive call from ComputeGCLayout does not, so I have omitted this part.

else if (gcPtrs[fieldStartIndex] != TYPE_GC_BYREF)

I don't see where this array is initialized to some default value, so I have omitted this part as well. Simplified to:

    if (pMT == g_TypedReferenceMT)
    {
        gcPtrs[0] = TYPE_GC_BYREF;
        gcPtrs[1] = TYPE_GC_NONE;
        return 1;
    }

And added the comment to getClassGClayout as well, which if/when changed, would remove the necessity for the addition to ComputeGCLayout.

kouvel · 2017-01-21T20:23:12Z

@dotnet-bot test this please

kouvel · 2017-01-22T07:26:29Z

@dotnet-bot test this please

Report by-ref-like types to the GC (SpanOfT) Fixes dotnet/coreclr#8517 Commit migrated from dotnet/coreclr@62ac5f0

Report by-ref-like types to the GC (SpanOfT)

2f21b7b

Fixes #8517

dnfclas added the cla-already-signed label Jan 21, 2017

jkotas reviewed Jan 21, 2017

View reviewed changes

Address feedback

797ec39

kouvel force-pushed the SpanGcReporting branch from ba18db1 to 797ec39 Compare January 21, 2017 04:59

jkotas reviewed Jan 21, 2017

View reviewed changes

Address feedback

442f56a

jkotas approved these changes Jan 21, 2017

View reviewed changes

kouvel merged commit 62ac5f0 into dotnet:master Jan 23, 2017

kouvel deleted the SpanGcReporting branch January 23, 2017 07:10

karelz modified the milestone: 2.0.0 Aug 28, 2017

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Report by-ref-like types to the GC (SpanOfT) (dotnet/coreclr#9034)

ac00821

Report by-ref-like types to the GC (SpanOfT) Fixes dotnet/coreclr#8517 Commit migrated from dotnet/coreclr@62ac5f0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report by-ref-like types to the GC (SpanOfT) #9034

Report by-ref-like types to the GC (SpanOfT) #9034

kouvel commented Jan 21, 2017

kouvel commented Jan 21, 2017

jkotas Jan 21, 2017

kouvel Jan 21, 2017

jkotas Jan 21, 2017

jkotas Jan 21, 2017

jkotas Jan 21, 2017

kouvel Jan 21, 2017

jkotas Jan 21, 2017

kouvel Jan 21, 2017 •

edited

Loading

jkotas Jan 21, 2017 •

edited

Loading

kouvel Jan 21, 2017

jkotas Jan 21, 2017

jkotas Jan 21, 2017

kouvel Jan 21, 2017

jkotas Jan 21, 2017

kouvel Jan 21, 2017

jkotas commented Jan 21, 2017 •

edited

Loading

jkotas commented Jan 21, 2017

kouvel commented Jan 21, 2017 •

edited

Loading

kouvel commented Jan 21, 2017

kouvel commented Jan 22, 2017


		_ASSERTE(pMT->IsByRefLike());

		// TODO: TypedReference should ideally be implemneted as a by-ref-like struct containing a ByReference<T> field,

Report by-ref-like types to the GC (SpanOfT) #9034

Report by-ref-like types to the GC (SpanOfT) #9034

Conversation

kouvel commented Jan 21, 2017

kouvel commented Jan 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kouvel Jan 21, 2017 • edited Loading

Choose a reason for hiding this comment

jkotas Jan 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Jan 21, 2017 • edited Loading

jkotas commented Jan 21, 2017

kouvel commented Jan 21, 2017 • edited Loading

kouvel commented Jan 21, 2017

kouvel commented Jan 22, 2017

kouvel Jan 21, 2017 •

edited

Loading

jkotas Jan 21, 2017 •

edited

Loading

jkotas commented Jan 21, 2017 •

edited

Loading

kouvel commented Jan 21, 2017 •

edited

Loading