Remove ILLink support for field init substitutions

[sbomer]

Fixes #107379.

Uses of this were very limited: https://github.com/search?q=path%3AILLink.Substitutions.xml%20initialize%3D%22true%22&type=code.

Would this need a breaking change doc?

[jtschuster]

Why are these methods removed now / why weren't they before?

[sbomer]

Without the static ctor, the type is now BeforeFieldInit, which lets UnreachableBlocksOptimizer treat the method calls as side-effect free and inline them.

[vitek-karas]

This is effectively a silent break - I would prefer we report a warning.
Pros - we don't need to "Educate" anyone on this really - the tooling will do the education for us
Cons - code to keep around and delete a release or two later (and the test)

[sbomer]

How about we throw on this code path like native AOT?

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/BodySubstitutionParser.cs

Lines 145 to 153 in 66e45ef

	if (string.Equals(GetAttribute(fieldNav, "initialize"), "true", StringComparison.InvariantCultureIgnoreCase))
	{
	// We would need to also mess with the cctor of the type to set the field to this value:
	//
	// * ILLink will remove all stsfld instructions referencing this field from the cctor
	// * It will place an explicit stsfld in front of the last "ret" instruction in the cctor
	//
	// This approach... has issues.
	throw new NotSupportedException();

That avoids having to keep around the machinery and tests for the new warning, which seems a bit much for a feature that is barely used and not really considered supported in the first place.

[vitek-karas]

That works - warning is a bit more "pleasant" (can be ignored), hard crash means that the code MUST be fixed, so if this comes from a NuGet I'm screwed. But given the rarity of this, it may be fine.

[MichalStrehovsky]

Uses of this were very limited: https://github.com/search?q=path%3AILLink.Substitutions.xml%20initialize%3D%22true%22&type=code.

Would this need a breaking change doc?

@tannergooding what would it take to remove the use of this in your repos? Looks like you're the only user of this on Github.

If I remember correctly, this is a for a feature that wasn't even needed in the end (with terrafx/terrafx.interop.windows#343, one shouldn't need to set feature switches to get size savings).

[tannergooding]

@MichalStrehovsky sorry I missed the ping here.

Are you specifically referring to removing the <field name="Name" value="..." /> entries from ILLink.Substitutions.xml?

If so, I think I'd really just want some definitive code pattern for dealing with static readonly T Name = ...; such that the linker can statically know the value of Name (at least for any primitive T).

While the use of ILLink.Substitutions.xml might be rare, it isn't so rare for code to rely on the ability for the JIT to optimize static readonly T Name = ... to being a constant. We do this all over in the libraries code and various perf oriented frameworks/libraries do the same. NAOT doesn't necessarily have this same capability, particularly not for "non-trivial" code. For example, one might have private static readonly int DefaultAlignment = GetAppContextData(nameof(DefaultAlignment), defaultValue: 16); which then uses AppContext.GetData(...), may support that data being int or a string it tries to parse, may fallback to checking environment variables, etc. It is very desirable to have the ability to have a NAOT compilation where this is actually a constant instead and so some mechanism to indicate it should be treated as such is desirable (I don't personally care what that mechanism is, provided some mechanism exists).

[MichalStrehovsky]

Are you specifically referring to removing the <field name="Name" value="..." /> entries from ILLink.Substitutions.xml?

Yep. We don't really substitute fields elsewhere, we substitute properties. ILLinker has support for the hacky <field name=... initialize=true> that we inherited from Xamarin, but the problem with fields is that we can't properly intercept places that read the field using reflection. Reading the field with code is fine, we can easily rewrite ldsfld into a ldc. But reflection is hard. So ILLinker has this weird piece of logic that just looks for the last ret in the type's static constructor and slams a stsfld in there to initialize the field (can't be the first instruction because the field might have some other default set later in the cctor). This has problems because we have no guarantee it will execute (cctor can just exit earlier, etc.). So we'd just like to get rid of all that. Looks like s_disableResolveLibraryHook is the only use of this in all of github. If it can be switched to properties, it will avoid getting broken.

[tannergooding]

Looks like s_disableResolveLibraryHook is the only use of this in all of github. If it can be switched to properties, it will avoid getting broken.

I can switch it to a property fairly easily. The main reason it wasn't is because such calls used to be opaque to the JIT and would impact inlining. Even now they still eat little bits of the budget and can have negative side effects in bulk.

But reflection is hard.

For sure. Just wondering though, is this perhaps something that would be fine to leave as undefined behavior? That is, just state that using the substituted fields with reflection is unsupported. The places I'm using it aren't using (and have no desire to use) reflection with such fields, as it kind-of defeats their purpose 😄.

If we could still support this and just leave reflecting over substituted fields as UB, then it mostly avoids the issue and doesn't force a lot of extra calls to exist for the JIT scenario that then have to be handled by the inliner and its budget.

-- Definitely understand if this still isn't desired, just thought I'd ask since it would keep a nice feature still efficient for that scenario.

[MichalStrehovsky]

If we could still support this and just leave reflecting over substituted fields as UB, then it mostly avoids the issue and doesn't force a lot of extra calls to exist for the JIT scenario that then have to be handled by the inliner and its budget.

Oh so this PR only removes the support for the initialize=true part of this. Terrafx/clangsharp is opting into it but maybe it doesn't have to set this attribute? It's specifically enables the sketchy logic where we try to actually set the field to this value. Support for field substitution within method bodies would stay - with the gotcha that you'll never see the substituted value outside of code that ILLinker saw (so reflection, reflection.emitted bodies, etc will see an inconsistent world).

[tannergooding]

Gotcha, so this just requires removal of initialize="true" not the field substitution itself, the field will still be substituted, and the field can potentially be trimmed after substitution since its not marked as initialize=true?

That's all good with me and I can get the repo's updated to drop that. I likely only added initialize=true because I thought it was required or similar, and I misunderstood the conversation above to be dropping field substitution altogether.

[MichalStrehovsky]

Gotcha, so this just requires removal of initialize="true" not the field substitution itself, the field will still be substituted, and the field can potentially be trimmed after substitution since its not marked as initialize=true?

Yes, that's the idea.