Fix an invalid IL bug with interface sweeping by mrvoorhe · Pull Request #463 · dotnet/linker

mrvoorhe · 2019-02-08T21:09:58Z

If a type can be on the stack then we need to keep the interface implementation on that type even if the type is never instantiated.

I think we can get away with only doing this when the using of the type and the interface are within the same body. Once beyond the body, there won't be invalid IL. Casting is similar, see ObjectHardCastedToInterface

The tests that hit the invalid IL bug were

FieldDowncastedToInterface
LocalDowncastedToInterface
ReturnValueDowncastedToInterface

The test ObjectHardCastedToInterface was passing on master. I think the expected result this test is asserting is OK.

The test LocalDowncastDoesNotCuaseOtherTypesToKeepInterface is to ensure we don't go overboard marking interface implementations.

marek-safar · 2019-02-11T10:25:57Z

I don't this is going to work. Unfortunately, there are many more places where the implicit reference conversion is done.

Consider simple

class X
{
	public static void Main ()
	{
		C c = null;
		Foo(c);
	}

	static void Foo (I i)
	{
	}
}

class C : I
{
}

interface I
{
}

mrvoorhe · 2019-02-11T12:03:17Z

Good catch. I can address that case by collecting the parameter types of all calls.

See any other cases where this falls apart?

mrvoorhe · 2019-02-11T12:06:55Z

Looks like I’m already collecting the parameter types. I will take a look at your test case later today when I get in. On the surface it seems like it should be ok.

marek-safar · 2019-02-11T12:18:04Z

what about generics with interface constraint, array operations or Activator calls?

mrvoorhe · 2019-02-11T16:06:53Z

what about generics with interface constraint

There were issues with a number of scenarios involving generics. I will update the PR with fixes shortly.

array operations

I'm adding array tests after fixing the generic issues and I'm not seeing any problems. Do you have a specific case you are concerned with?

Activator calls?

What specifically are you concerned with about Activator? In order for Activator to work, one would have to ensure an instance ctor is marked, and once an instance ctor is marked this bug is no longer an issue.

mrvoorhe · 2019-02-11T16:12:53Z

Summary of updated changes

Fixed a number of cases involving generics
Fixed an issue with interfaces on base types not being kept.
Added some array tests
Pushed as much of this logic into it's own class to help make it easier to follow.
Removed stack type collection from TypeMapStep. This is getting rather complex (and non-trivial cost) and I think it's better to do it during MarkMethodBody since that way we only have to pay the price for methods that are used.
Copied over the TypeMapStep.MapBaseTypeHierarchy change from WIP : Master base sweeping simplified #455. I needed to get base types so I figured I'd pull that change in rather than copy pasting the loop & resolve code necessary to iterate all the base types.

marek-safar · 2019-02-14T11:56:32Z

I'll have to think more about the complex scenarios but what about simple

class X
{
	public static void Main ()
	{
		object[] c = new I[2];
		c [0] = new C ();
	}
}

class C : I
{
}

interface I
{
}

mrvoorhe · 2019-02-14T13:29:36Z

@marek-safar new C() causes C to be marked as instance required which means interface implementations on C will be marked as soon as the interface implementation type is marked. In your case, I is marked so the interface will remain on C.

This bug was exclusive to reference types that do not have an instance constructor marked.

mrvoorhe · 2019-02-14T13:38:17Z

Something to consider. This PR is a bug fix, not a new feature. I think this plugs all of the holes that exist on master today. Maybe it does, or maybe there are still edge cases hiding. Either way, master is better off with this PR than without it.

If the approach in this PR seems acceptable, it doesn't introduce new bugs, and you have no concerns about undesirable side effects of the changes, then I'd rather we land this PR and then worry about other bugs than leave the PR open waiting to think of all possible bugs.

marek-safar · 2019-02-14T13:35:49Z


