EventSource contains code which ILLink can't analyze

[vitek-karas]

The implementation of EventSource has a code paths that uses reflection APIs which are not analyzable by the linker, effectively making any usage of EventSource potentially linker-unsafe. This in turn means that applications using ILLink during publish may end up broken at runtime due to missing assemblies/types/members.

Manifest creation

EventSource.CreateManifestAndDescriptors uses reflection enumeration methods Type.GetMethods and Type.GetFields as well as Type.GetNestedType to build the manifest description of an event source. If any of these methods/fields/nested types is not referenced directly by the application code linker may decide to remove them, so the manifest produced for a trimmed app might be different from the untrimmed app.

Accessing attributes

Most of the event source code uses EventSource.GetCustomAttributeHelper to access attributes on types. The way this is implemented is not analyzable by the linker.

attributeType is not analyzable right now

It comes from a fixed set of typeof expressions in the call sites, so this should be relatively easy to fix. Those attribute types must be kept as attributes, instantiated and must have all properties. The method will eventually call CreateInstance on the type and also GetProperty and then PropertyInfo.SetValue.

Performs name based scan of attributes

The method takes the attributeType and does a name based matched to all attributes from the member. So there's no good way to tell the linker it needs to keep attributes of a certain name (in any assembly). That said it seems this is for reflection-only scenarios. It's unclear if this is necessary in .NET Core - runtime itself doesn't support reflection-only. Maybe MetadataLoadContext is considered reflection-only. What are the scenarios for this? Does it have to be an enabled code path for runtime event-tracing?

Caching reflection information about types

The entry point to this system seems to be Statics.CreateDefaultTypeInfo.
This caches lot of reflection information about a specified Type. Uses GetProperties and GetCustomAttributes. Effectively requires the type to have all properties and attributes kept.

Using reflection to get a value of a property

PropertyValue uses PropertyInfo.GetValue or the indirect PropertyInfo.GetGetMethod to access property value. In some cases linker may remove the getter from a property if it is not used directly by any code in the application.

Similarly InvokeTypeInfo uses PropertyInfo.GetValue on a "random" property.

NullableTypeInfo uses reflection

The implementation of NullableTypeInfo uses reflection calls to create instance of the property value. The code is not analyzable by the linker as the type in question comes as an input. It's possible that with some advanced annotations the code is safe enough, hard to tell. Is it possible to create arbitrary types with this code, or is it really only a limited set of types which is allowed?

Asking for size of an object

EventSource.DecodeObject calls Marshal.SizeOf on a System.Type which is not statically analyzable. Technically this is unsafe as linker can't guarantee that the SizeOf will return the same values before and after linking (linker may remove unused fields).

Maps creation for enum types

ManifestBuilder.CreateManifestString uses Type.GetFields to enumerate all values on an Enum type. This is not analyzable by the linker currently as there's no way for linker to tell that the types in question are always enums. The code is safe (as linker keeps all fields on enums by default), we just need a way to describe this to linker in some maintainable way.

Similarly ManifestBuilder.GetTypeName also special cases enums and calls Type.GetFields on them. Same situation as above; just need to find a way to annotate this correctly.

[MichalStrehovsky]

.NET Native would inject overrides of the GetMetadata method at the time of build. The body of the injected GetMetadata method would have all the information EventSource otherwise collects with reflection. I think the transform had some limitations though, so it was only enabled for event providers we ship with .NET Native.

[MichalStrehovsky]

As an aside, why is Marshal.SizeOf linker unsafe? This method only works for types that specify sequential/explicit layout. Linker should not touch instance fields on those (and I believe it doesn't).

[stephentoub]

What is the negative ramification of this? Let's say I have an event source with 5 events, but my app calls only one of them. Presumably then the linker removes the other 4 event methods, and the problem is that the generated manifest won't contain them. Is that the issue? And if so, since the events aren't getting raised anyway, what does that break?

Simply trying to understand the scope of the issue.

[vitek-karas]

@stephentoub I think that answering those questions is kind of the point of this issue. I don't fully understand what EventSource does and all the corner case features it supports, so it's hard to tell if the above mentioned places are potentially behavior breaking or not.

I do agree that in general it doesn't sound too bad. That said I know that for example it supports a kind of a serializer (WriteEvent(object)) which compined with the fact that linker may remove property getters can lead to breaks. Also the attribute handling might be problematic as linker is typically very aggressive removing attributes, and the code accesses them in a way which the linker doesn't understand.

[stephentoub]

I think that answering those questions is kind of the point of this issue

Ok, that wasn't clear to me. Thanks. Then this issue is a really a statement that some things might get removed, and we need to determine whether that's ok or not, and if not, then come up with a solution (but "do nothing because nothing bad happens" may also be the outcome).

[josalem]

CC - @tommcdon @sywhang @noahfalk @lateralusX

[vitek-karas]

Regarding Marshal.SizeOf - thanks for the feedback - I'll investigate if it is really safe. For now just ignore that part of the issue above.