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.

Sign up for GitHub

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

Jump to bottom

[wasm] ILLinker crash System.NotImplementedException: Var in System.Reflection.Metadata tests #2963

Closed
radical opened this issue Aug 10, 2022 · 17 comments · Fixed by #2980
Closed

[wasm] ILLinker crash System.NotImplementedException: Var in System.Reflection.Metadata tests #2963

radical opened this issue Aug 10, 2022 · 17 comments · Fixed by #2980
Assignees
@sbomer
Milestone
7.0.x

Comments

@radical
Copy link
Member

radical commented Aug 10, 2022

Rolling build with /p:EnableAggressiveTrimming=true:

ILLink : error IL1012: IL Trimmer has encountered an unexpected error. Please report the issue at https://github.com/dotnet/linker/issues [/__w/1/s/src/libraries/System.Reflection.Metadata/tests/System.Reflection.Metadata.Tests.csproj]
##[error]ILLink(0,0): error IL1012: (NETCORE_ENGINEERING_TELEMETRY=Build) IL Trimmer has encountered an unexpected error. Please report the issue at https://github.com/dotnet/linker/issues
  Fatal error in IL Linker
  Unhandled exception. System.NotImplementedException: Var
     at Mono.Cecil.SignatureReader.ReadPrimitiveValue(ElementType type)
     at Mono.Cecil.SignatureReader.ReadCustomAttributeElementValue(TypeReference type)
     at Mono.Cecil.SignatureReader.ReadCustomAttributeElement(TypeReference type)
     at Mono.Cecil.SignatureReader.ReadCustomAttributeFixedArgument(TypeReference type)
     at Mono.Cecil.SignatureReader.ReadCustomAttributeConstructorArguments(CustomAttribute attribute, Collection`1 parameters)
     at Mono.Cecil.MetadataReader.ReadCustomAttributeSignature(CustomAttribute attribute)
     at Mono.Cecil.CustomAttribute.<Resolve>b__35_0(CustomAttribute attribute, MetadataReader reader)
     at Mono.Cecil.ModuleDefinition.Read[TItem](TItem item, Action`2 read)
     at Mono.Cecil.CustomAttribute.Resolve()
     at Mono.Cecil.CustomAttribute.get_HasConstructorArguments()
     at Mono.Linker.Steps.MarkStep.MarkCustomAttributeArguments(CustomAttribute ca)
     at Mono.Linker.Steps.MarkStep.MarkCustomAttribute(CustomAttribute ca, DependencyInfo& reason)
     at Mono.Linker.Steps.MarkStep.MarkCustomAttributes(ICustomAttributeProvider provider, DependencyInfo& reason)
     at Mono.Linker.Steps.MarkStep.MarkEntireType(TypeDefinition type, DependencyInfo& reason)
     at Mono.Linker.Steps.MarkStep.MarkEntireType(TypeDefinition type, DependencyInfo& reason)
     at Mono.Linker.Steps.MarkStep.MarkEntireAssembly(AssemblyDefinition assembly)
     at Mono.Linker.Steps.MarkStep.MarkAssembly(AssemblyDefinition assembly, DependencyInfo reason)
     at Mono.Linker.Steps.MarkStep.MarkModule(ModuleDefinition module, DependencyInfo reason)
     at Mono.Linker.Steps.MarkStep.ProcessMarkedPending()
     at Mono.Linker.Steps.MarkStep.Initialize()
     at Mono.Linker.Steps.MarkStep.Process(LinkContext context)
     at Mono.Linker.Pipeline.ProcessStep(LinkContext context, IStep step)
     at Mono.Linker.Pipeline.Process(LinkContext context)
     at Mono.Linker.Driver.Run(ILogger customLogger)
     at Mono.Linker.Driver.Main(String[] args)

Changes since the last rolling build: 063f7b7...4211dc4 .
The only commit with changes to SRM that I can see - b947dd6 .