+		bool IsInterfaceImplementationMarked (TypeDefinition type, TypeDefinition interfaceType)
+		{
+			if (type.Implements (@interfaceType, out InterfaceImplementation implementation))


Could be simple return type.Implements (@interfaceType, out InterfaceImplementation implementation) && Annotations.IsMarked (implementation);

marek-safar · 2019-02-14T13:43:05Z

+
+namespace Mono.Linker {
+	public static class TypeDefinitionExtensions {
+		public static bool Implements (this TypeDefinition type, TypeDefinition interfaceType, out InterfaceImplementation implementation)


The name is not right, it should be something like HasInterface (type, out iface)

marek-safar · 2019-02-14T13:44:31Z

+using Mono.Cecil.Cil;
+
+namespace Mono.Linker {
+	public static class BodyRequirementsCollector {


Let's call it MethodBodyScanner instead

Will do. I was struggling to come up with a good name.

marek-safar · 2019-02-14T13:46:16Z

+
+namespace Mono.Linker {
+	public static class BodyRequirementsCollector {
+		public static IEnumerable<InterfaceImplementation> InterfacesNeededByBody (AnnotationStore annotations, MethodBody body)


and this one GetReferencedInterfaces

marek-safar · 2019-02-14T13:48:45Z

+	public static class BodyRequirementsCollector {
+		public static IEnumerable<InterfaceImplementation> InterfacesNeededByBody (AnnotationStore annotations, MethodBody body)
+		{
+			var interfaceImplementations = new HashSet<InterfaceImplementation> ();


Return null instead of empty cases

marek-safar · 2019-02-14T13:49:57Z

+			if (possibleStackTypes.Count == 0)
+				return interfaceImplementations;
+
+			var interfaceTypes = possibleStackTypes.Where (t => t.IsInterface).Distinct ().ToArray ();


Why do we need Distinct over Hashset?

Artifact of an earlier version where I was return List from AllPossibleStackTypes. I missed this while cleaning up. Fixed.

marek-safar · 2019-02-14T13:50:50Z

+				return interfaceImplementations;
+
+			// We only sweep interfaces on classes so that's why we only care about classes
+			var classes = possibleStackTypes.Where (t => t.IsClass).Distinct ().ToArray ();


This loop is redundant, you can fold it to the one bellow

marek-safar · 2019-02-14T13:58:44Z

 			return marked_types_with_cctor.Add (type);
 		}

+		public void SetBaseHierarchy (TypeDefinition type, List<TypeDefinition> bases)


This is for classes only, the name should reflect that (e.g. SetClassHierarchy)

marek-safar · 2019-02-14T14:32:14Z

+			if (classes.Length == 0)
+				return interfaceImplementations;
+
+			// If a type could be on the stack in the body and an interface it implements could be on the stack on the body


I am not sure this will cover all cases (will have to experiment with it)

One thing we don't have tests for, and I thought this might not be enough to address is covariance and contravariance.

If there is a bug with covariance and/or contravariance I'd rather address it in a follow on pr. That is an increasingly less common scenario where this interface sweep bug could appear.

If a type can be on the stack then we need to keep the interface implementation on that type even if the type is never instantiated. I think we can get away with only doing this when the using of the type and the interface are within the same body. Once beyond the body, there won't be invalid IL. Casting is similar, see `ObjectHardCastedToInterface`

mrvoorhe requested a review from marek-safar as a code owner February 8, 2019 21:09

mrvoorhe force-pushed the master-interface-sweep-bug-fix branch 2 times, most recently from 703fea0 to 32cffcb Compare February 8, 2019 21:22

mrvoorhe force-pushed the master-interface-sweep-bug-fix branch from 32cffcb to dd8b93e Compare February 11, 2019 16:13

marek-safar requested changes Feb 14, 2019

View reviewed changes

mrvoorhe force-pushed the master-interface-sweep-bug-fix branch from dd8b93e to 238025e Compare February 14, 2019 15:59

marek-safar merged commit e641487 into dotnet:master Feb 14, 2019

MichalStrehovsky mentioned this pull request Aug 11, 2020

MarkInterfacesNeededByBodyStack keeps more interfaces than necessary #1428

Open

Conversation

mrvoorhe commented Feb 8, 2019

Uh oh!

marek-safar commented Feb 11, 2019

Uh oh!

mrvoorhe commented Feb 11, 2019

Uh oh!

mrvoorhe commented Feb 11, 2019

Uh oh!

marek-safar commented Feb 11, 2019

Uh oh!

mrvoorhe commented Feb 11, 2019

Uh oh!

mrvoorhe commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marek-safar commented Feb 14, 2019

Uh oh!

mrvoorhe commented Feb 14, 2019

Uh oh!

mrvoorhe commented Feb 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mrvoorhe commented Feb 11, 2019 •

edited

Loading