How does preserving something via xml necessarily mean that it is reflected?

If a method is private and accessed via a method other than the visible number of callers in the assembly, the call happens through a method other than traditional C#-compiled function calls. This would require either managed reflection or an equivalent unmanaged reflection of the method through the metadata tables.

If the word "Reflected" portrays a false connotation that this runtime code reflection / invocation happens through the managed System.Reflection, I'm happy to take any suggestions to rename it to.

I wanted to clarify that it was more specific than just "ExternallyVisible" though. This won't be applied to public methods, only private ones that we know can be accessed.

I was thinking of methods called from native code. It wasn't clear to me if you wanted to capture these as well.

I don't have a better naming suggestion at the moment.

I wanted to clarify that it was more specific than just "ExternallyVisible" though. This won't be applied to public methods, only private ones that we know can be accessed.

Are you also going to prevent the non-reflectable methods from being invoked via reflection (or other indirect means) at runtime?

Otherwise, you can get very weird crashes when the IL linker guesses wrong and the optimizations takes advantage of invariant that does not hold.

We have a very similar attribute for .NET Native, just with a opposite polarity: https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/ReflectionBlockedAttribute.cs. The AOT compiler takes advantage of this attribute for optimizations, but the runtime also prevents these methods from being invoked by reflection.

The opposite polarity is nice because it means that files that IL Linker didn't touch (because we didn't run it) will be set up the right way ("assume everything can be accessed by reflection").

Oh, nice. I didn't think there already was something with those semantics.

I was trying to avoid the size increase of tagging the majority of methods with an attribute. There's definitely soundness gains from doing it the way suggested here though.

Couple things

1. From a design perspective I don't think a "SweepStep" should inject additional attributes. The steps role is to "Sweep".
2. For any injection of new attributes, I would like to have a switch to turn it off. I don't see why we would want these extra attributes in our UnityLinker. It would just increase code size.

For #2, mono embedders using the LLVM AOT backend can expect to see code size reductions when using this attribute: mono/mono#13697

For #2, mono embedders using the LLVM AOT backend can expect to see code size reductions when using this attribute: mono/mono#13697

That's good to know.

Currently, we don't use the LLVM AOT backend so for us it would only increase size.

I don't really care what the switch looks like. Something like the following may kill two birds with one stone.

protected virtual GetMonoCodeReflectedAttributeCtor()
{
     return System.Type.GetType ("Mono.Codegen+Reflected")?.GetConstructor (Array.Empty<System.Type> ());
}

		void AddReflectedAttr (MethodDefinition method, AssemblyDefinition assembly)
		{
			// Public methods have non-visible call sites by default, this attribute doesn't
			// help us at all.
			//
			// See mono_aot_can_specialize in aot-compiler.c in mono
			if (!method.IsPrivate)
				return;

			var reflectionMethod = GetMonoCodeReflectedAttributeCtor();
			if (reflectionMethod == null)
				return;

			MethodReference methodRef = assembly.MainModule.Import (reflectionMethod);
			method.CustomAttributes.Add(new CustomAttribute (methodRef));
		}

We could override GetMonoCodeReflectedAttributeCtor to turn this behavior off. And the null check would fix the tests that are failing with this PR when ran on windows against the .NET Framework assemblies.

Or you could pull this logic out into it's own step. Solving the slightly confusing situation of this being in the SweepStep and then giving us the ability to simply not register this new step.

I moved this to the CodeRewriter step. Can definitely put it in it's own step instead.

I don't like this. It effectively hides a code size increasing behavior from all tests. I believe this change is the first time monolinker would begin adding to size. That makes me less inclined to believe it is OK to hide this behavior.

I can make this optional if the addition of attributes is too much, but the codegen size savings are going to motivate it being the default for Xamarin when possible.

My comment here is with regards to how tests should work. If I look at a test file, I should be able to understand the changes the linker is expected to make. Quietly filtering out an attribute injected by the linker makes it so that one can no longer understand all of the changes the linker will make.

Whether or not this new behavior is on or off by default is a separate discussion.

It's easy enough for me to just add a set of these tests with it disabled and a set with it enabled. I'll do that instead.

