Unable to resolve some types from System.Runtime.dll in custom linker step

[rolfbjarne]

Repro:

removedtypeforwarder-7b80c56.zip

Download & extract && run ./test.sh (I'm using .NET 6.0.100-rc.2.21420.30, if you have a different version, you'll probably have to update a few paths at the top of the script to find illink.dll).

$ ./test.sh
[...]
ILLink: iOS error IL6999: Failed to execute the custom steps.
error MT4111: The registrar cannot build a signature for type `Bindings.Test.Sf' in method `Sf()`.
[...]

Full output: https://gist.github.com/rolfbjarne/53ced784587bc75b25bba67ece307891

This is what happens:

We have a custom linker step that runs at the end of the linker process (after the linked output has been saved), and looks at certain types. When it fails, it's looking at the Bindings.Test.Sf struct in the bindings-test.dll. This is a struct with a single float:

$ monodis linked/bindings-test.dll| grep "class public sequential ansi sealed beforefieldinit Sf$"  -A 13    
  .class public sequential ansi sealed beforefieldinit Sf
  	extends [System.Runtime]System.ValueType
  {
    .field  public  float32 x0

    // method line 505
    .method public virtual hidebysig 
           instance default string ToString ()  cil managed 
    {
        // Method begins at RVA 0x5280
    } // end of method Sf::ToString

  } // end of class Bindings.Test.Sf
}

The error occurs because the field's type can't be resolved here:

https://github.com/xamarin/xamarin-macios/blob/8dc41314ed3c5f90376d87c760a697bf942d0589/tools/common/StaticRegistrar.cs#L2318-L2320

I've added the following debug spew locally: https://gist.github.com/rolfbjarne/034debd1d54c5273e22ac47f44405cfa, and this is printed earlier (you can also see this output in the full log output from above):

Unable to resolve System.Single of scope System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a

The interesting part is that fields of other types are resolved just fine:

Resolved System.Int32 of scope System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a to System.Int32

The difference seems to be that the linked version of System.Runtime doesn't have a type forwarder for System.Single:

$ monodis --forward-decls linked/System.Runtime.dll|grep System.Single
[no output]

while it does for System.Int32:

$ monodis --forward-decls linked/System.Runtime.dll|grep System.Int   
.class extern forwarder System.Int32
.class extern forwarder System.IntPtr

It seems the version of bindings-test.dll that's in memory has references to type forwarders in System.Runtime.dll that have been removed.

[rolfbjarne]

Looks like there's a file in my repro that still points into paths on my local system; this version hopefully works better:

removedtypeforwarder-7b80c56.zip

[MichalStrehovsky]

IL Linker will strip away unused forwarders. Did marking step see the bindings-test.dll, or was that added after sweep was done? If it's the latter, it's kind of expected. Adding assemblies (either in custom steps or at runtime with APIs like Assembly.LoadFrom) will not work great.

[rolfbjarne]

IL Linker will strip away unused forwarders. Did marking step see the bindings-test.dll, or was that added after sweep was done? If it's the latter, it's kind of expected. Adding assemblies (either in custom steps or at runtime with APIs like Assembly.LoadFrom) will not work great.

The marking step saw bindings-test.dll. No assemblies were added after the linker started executing, either in a custom step or with Assembly.LoadFrom.

This is a snippet from interdependent-binding-projects.dll (the executable assembly):

	IL_0053:  ldarg.0 
	IL_0054:  ldfld class [Touch.Client]MonoTouch.NUnit.UI.TouchRunner AppDelegate::runner
	IL_0059:  ldtoken ['bindings-test']Xamarin.BindingTests.ProtocolTest
	IL_005e:  call class [mscorlib]System.Type class [mscorlib]System.Type::GetTypeFromHandle(valuetype [mscorlib]System.RuntimeTypeHandle)
	IL_0063:  callvirt instance class [mscorlib]System.Reflection.Assembly class [mscorlib]System.Type::get_Assembly()
	IL_0068:  callvirt instance void class [Touch.Client]MonoTouch.NUnit.UI.BaseTouchRunner::Add(class [mscorlib]System.Reflection.Assembly)

The log shows this at some point:

Assembly bindings-test, Version=1.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065 : skipped (Copy)

I'm not sure what's being skipped here.

The final action seems to be Save:

ILLink: Output action: Save assembly: bindings-test, Version=1.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065

The default action is copy, this is what we pass to the linker (the bindings-test.dll assembly does not have any assembly-level linker attributes saying it's linkable):

--trim-mode link --action copy

[vitek-karas]

@sbomer assigning to you... since it needs a mac.

[sbomer]

This has to do with how Cecil represents references to built-in types. When the linker is marking the field type of Sf, Cecil reads the field signature (which has a token representing the single) and constructs a TypeReference that has System.Runtime as a scope. It picks this scope somewhat arbitrarily - it uses the first AssemblyReference that it finds in metadata to one of System.Runtime/mscorlib/SPC/netstandard.

Usually for copy assemblies we mark things pointed to by any typerefs:
https://github.com/mono/linker/blob/8b7695af6debcbdaad531d521f8a8e10c553541a/src/linker/Linker.Steps/MarkStep.cs#L1388-L1389

But this check doesn't cover typereferences which don't exist in metadata (like those constructed for built-in types), so the forwarder in System.Runtime isn't marked. I think it actually doesn't need to be marked since the final metadata written to disk shouldn't reference it, but when the custom step calls Resolve, it hits issues if we happened to remove the forwarder.

I can see a few ways to fix it:

• Mark these forwarders. This might keep around unnecessary forwarders for built-in types. It's probably not concerning from a size perspective, but might produce extra files in some cases. If possible I'd prefer to remove them.
• Update the in-memory TypeReferences so they point to System.Private.CoreLib. We already do this for copyused and link assemblies. Here we'd need to do it for copy assemblies as well, but only for the TypeReferences that won't be persisted. This would produce a cleaner output, but could have issues if someone tries to read the produced assembly back into Cecil, since it might create a TypeReference using System.Runtime even if we removed some forwarders.

Based on recent conversation with @vitek-karas I think type references encoded as custom attribute arguments will have a similar problem (I think the target needs to be marked).

[agocke]

@rolfbjarne Any idea why this just started failing now? Trying to gauge the need to have this in 6.0

[filipnavara]

@agocke It was tracked in the issues for quite a while as failed builds so it was likely always happening after the .NET 6 shift. It took time to track down the root cause though.

[agocke]

Is this a common scenario?

[filipnavara]

Now, I cannot answer that so hopefully @rolfbjarne will chime in. On the Xamarin tracking issue it's listed as P1, or "something we can't go to stable with".

The original issue was tracked as xamarin/xamarin-macios#12275, which split off from on-device tests.

[rolfbjarne]

@rolfbjarne Any idea why this just started failing now? Trying to gauge the need to have this in 6.0

It's been failing for a while, I just hadn't had time to track it down to a linker issue (my first guess before starting to investigate was that it was a problem with the test in question, so the priority wasn't as high as it should have been).

Is this a common scenario?

It only happens in one out of quite a few tests we have. It's a small test project, which makes sense from the investigation here - from the description it's probably more likely to run into this with a small project than a big project (because a big project is more likely to use all the intrinsic types and preserve the corresponding type forwarders).

I'm unsure how common it would be in the real world; after all most projects start out small, and it wouldn't be good if a lot of people ran into this after writing a little bit of code.

[agocke]

That's good enough -- unless it's a known corner case I'm comfortable taking it through, assuming we can get a fix ready relatively soon

[sbomer]

Fixed in #2276