Add new optimization steps to make Mark step more effective by marek-safar · Pull Request #848 · dotnet/linker

marek-safar · 2019-12-06T15:12:22Z

Two new steps have been introduced

BodySubstituterStep

This step removes any conditional blocks when the condition or conditions
are evaluated as constants. This step does not do any inlining. The conditional
logic is kept but based on the known values only one branch is always taken.

A simple example which can be detected by linker.

class C
{
	static void Main ()
	{
		if (Result)
			Console.WriteLine (); // <- this call will not be marked and removed
	}

	static bool Result () => false;
}

RemoveUnreachableBlocksStep

A new command-line option called --substitutions allow external customization of any
methoda for assemblies which are linked. The syntax is same as for existing linker descriptor
XML files but it add way to control body modifications.

An example of XML file

<linker>
  <assembly fullname="test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null">
    <type fullname="Mono.Linker.Tests.Cases.Substitutions.StubBodyWithValue">
      <method signature="System.String TestMethod_1()" body="stub" value="abcd">
      </method>
    </type>
  </assembly>
</linker>

The value attribute is optional and only required when the method stub should not
fallback to default behaviour.

Additional to stub value also remove value is supported to forcibly remove body
of the method when the method is marked. This is useful when the conditional logic
cannot be evaluated by linker and the method will be marked but never actually reached.

Applicability

Both steps can be combined to achieve the effect of externally customizing which
conditional branches can be removed during the linking.

I can illustrate that on IntPtr.Size property. With following substitution any code
that has compiled in conditional logic for 64 bits handling by checking IntPtr.Size will
be removed during linking.

<linker>
  <assembly fullname="mscorlib">
    <type fullname="System.IntPtr">
      <method signature="System.Int32 get_Size()" body="stub" value="4">
      </method>
    </type>
  </assembly>
</linker>

mrvoorhe · 2019-12-06T18:08:26Z

src/linker/Linker.Steps/BodySubstituterStep.cs