I agree with Mike this is not the correct way to filter the attribute and in reality we want exact opposite

Won't think lookup the type in the currently running linker process?

Please rename the method

This is wrong, you need to use target reference type. Use FindPredefinedType instead

I gave that a try before and Cecil reported that it couldn't import the MethodImplAttribute class when doing some emitter pass. I'll look back into it.

Still running into cecil issues, cecil doesn't see any of the constructors for that type beyond the copy constructor.

Also this:

https://github.com/mono/linker/blob/fa9ccbdaf6907c69ef1bb117906f8f012218d57f/src/linker/Linker.Steps/CodeRewriterStep.cs#L51

doesn't actually change the emitted method's attributes at all.

It's also definitely not tested, because if I remove those lines the tests pass.

Fyi, the test infrastructure does not assert ImplAttributes currently. See VerifyMethodKept.VerifyMethodKept. We should hook up the same pattern used for VerifyPseudoAttributes

use Mono.Cecil.MethodImplAttributes value directly

Please undo this change

Where is this file?

I agree with Mike this is not the correct way to filter the attribute and in reality we want exact opposite

This needs to be documented and please try to come up with more specific name (this is too vague)

This should be conditional

This check should happen before you populate the list

Please add tests for the new logic

Something to watch out for.

You can run the linker tests on windows. When you do, the tests are linked against the .net framework assemblies. I doubt they have this type which I suspect means this exception is going to be hit.

As long as --annotate-unseen-callers is opt-in, the solution is simple. Any tests you add that turn on --annotate-unseen-callers will likely need to be ignored on Windows.

I would move this out into it's own top level directory and add a new test method w/ [TestCaseSource] in TestSuites.cs. Two reasons

1. To date, the Reflection folder has been focused on tests for the detection & marking of reflection usage. I feel like this is a separate behavior from that.
2. It will be easy to ignore all of these tests on Windows if they all fall under a single [TestCaseSource] method . See https://github.com/mono/linker/pull/506/files#r270871566

Indent

I believe you will need to call Resolve() on cattr.AttributeType and cattr.Constructor and then mark the TypeDefinition and MethodDefinition instead. Marking the TypeReference and MethodReference will not ensure they survive in all cases

Into this method I would add

if (Windows)
    Assert.Ignore("These tests are not valid when linking against .NET Framework");

#if NETCOREAPP
    Assert.Ignore("These tests are not valid when linking against .NET Core");
#endif

Sorry, I should have been more clear. When I said Windows, I meant "Replace me with the verbose way .NET makes you check if you are on windows"

SystemEnvironment.OSVersion.Platform == PlatformID.Win32

I thought you had some variables inherited from test infrastructure or something. That makes more sense. Thanks

This has to happen during Mark step when we know we'll need the type, see for example https://github.com/mono/linker/blob/37c9aa8a9617e4999f07de457ef2ebe02e5e3f64/src/linker/Linker.Steps/MarkStep.cs#L1894 how to mark it

Move this to mark step

You should cache this, we don't need to do the loop up in each assembly

Hmm, we need the context to look up the type reference, and the assembly to look up the definition. I'm not sure how much we can cache, but I'll look into it.

The attribute can be applied to types. It might be better to optimize for that as it'll be really used a lot otherwise (or maybe even do it at assembly level)

If all of the private methods can have this attribute, I now hoist it and apply it to the type.

The meaning of the attribute is to completely block reflection, not just the private methods on a type - there's nothing in the attribute's name implying it would be for privates only. .NET Native and CoreRT will discard metadata completely when this attribute is applied on a type (we get like 20% size on disk savings from reflection blocking in general, so aggressively discarding metadata is pretty huge).

There is an attribute with the semantic we would want for this: DisablePrivateReflectionAttribute. I would suggest using that. I know the AttributeUsage doesn't allow using it on types, but tools typically don't care. We could file a CoreFX issue to allow extending the AttributeUsage.

@MichalStrehovsky Did that here: 7dfa1ec

Rename this to ReflectionBlockedStep

I'd suggest this to be more explicit e.g. --no-reflection-methods Mark all methods never used using reflection. Defaults to false`