commit b947dd6e1bb45b064e30159c9251add5c80914af
Author: Buyaa Namnan <[email protected]>
Date:   Tue Aug 9 09:09:49 2022 -0700

    Generic attributes handling in CustomAttributeDecoder (#72561)

    * Add handling of generic attributes to CustomAttributeDecoder

    If the attribute constructor refers to the generic T in its signature, we would throw `BadImageFormatException`. This adds h
andling by capturing the generic context and interpreting the signature variables when needed.

cc @eerhardt @buyaa-n @vitek-karas
@ghost
Copy link

ghost commented Aug 10, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Rolling build with /p:EnableAggressiveTrimming=true:

ILLink : error IL1012: IL Trimmer has encountered an unexpected error. Please report the issue at https://github.com/dotnet/linker/issues [/__w/1/s/src/libraries/System.Reflection.Metadata/tests/System.Reflection.Metadata.Tests.csproj]
##[error]ILLink(0,0): error IL1012: (NETCORE_ENGINEERING_TELEMETRY=Build) IL Trimmer has encountered an unexpected error. Please report the issue at https://github.com/dotnet/linker/issues
  Fatal error in IL Linker
  Unhandled exception. System.NotImplementedException: Var
     at Mono.Cecil.SignatureReader.ReadPrimitiveValue(ElementType type)
     at Mono.Cecil.SignatureReader.ReadCustomAttributeElementValue(TypeReference type)
     at Mono.Cecil.SignatureReader.ReadCustomAttributeElement(TypeReference type)
     at Mono.Cecil.SignatureReader.ReadCustomAttributeFixedArgument(TypeReference type)
     at Mono.Cecil.SignatureReader.ReadCustomAttributeConstructorArguments(CustomAttribute attribute, Collection`1 parameters)
     at Mono.Cecil.MetadataReader.ReadCustomAttributeSignature(CustomAttribute attribute)
     at Mono.Cecil.CustomAttribute.<Resolve>b__35_0(CustomAttribute attribute, MetadataReader reader)
     at Mono.Cecil.ModuleDefinition.Read[TItem](TItem item, Action`2 read)
     at Mono.Cecil.CustomAttribute.Resolve()
     at Mono.Cecil.CustomAttribute.get_HasConstructorArguments()
     at Mono.Linker.Steps.MarkStep.MarkCustomAttributeArguments(CustomAttribute ca)
     at Mono.Linker.Steps.MarkStep.MarkCustomAttribute(CustomAttribute ca, DependencyInfo& reason)
     at Mono.Linker.Steps.MarkStep.MarkCustomAttributes(ICustomAttributeProvider provider, DependencyInfo& reason)
     at Mono.Linker.Steps.MarkStep.MarkEntireType(TypeDefinition type, DependencyInfo& reason)
     at Mono.Linker.Steps.MarkStep.MarkEntireType(TypeDefinition type, DependencyInfo& reason)
     at Mono.Linker.Steps.MarkStep.MarkEntireAssembly(AssemblyDefinition assembly)
     at Mono.Linker.Steps.MarkStep.MarkAssembly(AssemblyDefinition assembly, DependencyInfo reason)
     at Mono.Linker.Steps.MarkStep.MarkModule(ModuleDefinition module, DependencyInfo reason)
     at Mono.Linker.Steps.MarkStep.ProcessMarkedPending()
     at Mono.Linker.Steps.MarkStep.Initialize()
     at Mono.Linker.Steps.MarkStep.Process(LinkContext context)
     at Mono.Linker.Pipeline.ProcessStep(LinkContext context, IStep step)
     at Mono.Linker.Pipeline.Process(LinkContext context)
     at Mono.Linker.Driver.Run(ILogger customLogger)
     at Mono.Linker.Driver.Main(String[] args)

Changes since the last rolling build: 063f7b7...4211dc4 .
The only commit with changes to SRM that I can see - b947dd6 .

commit b947dd6e1bb45b064e30159c9251add5c80914af
Author: Buyaa Namnan <[email protected]>
Date:   Tue Aug 9 09:09:49 2022 -0700

    Generic attributes handling in CustomAttributeDecoder (#72561)

    * Add handling of generic attributes to CustomAttributeDecoder

    If the attribute constructor refers to the generic T in its signature, we would throw `BadImageFormatException`. This adds h
andling by capturing the generic context and interpreting the signature variables when needed.

cc @eerhardt @buyaa-n @vitek-karas

Author: radical
Assignees: -
Labels:

arch-wasm, blocking-clean-ci, area-Build-mono, linkable-framework
Milestone: -

@ghost ghost added the untriaged label Aug 10, 2022
@ghost
Copy link

ghost commented Aug 10, 2022

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr
See info in area-owners.md if you want to be subscribed.

Issue Details

Rolling build with /p:EnableAggressiveTrimming=true:

ILLink : error IL1012: IL Trimmer has encountered an unexpected error. Please report the issue at https://github.com/dotnet/linker/issues [/__w/1/s/src/libraries/System.Reflection.Metadata/tests/System.Reflection.Metadata.Tests.csproj]
##[error]ILLink(0,0): error IL1012: (NETCORE_ENGINEERING_TELEMETRY=Build) IL Trimmer has encountered an unexpected error. Please report the issue at https://github.com/dotnet/linker/issues
  Fatal error in IL Linker
  Unhandled exception. System.NotImplementedException: Var
     at Mono.Cecil.SignatureReader.ReadPrimitiveValue(ElementType type)
     at Mono.Cecil.SignatureReader.ReadCustomAttributeElementValue(TypeReference type)
     at Mono.Cecil.SignatureReader.ReadCustomAttributeElement(TypeReference type)
     at Mono.Cecil.SignatureReader.ReadCustomAttributeFixedArgument(TypeReference type)
     at Mono.Cecil.SignatureReader.ReadCustomAttributeConstructorArguments(CustomAttribute attribute, Collection`1 parameters)
     at Mono.Cecil.MetadataReader.ReadCustomAttributeSignature(CustomAttribute attribute)
     at Mono.Cecil.CustomAttribute.<Resolve>b__35_0(CustomAttribute attribute, MetadataReader reader)
     at Mono.Cecil.ModuleDefinition.Read[TItem](TItem item, Action`2 read)
     at Mono.Cecil.CustomAttribute.Resolve()
     at Mono.Cecil.CustomAttribute.get_HasConstructorArguments()
     at Mono.Linker.Steps.MarkStep.MarkCustomAttributeArguments(CustomAttribute ca)
     at Mono.Linker.Steps.MarkStep.MarkCustomAttribute(CustomAttribute ca, DependencyInfo& reason)
     at Mono.Linker.Steps.MarkStep.MarkCustomAttributes(ICustomAttributeProvider provider, DependencyInfo& reason)
     at Mono.Linker.Steps.MarkStep.MarkEntireType(TypeDefinition type, DependencyInfo& reason)
     at Mono.Linker.Steps.MarkStep.MarkEntireType(TypeDefinition type, DependencyInfo& reason)
     at Mono.Linker.Steps.MarkStep.MarkEntireAssembly(AssemblyDefinition assembly)
     at Mono.Linker.Steps.MarkStep.MarkAssembly(AssemblyDefinition assembly, DependencyInfo reason)
     at Mono.Linker.Steps.MarkStep.MarkModule(ModuleDefinition module, DependencyInfo reason)
     at Mono.Linker.Steps.MarkStep.ProcessMarkedPending()
     at Mono.Linker.Steps.MarkStep.Initialize()
     at Mono.Linker.Steps.MarkStep.Process(LinkContext context)
     at Mono.Linker.Pipeline.ProcessStep(LinkContext context, IStep step)
     at Mono.Linker.Pipeline.Process(LinkContext context)
     at Mono.Linker.Driver.Run(ILogger customLogger)
     at Mono.Linker.Driver.Main(String[] args)

Changes since the last rolling build: 063f7b7...4211dc4 .
The only commit with changes to SRM that I can see - b947dd6 .

commit b947dd6e1bb45b064e30159c9251add5c80914af
Author: Buyaa Namnan <[email protected]>
Date:   Tue Aug 9 09:09:49 2022 -0700

    Generic attributes handling in CustomAttributeDecoder (#72561)

    * Add handling of generic attributes to CustomAttributeDecoder

    If the attribute constructor refers to the generic T in its signature, we would throw `BadImageFormatException`. This adds h
andling by capturing the generic context and interpreting the signature variables when needed.

cc @eerhardt @buyaa-n @vitek-karas

Author: radical
Assignees: -
Labels:

arch-wasm, blocking-clean-ci, area-Build-mono, linkable-framework
Milestone: -

@ghost ghost removed the untriaged label Aug 10, 2022
@radical radical mentioned this issue Aug 10, 2022
Merged
@buyaa-n
Copy link
Member

buyaa-n commented Aug 10, 2022

I guess its better to revert my PR to unblock dotnet/runtime#73410

@radical
Copy link
Member Author

radical commented Aug 10, 2022

I'm not sure about dotnet/runtime#73410 having this, but this is breaking scheduled rolling builds on main.

@MichalStrehovsky
Copy link
Member

This is a Cecil bug. Apparently, it also doesn't support generic attributes that Roslyn is introducing support for.

It will probably take a while to fix that because it's in Cecil, and that needs to propagate to the linker repo and that needs to propagate to the runtime repo.

The System.Reflection.Metadata support for generic attributes is critical for 7.0 (both for our customers and for our own tools that depend on S.R.Metadata, such as the NativeAOT compiler). @buyaa-n Once the revert is done, would it be possible to re-submit just the product change of dotnet/runtime#72561 without the test changes so that we have it in main again soon?

@naricc naricc mentioned this issue Aug 10, 2022
Merged
@buyaa-n
Copy link
Member

buyaa-n commented Aug 10, 2022

@buyaa-n Once the revert is done, would it be possible to re-submit just the product change of dotnet/runtime#72561 without the test changes so that we have it in main again soon?

Yes we can as tests are passed previously, also maybe create an issue and add active issue to the new tests instead of removing them

but this is breaking scheduled rolling builds on main.

@radical I am not familiar with what rolling builds does, is there a way to conditionally disable/ifdef the new tests from that build? I removing or commenting those tests is only option?

@radical
Copy link
Member Author

radical commented Aug 10, 2022

Rolling builds are scheduled builds that run twice a day. In this case they are running against 'main' with no other changes.

The revert should be enough for them.

@MichalStrehovsky
Copy link
Member

The problem is while building the tests, so ActiveIssue won't cut it.

We could alternatively disable building the tests on WASM (with a link to this issue) by adding S.R.Metadata tests somewhere around here:

https://github.com/dotnet/runtime/blob/a3fb0d383adf98ee2fb2ab816c28735dc1caaba0/src/libraries/tests.proj#L345-L374

Basically, I want to make sure the S.R.Metadata change ships in 7.0 irrespective of whether the illinker fix makes it. The Roslyn feature is shipping.

@radical
Copy link
Member Author

radical commented Aug 10, 2022

I don't know the details, but can we disable only the specific individual tests for wasm, so they are not included in the test suite at all, instead of disabling tests for the entire library?

@buyaa-n
Copy link
Member

buyaa-n commented Aug 10, 2022

The problem is while building the tests, so ActiveIssue won't cut it.

Right, looked into the build log, I see that now. I will try to ifdef the generic attributes part (will run wasm build on my new PR)

@MichalStrehovsky please take a look at this log: https://dev.azure.com/dnceng/public/_build/results?buildId=1932703&view=logs&j=d9366717-4230-52c6-0a51-0b43431c29c7&t=c627612c-ea4b-5282-5e42-8a39063fd2d8. It throw BadImageFormatException on windows x64 Release NativeAOT_Libs_Passing and Linux x64 Release NativeAOT_Libs_Passing builds, I suspect that is not expected

@MichalStrehovsky
Copy link
Member

I don't know the details, but can we disable only the specific individual tests for wasm, so they are not included in the test suite at all, instead of disabling tests for the entire library?

illinker chokes as soon as it sees this:

https://github.com/dotnet/runtime/blob/a3fb0d383adf98ee2fb2ab816c28735dc1caaba0/src/libraries/System.Reflection.Metadata/tests/Metadata/Decoding/CustomAttributeDecoderTests.cs#L302-L310

We would need to ifdef it out (and the corresponding test) on browser. I don't know if we can per-platform ifdef things for libraries that don't have a WASM-specific version (this all targets net7.0, not net7.0-windows/net7.0-browser, etc.). Probably not.

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky please take a look at this log: https://dev.azure.com/dnceng/public/_build/results?buildId=1932703&view=logs&j=d9366717-4230-52c6-0a51-0b43431c29c7&t=c627612c-ea4b-5282-5e42-8a39063fd2d8. It throw BadImageFormatException on windows x64 Release NativeAOT_Libs_Passing and Linux x64 Release NativeAOT_Libs_Passing builds, I suspect that is not expected

Right, it needs follow up work now that NativeAOT can parse these attributes. That's why it's critical for this change to get in :). I cannot do the follow up work without the S.R.Metadata change.

@radical
Copy link
Member Author

radical commented Aug 10, 2022

This is just a test though. We can ifdef tests.

@runfoapp runfoapp bot mentioned this issue Aug 10, 2022
Closed
@radical
Copy link
Member Author

radical commented Aug 10, 2022

The change was reverted, and we no longer have the crash. Closing this.

@radical radical closed this as completed Aug 10, 2022
@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Aug 10, 2022 •
edited
Loading

The change was reverted, and we no longer have the crash. Closing this.

Is there an issue tracking the Cecil bug?

@radical
Copy link
Member Author

radical commented Aug 10, 2022

The change was reverted, and we no longer have the crash. Closing this.

Is there an issue tracking the Cecil bug?

Not that I know of.

@LakshanF LakshanF mentioned this issue Aug 10, 2022
Merged
@MichalStrehovsky
Copy link
Member

Not that I know of.

Reopening then and throwing it over the fence to the linker repo.

@MichalStrehovsky MichalStrehovsky transferred this issue from dotnet/runtime Aug 10, 2022
@MichalStrehovsky MichalStrehovsky mentioned this issue Aug 10, 2022
Closed
This was referenced Aug 17, 2022
Merged
Merged
Merged
@sbomer sbomer added this to the .NET 7.0 milestone Aug 23, 2022
@sbomer sbomer self-assigned this Aug 23, 2022
@sbomer sbomer mentioned this issue Aug 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sbomer sbomer

Labels
None yet
Projects
AppModel
Archived in project
Milestone
7.0.x
Development

Successfully merging a pull request may close this issue.

Add support for generic attributes sbomer/linker
4 participants
@radical @sbomer @MichalStrehovsky @buyaa-n