+	{
+		protected override void Process ()
+		{
+			var files = Context.Substitutions;


Why did you decide to go with a separate list of files instead of extending the ResolveFromXmlStep to be able to parse the body and value xml attributes?

I can see pros and cons to both approaches. Just curious what your thinking was.

There is actually not that much in common and adding if everywhere would not make it readable

mrvoorhe · 2019-12-06T18:13:23Z

src/linker/Linker.Steps/BodySubstituterStep.cs

+				string value = GetAttribute (iterator.Current, "value");
+				if (value != "") {
+					if (!TryConvertValue (value, method.ReturnType, out object res)) {
+						Context.LogMessage (MessageImportance.Normal, $"Invalid value for '{signature}' stub");


I'd argue for throwing instead of logging a message. If we log a message & ignore the item then the user doesn't get the behavior they are expecting and then they have to waste time figuring out why whatever they intended to have stubbed wasn't stubbed. Best case scenario they notice the log message and fix it, worst case they waste a lot more time fumbling around. In either case you are wasting more of the users time than just throwing and telling them what mistake they made.

If you are not a fan of that, then my other recommendations would be

Use MessageImportance.High instead.

Put the LogMessge call into a protected virtual method so that we can choose to change the behavior if we want.

Fixed by using MessageImportance.High

mrvoorhe · 2019-12-06T18:16:12Z

src/linker/Linker.Steps/BodySubstituterStep.cs

+				Annotations.SetAction (method, MethodAction.ConvertToStub);
+				return;
+			default:
+				Context.LogMessage (MessageImportance.Normal, $"Unknown body modification '{action}' for '{signature}'");


Whatever is decided above, same here.

Ditto as above

mrvoorhe · 2019-12-06T18:21:00Z

src/linker/Linker.Steps/CodeRewriterStep.cs

-			il.Emit (OpCodes.Ldc_I4_0);
-			il.Emit (OpCodes.Ret);
-			return body;
+			throw new NotImplementedException (method.ReturnType.MetadataType.ToString ());


I think it would be nice to display the name of the method that was beings stubbed in the exception message.

mrvoorhe · 2019-12-06T18:39:26Z

test/Mono.Linker.Tests.Cases/Substitutions/RemoveBody.cs

+		public static void Main()
+		{
+			new RemoveBody ();
+            new NestedType (5);


Indent looks messed up in a couple places in this file.

mrvoorhe · 2019-12-06T18:50:20Z

test/Mono.Linker.Tests.Cases/UnreachableBlock/TryFinallyBlocks.cs

+			}
+		}        
+
+        [Kept]


mrvoorhe · 2019-12-06T20:44:10Z

test/Mono.Linker.Tests.Cases/UnreachableBlock/MultiStageRemoval.cs

+		[ExpectBodyModified]
+		static void TestMethod_1 ()
+		{
+			if (TestProperty_int () == 0)


Once we got into more complex body editing and leveraging [ExpectBodyModified] I noticed it wasn't that hard to have bugs that resulted in valid IL but the modified IL was wrong. If you were really diligent with your test setup and calling methods and should be removed or kept you could prevent most bugs from slipping by. However, for added safety we added a series of attributes to the test framework to invoke the linked code.

We a few attributes that will load the input and output assemblies in an AppDomain and invoke methods and check their behavior.
[CheckInvokeMatchesOriginal]
[CheckInvoke(<expected result>)]
[CheckInvokeThrows]

And we have one other attribute that you can put on the test case type itself that will tell the test framework to run the input and output test.exe and compare their output.
[CheckOutput]

If you would like me to bring these upstream let me know.

I think peverify is able to catch such issue so no for now

Therzok · 2019-12-08T14:25:50Z

src/linker/Linker.Steps/BodySubstituterStep.cs

+		static XPathDocument GetSubstitutions (Stream substitutions)
+		{
+			using (StreamReader sr = new StreamReader (substitutions)) {
+				return new XPathDocument (new StringReader (sr.ReadToEnd ()));


Avoid loading the whole stream into a string, then making another reader on top. This should help with performance of the operation and is simpler

Suggested change

return new XPathDocument (new StringReader (sr.ReadToEnd ()));

return new XPathDocument (sr);

Two new steps have been introduced BodySubstituterStep This step removes any conditional blocks when the condition or conditions are evaluated as constants. This step does not do any inlining. The conditional logic is kept but based on the known values only one branch is always taken. A simple example which can be detected by linker. ```c# class C { static void Main () { if (Result) Console.WriteLine (); // <- this call will be marked and removed } static bool Result () => false; } ``` RemoveUnreachableBlocksStep A new command-line option called `--substitutions` allow external customization of any methoda for assemblies which are linked. The syntax is same as for existing linker descriptor XML files but it add way to control body modifications. An example of XML file ```xml <linker> <assembly fullname="test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null"> <type fullname="Mono.Linker.Tests.Cases.Substitutions.StubBodyWithValue"> <method signature="System.String TestMethod_1()" body="stub" value="abcd"> </method> </type> </assembly> </linker> ``` The `value` attribute is optional and only required when the method stub should not fallback to default behaviour. Additional to `stub` value also `remove` value is supported to forcibly remove body of the method when the method is marked. This is useful when the conditional logic cannot be evaluated by linker and the method will be marked but never actually reached. Applicability Both steps can be combined to achieve the effect of externally customizing which conditional branches can be removed during the linking. I can illustrate that on IntPtr.Size property. With following substitution any code that has compiled in conditional logic for 64 bits handling by checking IntPtr.Size will be removed during linking. ```xml <linker> <assembly fullname="mscorlib"> <type fullname="System.IntPtr"> <method signature="System.Int32 get_Size()" body="stub" value="4"> </method> </type> </assembly> </linker> ``` Implements dotnet#752

marek-safar force-pushed the inliner branch 2 times, most recently from f884c05 to ec39b3c Compare December 6, 2019 17:54

mrvoorhe reviewed Dec 6, 2019

View reviewed changes

marek-safar force-pushed the inliner branch 5 times, most recently from 24bfd40 to 70cc9ca Compare December 8, 2019 12:19

Therzok reviewed Dec 8, 2019

View reviewed changes

marek-safar force-pushed the inliner branch from 70cc9ca to edefe58 Compare December 9, 2019 10:27

marek-safar force-pushed the inliner branch from edefe58 to 78965be Compare December 9, 2019 11:14

marek-safar merged commit 132d8af into dotnet:master Dec 10, 2019

marek-safar deleted the inliner branch December 10, 2019 23:05

jkotas mentioned this pull request Feb 5, 2020

Configure illink task to trim unreachable platform-specific code dotnet/runtime#31785

Closed

+              			}
+              		}
+                      [Kept]

	return new XPathDocument (new StringReader (sr.ReadToEnd ()));
	return new XPathDocument (sr);

Conversation

marek-safar commented Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

BodySubstituterStep

RemoveUnreachableBlocksStep

Applicability

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!

3 participants

marek-safar commented Dec 6, 2019 •

edited

Loading