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

Add option to the XML input to rewrite methods to throw / stubs. #555

Closed
wants to merge 1 commit into from

Conversation

baulig
Copy link

@baulig baulig commented Apr 30, 2019

When using feature=<feature> to conditionalize a <method>, there is now a new
action=<action> attribute (which currently allows throw and stub).

You can use it like this:

<type fullname="System.RuntimeType">
	<method name="MakeTypeBuilderInstantiation" action="throw" feature="sre" />
</type>

Then you run the linker with --exclude-feature sre and that method will be rewritten
into a throw. This is intended for scenarios where that method would otherwise bring
in types that we want to exclude.

When using `feature=<feature>` to conditionalize a `<method>`, there is now a new
`action=<action>` attribute (which currently allows `throw` and `stub`).

You can use it like this:

	<type fullname="System.RuntimeType">
		<method name="MakeTypeBuilderInstantiation" action="throw" feature="sre" />
	</type>

Then you run the linker with `--exclude-feature sre` and that method will be rewritten
into a throw.  This is intended for scenarios where that method would otherwise bring
in types that we want to exclude.
@baulig
Copy link
Author

baulig commented Apr 30, 2019

Turns out that we can actually do without this, by using constants.

@baulig baulig closed this Apr 30, 2019
@baulig baulig reopened this May 23, 2019
@marek-safar
Copy link
Contributor

I am not sure this is the best approach as it does not compose well. From what I can see you'd like to force explicit removal of some block of code. By coincidence, the method already exists in the descriptor but what if does not? You'd need to add it there which means it would be always persisted which is not what you actually want.

@baulig
Copy link
Author

baulig commented May 24, 2019

The problem that I'm trying to solve is that some method must be forcibly turned into a stub:

Type MakeTypeBuilderInstantiation(Type[] instantiation)
{
    return System.Reflection.Emit.TypeBuilderInstantiation.MakeGenericType(this, instantiation);
}

This is the last remaining "link" to the System.Reflection.Emit namespace and this method pulls it all in.

So the idea is that when you pass --exclude-feature sre, this method must be turned into a stub.

Do you have any suggestions how to best achieve that?

@baulig
Copy link
Author

baulig commented May 24, 2019

I am not sure this is the best approach as it does not compose well. From what I can see you'd like to force explicit removal of some block of code. By coincidence, the method already exists in the descriptor but what if does not? You'd need to add it there which means it would be always persisted which is not what you actually want.

Right, good point. In this particular example, adding that method to the preserve list does not matter because it is always referenced from already always preserved code. But it means that my approach won't work in the general sense, so having it in XML is probably not such a good idea.

I'm going to close this again, but let's discuss alternative solutions for this.

@baulig baulig closed this May 24, 2019
@mrvoorhe
Copy link
Contributor

FYI, I added something very similar to this in our UnityLinker a year or so ago. It does almost what you want. Here are some examples

    public enum StubAction
    {
        None,
        // Stub it if the method is not marked.
        IfNotUsed,
        // Always stub it
        Force
    }
    class ForceStubUsedMethod
    {
        public static void Main()
        {
            MethodViaSignature();
            MethodViaName();
        }

        [Stubbed]
        private static void MethodViaSignature()
        {
            Console.WriteLine("Bark");
        }

        [Stubbed]
        private static void MethodViaName()
        {
            Console.WriteLine("Bark");
        }
    }
<linker>
  <assembly fullname="test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null">
    <type fullname="Unity.Linker.Tests.Cases.Stubify.ForceStubUsedMethod">
      <method signature="System.Void MethodViaSignature()" stub="force"/>
      <method name="MethodViaName" stub="force"/>
    </type>
  </assembly>
</linker>

We never really ended up using this ability. If you wanted to take this approach and tweak it it wouldn't cause us any issues. It sounds like you'd want to add a StubAction.IfMarked or something along those lines so that the method would still be removed if it was not marked. That probably wouldn't be that hard.

If you are interested let me know and I can move our code & tests to support this out of UnityLinker and into monolinker and toss it on a branch for you.

@baulig
Copy link
Author

baulig commented May 28, 2019

@mrvoorhe That's really awesome - that you so much for sharing this with me!

I'm very interested in a solution that does not cause any problems for you guys and if you already have this in your code and tests for it, then I think it would be awesome to get this into monolinker, so we can build on top of it.

However, I would need some way of making this feature conditional - the method should only be stubbed if the --exclude-feature sre argument has been provided.

Could we do something like the following with it:

<linker>
  <assembly fullname="test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null">
    <type fullname="Unity.Linker.Tests.Cases.Stubify.ForceStubUsedMethod">
      <method signature="System.Void MethodViaSignature()" stub="force" feature="sre" />
      <method name="MethodViaName" stub="feature" feature="sre" />
    </type>
  </assembly>
</linker>

Ideally, the --exclude-feature sre logic should also be determined automatically, but that's a topic of it's own.

@mrvoorhe
Copy link
Contributor

However, I would need some way of making this feature conditional - the method should only be stubbed if the --exclude-feature sre argument has been provided.

When the linker is ran with --exclude-feature sre that means that elements in the xml files with feature=sre will not be processed.

It sounds like what you want to do is a little different. You want to process the xml element only if the feature is excluded. It feels like we might want a new xml attribute. Something like

<method name="MethodViaName" stub="ifmarked" if_excluded_feature="sre" />

Not the best name in the world but hopefully it gets the idea across.

Can you pin down an XML syntax for this and get buy in from @marek-safar ? It will be a little bit of work for me to move our code to a branch. I'm happy to do it, but I'd like to make sure there is buy in on the general approach before I go through it.

@baulig baulig mentioned this pull request Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants