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

ILStrip failure for FsToolkit.ErrorHandling(Must be of type CilStrip.Mono.Cecil.TypeReference) #87867

Open
rolfbjarne opened this issue Jun 21, 2023 · 12 comments

Comments

@rolfbjarne
Copy link
Member

From @NickDarvey on Wed, 21 Jun 2023 11:28:51 GMT

Steps to Reproduce

  1. Download FsToolkitiOSApp.zip (attached)
  2. Run
    dotnet build -c Release -t:Run -f net7.0-ios -p:RuntimeIdentifier=ios-arm64 -p:_DeviceName=1234 -bl
    

Expected Behavior

Release build completes.

Actual Behavior

/usr/local/share/dotnet/packs/Microsoft.iOS.Sdk/16.4.7067/targets/Xamarin.Shared.Sdk.targets(765,3): ILStrip failed for obj/Release/net7.0-ios/ios-arm64/linked/FsToolkit.ErrorHandling.dll: Must be of type CilStrip.Mono.Cecil.TypeReference [/FsToolkitiOSApp.fsproj]


System.ArgumentException: Must be of type CilStrip.Mono.Cecil.TypeReference
   at CilStrip.Mono.Cecil.GenericArgumentCollection.OnValidate(Object o)
   at System.Collections.CollectionBase.System.Collections.IList.Add(Object value)
   at CilStrip.Mono.Cecil.GenericArgumentCollection.Add(TypeReference value)
   at CilStrip.Mono.Cecil.ReflectionReader.GetTypeRefFromSig(SigType t, GenericContext context)
   at CilStrip.Mono.Cecil.ReflectionReader.GetGenericArg(GenericArg arg, GenericContext context)
   at CilStrip.Mono.Cecil.ReflectionReader.GetTypeRefFromSig(SigType t, GenericContext context)
   at CilStrip.Mono.Cecil.ReflectionReader.CompleteParameter(ParameterDefinition parameter, Param signature, GenericContext context)
   at CilStrip.Mono.Cecil.ReflectionReader.BuildParameterDefinition(String name, Int32 sequence, ParameterAttributes attrs, Param psig, GenericContext context)
   at CilStrip.Mono.Cecil.ReflectionReader.CompleteMethods()
   at CilStrip.Mono.Cecil.ReflectionReader.VisitTypeDefinitionCollection(TypeDefinitionCollection types)
   at CilStrip.Mono.Cecil.AggressiveReflectionReader.VisitTypeDefinitionCollection(TypeDefinitionCollection types)
   at CilStrip.Mono.Cecil.ReflectionReader.VisitModuleDefinition(ModuleDefinition mod)
   at CilStrip.Mono.Cecil.StructureReader.TerminateAssemblyDefinition(AssemblyDefinition asm)
   at CilStrip.Mono.Cecil.AssemblyDefinition.Accept(IReflectionStructureVisitor visitor)
   at CilStrip.Mono.Cecil.AssemblyFactory.GetAssembly(ImageReader irv, Boolean manifestOnly)
   at CilStrip.Mono.Cecil.AssemblyFactory.GetAssembly(ImageReader reader)
   at CilStrip.Mono.Cecil.AssemblyFactory.GetAssembly(String file)
   at ILStrip.StripAssembly(ITaskItem assemblyItem)
   

Environment

Version information
dotnet  7.0.304
dotnet workload maui-ios 7.0.86/7.0.100

Build Logs and Example Project

FsToolkitiOSApp.zip

Workaround

Disabling IL stripping, as described in xamarin/xamarin-macios#14841 (comment), works.

Is there a way to only disable it for this asssembly?

Copied from original issue xamarin/xamarin-macios#18485

@rolfbjarne
Copy link
Member Author

From @rolfbjarne on Wed, 21 Jun 2023 12:18:17 GMT

Smaller repro: testproj.zip

Download & execute:

$ dotnet build /t:RunILStrip
testproj.csproj(10,5): error : ILStrip failed for FsToolkit.ErrorHandling.dll: Must be of type CilStrip.Mono.Cecil.TypeReference

