Closed eerhardt closed 2 months ago
The --ignore-substitutions
option claims to skip reading embedded substitutions:
https://github.com/dotnet/linker/blob/4b3f78cbc7284b4198652a695e9fe0267133728e/src/linker/Linker/Driver.cs#L1367
However, I think that setting also skips reading command-line substitutions: https://github.com/dotnet/linker/blob/4f0d349e43a71d44ef71339293701fcfc2da997e/src/linker/Linker.Steps/BodySubstitutionParser.cs#L32
If we fix this, I think it would make sense to:
--ignore-substitutions
during the corelib build--substitutions
Set --ignore-substitutions during the corelib build
Could it make sense to flip this around and have an "opt out" list of substitutions on the command line? Basically all the substitutions that happen during the build today of CoreLib should be respected at build time. There are just a handful (at this point - only one) that should be opted out of.
I would argue that you don't need the embedded ones if you have the same on command line - because trimming will bake the value into and it will turn that property into return const
- and constant prop should be able to figure that out during app build. But I don't want to depend on that behavior - at least not yet.
I don't have a strong opinion either way. It would be easier to do what @sbomer suggest on the linker side, but would require more work in runtime build. And the other way round.
How much work would it be to add a command line option like --ignore-featuredefault <feature name>
? What it would mean would be for this run of the linker to not respect the featuredefault
setting for that feature
in any of the ILLink.*.xml
files.
Then everything else stays the same. We don't need to mess with any of the existing unconditional substitutions that are happening (which is actually kind of complicated because of all the ways we build CoreLib - mono/coreclr/nativeaot x OS x architecture).
We could also ignore all embedded features switches in 'library
mode.
Maybe it's time to revisit the "library build" in runtime? is it still worth it?
is it still worth it?
IMO the value is in the following places:
Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas See info in area-owners.md if you want to be subscribed.
Author: | eerhardt |
---|---|
Assignees: | - |
Labels: | `untriaged`, `area-Tools-ILLink`, `needs-area-label` |
Milestone: | - |
Per discussion at https://github.com/dotnet/runtime/pull/103463#discussion_r1640328125, I think a better way to accomplish this is to avoid using featuredefault
for substitutions, and to instead place defaults into the MSBuild targets that are applied only at app publish time.
That makes sense to me. Not sure why that wasn't considered when I opened this issue. If we can make the MSBuild targets work for https://github.com/dotnet/runtime/issues/80398, then we can close this issue.
In https://github.com/dotnet/runtime/pull/80246, we are adding support for RuntimeFeature.IsDynamicCodeSupported to be a feature switch, instead of being hard-coded to
true
for CoreCLR. This allows developers to test their code simulating it running with NativeAOT, without actually needing to publish for NativeAOT.As part of adding this feature switch, I don't want to regress app sizes for existing apps. Since CoreCLR currently hard-codes
RuntimeFeature.IsDynamicCodeSupported
totrue
. So when you trim a CoreCLR app with code like the following:The
ReflectionMemberAccessor
class can be trimmed, sinceIsDynamicCodeSupported
will always betrue
.To do this, I wanted to use a
featuredefault
option in myILLink.Substitutions.xml
. This would tell the trimmer that if no one specifies the RuntimeFeature.IsDynamicCodeSupported feature switch, assume it istrue
.However, since we run the trimmer during the build of System.Private.CoreLib, this
featuredefault
is getting applied during the "library build" time. Which means the property is getting hard-coded totrue
at the build time, and the code in the RuntimeFeature.IsDynamicCodeSupported property is getting removed.We should figure out a way where a library can embed a
featuredefault
ILLink.Substitutions.xml element, but only have it apply during "app publish" time.cc @sbomer @vitek-karas