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

Use similar types for self-referential generics instead of the exact canonical type #83995

Merged
merged 8 commits into from
Apr 4, 2023

Conversation

davidwrighton
Copy link
Member

@davidwrighton davidwrighton commented Mar 27, 2023

  • Take advantage of work done a few years ago to simplify the interaction with the interop subsystem

  • In the problematic case, simulate loads with two different types as instantiations which are unrelated in field layout, and see if they match up. Only enable this code in a few very small isolated parts of the runtime

  • Filter more of the type loader logic through the byvalue class cache which should improve performance a bit

  • Similarly when considering blittability, tweak the logic to use the special load path

  • Support for self-recursive generics is not enabled for static fields, as that requires a somewhat different tweak, and there is less apparent demand. (For that scenario self-referential generics really should support having fields of type T.)

  • Support for indirect self-recursive generics is also not enabled. The approach taken here is not practical for that, and there does not appear to be significant demand for that either.

Fixes #6924

- Take advantage of work done a few years ago to simplify the interaction with the interop subsystem
- In the problematic case, simulate loads with two different types as instantiations which are unrelated in field layout, and see if they match up. Only enable this code in a few very small isolated parts of the runtime
- Filter more of the logic through the byvalue class cache which should improve performance a bit
- Similarly when considering blittability, tweak the logic to use the special load path

- The current fix is expected to break Unix X64, but I believe the rest of the OS/Architecture pairs should work
- Handling of static fields also needs examination for correctness
@ghost ghost assigned davidwrighton Mar 27, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 27, 2023
@davidwrighton
Copy link
Member Author

Fixes #6924

@davidwrighton davidwrighton changed the title Potential fix for #6924 Use similar types for self-referential generics instead of the exact canonical type Mar 28, 2023
@davidwrighton davidwrighton marked this pull request as ready for review March 28, 2023 00:33
@davidwrighton davidwrighton removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 28, 2023
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

I'm still going through this.

src/coreclr/vm/fieldmarshaler.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/class.h Outdated Show resolved Hide resolved
Comment on lines +554 to +558
// Everett C++ compiler can generate a TypeRef with RS=0
// without respective TypeDef for unmanaged valuetypes,
// referenced only by pointers to them.
// In such case, GetTypeHandleThrowing returns null handle,
// and we return E_T_VOID
Copy link
Member

Choose a reason for hiding this comment

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

I think we should update this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably, but this change is confusing enough as it is without adding a known potential behavior change. I'd like to see that done as a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you do that, please get rid of the weirdo ThrowButNullV11McppWorkaround logic too.

src/coreclr/vm/siginfo.cpp Outdated Show resolved Hide resolved
@@ -2238,11 +2239,20 @@ static SystemVClassificationType ReClassifyField(SystemVClassificationType origi
}
}

MethodTable* ByValueClassCacheLookup(MethodTable** pByValueClassCache, unsigned index)
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT Mar 31, 2023

Choose a reason for hiding this comment

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

Suggested change
MethodTable* ByValueClassCacheLookup(MethodTable** pByValueClassCache, unsigned index)
static MethodTable* ByValueClassCacheLookup(MethodTable** pByValueClassCache, uint32_t index)

}
}
}
handlingRecursiveGenericFieldScenario = true;
Copy link
Member

Choose a reason for hiding this comment

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

recursionDetected not used?

Comment on lines 209 to 215
CONTRACTL
{
THROWS;
GC_TRIGGERS;
MODE_ANY;
}
CONTRACTL_END;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CONTRACTL
{
THROWS;
GC_TRIGGERS;
MODE_ANY;
}
CONTRACTL_END;
STANDARD_VM_CONTRACT;

@@ -324,6 +324,9 @@ class FieldDesc
// Return -1 if the type isn't loaded yet (i.e. if LookupFieldTypeHandle() would return null)
UINT GetSize();

// Return -1 if the type is a structure and pMTOfValueTypeField is NULL and LookupFieldTypeHandle() returns null
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually depend on this returning -1 anywhere? It seems we always assume that it is going to succeed. Should we make this more restrictive and assert instead of returning -1?

@jkotas
Copy link
Member

jkotas commented Apr 1, 2023

This does not seem to handle indirectly self-referential generics. For example, this compiles fine, but still fails to run:

struct N<T> { }
struct M { public N<Nullable<M>> E; }

Is it expected that this does not work?

EDIT: https://github.com/dotnet/runtime/pull/83995/files#diff-07105ca3a0aa4623290e4d68b73dbba2ff71b75d61ab7b0ee4f28d91e3a64864R1414 comment says this is not expected to work.

@davidwrighton davidwrighton merged commit bc887b3 into dotnet:main Apr 4, 2023
@teo-tsirpanis teo-tsirpanis added this to the 8.0.0 milestone Apr 4, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unused generic type parameter should not cause loader failure
4 participants