@rolfbjarne
Copy link
Member Author

From @rolfbjarne on Wed, 21 Jun 2023 12:18:34 GMT

Is there a way to only disable it for this asssembly?

It's rather ugly, but you can add something like this to your project file:

  <Target Name="_RemoveILStripTrigger" BeforeTargets="_StripAssemblyIL">
    <ItemGroup>
      <_RemovedResolvedFileToPublish Include="@(ResolvedFileToPublish)" Condition="'%(Filename)%(Extension)' == 'FsToolkit.ErrorHandling.dll'" />
      <ResolvedFileToPublish Remove="@(_RemovedResolvedFileToPublish)" />
    </ItemGroup>
  </Target>

  <Target Name="_RestoreILStripTrigger" AfterTargets="_StripAssemblyIL">
    <ItemGroup>
      <ResolvedFileToPublish Include="@(_RemovedResolvedFileToPublish)" />
    </ItemGroup>
  </Target>

@rolfbjarne
Copy link
Member Author

From @rolfbjarne on Wed, 21 Jun 2023 12:18:58 GMT

Moving to dotnet/runtime, since the ILStrip implementation lives there.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 21, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 21, 2023
@jeffschwMSFT jeffschwMSFT added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Jun 22, 2023
@ghost
Copy link

ghost commented Jun 22, 2023

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

From @NickDarvey on Wed, 21 Jun 2023 11:28:51 GMT

Steps to Reproduce

  1. Download FsToolkitiOSApp.zip (attached)
  2. Run
    dotnet build -c Release -t:Run -f net7.0-ios -p:RuntimeIdentifier=ios-arm64 -p:_DeviceName=1234 -bl
    

Expected Behavior

Release build completes.

Actual Behavior

/usr/local/share/dotnet/packs/Microsoft.iOS.Sdk/16.4.7067/targets/Xamarin.Shared.Sdk.targets(765,3): ILStrip failed for obj/Release/net7.0-ios/ios-arm64/linked/FsToolkit.ErrorHandling.dll: Must be of type CilStrip.Mono.Cecil.TypeReference [/FsToolkitiOSApp.fsproj]


System.ArgumentException: Must be of type CilStrip.Mono.Cecil.TypeReference
   at CilStrip.Mono.Cecil.GenericArgumentCollection.OnValidate(Object o)
   at System.Collections.CollectionBase.System.Collections.IList.Add(Object value)
   at CilStrip.Mono.Cecil.GenericArgumentCollection.Add(TypeReference value)
   at CilStrip.Mono.Cecil.ReflectionReader.GetTypeRefFromSig(SigType t, GenericContext context)
   at CilStrip.Mono.Cecil.ReflectionReader.GetGenericArg(GenericArg arg, GenericContext context)
   at CilStrip.Mono.Cecil.ReflectionReader.GetTypeRefFromSig(SigType t, GenericContext context)
   at CilStrip.Mono.Cecil.ReflectionReader.CompleteParameter(ParameterDefinition parameter, Param signature, GenericContext context)
   at CilStrip.Mono.Cecil.ReflectionReader.BuildParameterDefinition(String name, Int32 sequence, ParameterAttributes attrs, Param psig, GenericContext context)
   at CilStrip.Mono.Cecil.ReflectionReader.CompleteMethods()
   at CilStrip.Mono.Cecil.ReflectionReader.VisitTypeDefinitionCollection(TypeDefinitionCollection types)
   at CilStrip.Mono.Cecil.AggressiveReflectionReader.VisitTypeDefinitionCollection(TypeDefinitionCollection types)
   at CilStrip.Mono.Cecil.ReflectionReader.VisitModuleDefinition(ModuleDefinition mod)
   at CilStrip.Mono.Cecil.StructureReader.TerminateAssemblyDefinition(AssemblyDefinition asm)
   at CilStrip.Mono.Cecil.AssemblyDefinition.Accept(IReflectionStructureVisitor visitor)
   at CilStrip.Mono.Cecil.AssemblyFactory.GetAssembly(ImageReader irv, Boolean manifestOnly)
   at CilStrip.Mono.Cecil.AssemblyFactory.GetAssembly(ImageReader reader)
   at CilStrip.Mono.Cecil.AssemblyFactory.GetAssembly(String file)
   at ILStrip.StripAssembly(ITaskItem assemblyItem)
   

