cecil icon indicating copy to clipboard operation
cecil copied to clipboard

TypeReference etype not set properly during writing assembly attributes and method parameters (maybe more)...

Open TYoung86 opened this issue 7 years ago • 4 comments

For reference; https://github.com/TYoung86/cecil/commits/6efee375659087fb31e81334f3d223a1d8d42d08

etype was almost always ElementType.None for primitive type references (because nothing ever sets it), causing the assembly attributes to use non-standard references to system types in the core library (in this case, even referencing the System.Private.CoreLib), and almost always ElementType.None for type definitions unless the CustomAttributes property was read, invoking the MetadataReader that set the etype properly (thus this was hard to debug as hell because breakpoints would skip and a heisenbug would appear; one minute it's None, the next it's resolved to Void or I4 or whatever appropriately)

the conversion of etype to a property that looks up when not set specifically resolved it for me, but not sure if this is the right approach to resolve (thus no PR) other changes in the fork include port to new project files and making ParameterReference non-abstract (for no effective result); they'd need to be pulled out for a PR, and they actually have no impact on the output

I can provide before and after assemblies for compare if needed, but it's a pretty clear difference.. I might also try to fix the enum type references missing in written attributes later (they're written as primitives right now)...

TYoung86 avatar Jul 04 '17 00:07 TYoung86

I'm not quite sure where to start here.

Do you have a failing test case? An assembly that doesn't round trip properly?

jbevain avatar Jul 20 '17 00:07 jbevain

Yeah - I am at a state where the Vulkan.Binder project is starting to stabilize and work is getting calmer. I'll have some time to create a full test case soonish.

It is a bit rambly, I must have been pretty tired when I originally wrote this...

The platform was .NET Core 1.1, reading as a base and attempting to produce a .NET Standard 1.6 targeted assembly. The core library is System.Private.CoreLib; nothing should generate an AssemblyNameReference to it, so care was taken to refer to the type redirections in System.Runtime as needed.

I can write a demonstration test case showing an assembly being generated with a CustomAttribute that has a Class or ValueType ElementType describing one or more of it's primitive (e.g. an I4) constructor arguments. PEVerify /MD identifies the problem. It appears to be due to the etype being it's default value of None. Interactively debugging and inspecting the TypeReference causes the etype to become populated and the correct etype to be filled in (thus the heisenbug) and the resulting assembly to be produced with the correct ElementType.

Interactive inspection during debugging invokes the CustomAttributes property getter, which reads more metadata and correctly sets the etype on the TypeReference being inspected. There are very few places in which etype is assigned or read, so it was easy to identify where but not how this was occurring at the time, resulting in forking Mono.Cecil and rambling out this issue.

I'm not sure at the moment which of module.ImportReference(typeof(int)), module.TypeSystem.Int32, or a foreign imported primitive type reference produce this scenario, I will write a test for each.

It can manifest in both saving a new assembly and in saving an assembly that was read.

The version of Cecil used was based on master from 2017/06/15, commit 9d748473214628b4852038d9ac31ece918b3f1a7.

TYoung86 avatar Jul 20 '17 02:07 TYoung86

A test case would be great! Thank you!

jbevain avatar Jul 20 '17 02:07 jbevain

But yeah, I was debugging the other day and noticed that ElementType.None was used for «we don't know, probably a class» at different places, which isn't great. Might not be an easy fix.

jbevain avatar Jul 20 '17 03:07 jbevain