Environment

Version information
dotnet  7.0.304
dotnet workload maui-ios 7.0.86/7.0.100

Build Logs and Example Project

FsToolkitiOSApp.zip

Workaround

Disabling IL stripping, as described in xamarin/xamarin-macios#14841 (comment), works.

Is there a way to only disable it for this asssembly?

Copied from original issue xamarin/xamarin-macios#18485

Author: rolfbjarne
Assignees: -
Labels:

untriaged, area-Tools-ILLink, needs-area-label

Milestone: -

@vcsjones vcsjones removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 23, 2023
@MichalStrehovsky MichalStrehovsky added area-Codegen-AOT-mono and removed area-Tools-ILLink .NET linker development as well as trimming analyzers labels Jun 23, 2023
@marek-safar marek-safar removed the untriaged New issue has not been triaged by the area owner label Jul 21, 2023
@marek-safar marek-safar added this to the 8.0.0 milestone Jul 21, 2023
@SamMonoRT
Copy link
Member

cc @fanyang-mono

@jandupej
Copy link
Member

I can replicate the issue both with the current ILStrip and one from net6.0.

@jandupej
Copy link
Member

Moving to net9.0, since there is a workaround.

@jandupej jandupej modified the milestones: 8.0.0, 9.0.0 Aug 14, 2023
@jandupej
Copy link
Member

When opening the assembly to be trimmed, Cecil chokes on the following method:

.method public static 
	valuetype [FSharp.Core]Microsoft.FSharp.Core.FSharpValueOption`1<!!'value'> tryParse$W<'value'> (
		class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<string, class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<!!'value'&, bool>> tryParse,
		string valueToParse
	) cil managed
{...}

Note the 'value'& in the inner FSharpFunc'2, this results in ReflectionReader.GetTypeRefFromSig to return null, because it has no case for ElementType.ByRef. Certain validations fail and build crashes.

@xperiandri
Copy link

xperiandri commented Aug 28, 2024

This is our repro https://github.com/Ecierge/FsToolkit.ErrorHandling_AOT_repro on .NET 8

@vitek-karas
Copy link
Member

This makes me sad...

The ILStrip task uses a private copy of Mono.Cecil because it seems to need to set metadata token values (which mainline Cecil doesn't support). This private copy is maintained in dotnet/runtime-assets and was added with dotnet/runtime-assets#160. But it's really just a copy from mono/mono which was added with mono/mono@29e9001. That is a 14 years old commit - there's no way to tell how old the source was when the copy was made, but at least 14 years.

The problem:

  • We've been making changes to IL since, some of which needed Mono.Cecil updates so that it can correctly parse the input and write it back out. So, there are likely cases where ILStrip corrupts the content to some degree.
  • Any fixes made to Cecil in the last 14 years are not there. It's very possible that the above issue doesn't exist in current Cecil.

The thing which breaks the 14 year old Cecil is a ref type in a generic parameter - not surprising as ref types didn't exist 14 years ago really - specifically this: https://github.com/demystifyfp/FsToolkit.ErrorHandling/blob/fd9460f0bfa9b670d7ef95ba48ec274a5146871c/src/FsToolkit.ErrorHandling/ValueOption.fs#L44

Quick look at the usage of Cecil says that this is not going to be easy to fix as Cecil changed a LOT since then (the public APIs used
to just load assembly doesn't exist anymore), so it's not just the ability to manipulate tokens which is different.

The "fix" for this whole situation is probably tracked by #60273.

Honestly, it's surprising this is not a more common problem.

/cc @steveisok

@vitek-karas
Copy link
Member

Also cc @agocke, @sbomer as sort of owners of Cecil's usage in runtime. See the comment above for context.

@xperiandri
Copy link

Any updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests