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

Remove ILLink support for field init substitutions #107380

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Remove ILLink support for field init substitutions #107380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
12ec32f
Remove ILLink support for field init substitutions
sbomer Sep 4, 2024
6178320
Remove unnecessary initialize="false"
sbomer Sep 4, 2024
a766c27
Throw instead
sbomer Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 0 additions & 14 deletions docs/tools/illink/data-formats.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,20 +253,6 @@ Entire method body is replaces with `throw` instruction when method is reference
</linker>
```

### Override static field value with a constant

The `initialize` attribute is optional and when not specified the code to set the static field to the value will not be generated.

```xml
<linker>
<assembly fullname="Assembly">
<type fullname="Assembly.A">
<field name="MyNumericField" value="5" initialize="true" />
</type>
</assembly>
</linker>
```

### Remove embedded resources

```xml
Expand Down
8 changes: 2 additions & 6 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/BodySubstitutionParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,12 +144,8 @@ protected override void ProcessField(TypeDesc type, XPathNavigator fieldNav)

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.
// We would need to also mess with the cctor of the type to set the field to this value,
// and doing so correctly is difficult.
throw new NotSupportedException();
}

Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
<method signature="System.Boolean InitializeBinaryFormatter()" body="stub" value="false" feature="System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization" featurevalue="false" />
</type>
<type fullname="System.Threading.Tasks.Task" feature="System.Diagnostics.Debugger.IsSupported" featurevalue="false">
<field name="s_asyncDebuggingEnabled" value="false" initialize="false" />
<field name="s_asyncDebuggingEnabled" value="false" />
</type>
</assembly>
</linker>
8 changes: 0 additions & 8 deletions src/tools/illink/src/linker/CompatibilitySuppressions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,14 +743,6 @@
<DiagnosticId>CP0002</DiagnosticId>
<Target>M:Mono.Linker.AnnotationStore.HasPreservedStaticCtor(Mono.Cecil.TypeDefinition)</Target>
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>M:Mono.Linker.AnnotationStore.HasSubstitutedInit(Mono.Cecil.FieldDefinition)</Target>
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>M:Mono.Linker.AnnotationStore.HasSubstitutedInit(Mono.Cecil.TypeDefinition)</Target>
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>M:Mono.Linker.AnnotationStore.IsIndirectlyCalled(Mono.Cecil.MethodDefinition)</Target>
Expand Down
4 changes: 3 additions & 1 deletion src/tools/illink/src/linker/Linker.Steps/BodySubstitutionParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,9 @@ protected override void ProcessField (TypeDefinition type, XPathNavigator fieldN

string init = GetAttribute (fieldNav, "initialize");
if (init?.ToLowerInvariant () == "true") {
_substitutionInfo.SetFieldInit (field);
// We would need to also mess with the cctor of the type to set the field to this value,
// and doing so correctly is difficult.
throw new NotSupportedException ();
}
}

Expand Down
58 changes: 0 additions & 58 deletions src/tools/illink/src/linker/Linker.Steps/CodeRewriterStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,68 +37,10 @@ void ProcessType (TypeDefinition type)
ProcessMethod (method);
}

if (type.HasFields && Annotations.HasSubstitutedInit (type)) {
AddFieldsInitializations (type);
}

foreach (var nested in type.NestedTypes)
ProcessType (nested);
}

void AddFieldsInitializations (TypeDefinition type)
{
Instruction ret;
LinkerILProcessor processor;

var cctor = type.Methods.FirstOrDefault (MethodDefinitionExtensions.IsStaticConstructor);
if (cctor == null) {
type.Attributes |= TypeAttributes.BeforeFieldInit;

var method = new MethodDefinition (".cctor",
MethodAttributes.Static | MethodAttributes.Private | MethodAttributes.SpecialName | MethodAttributes.RTSpecialName | MethodAttributes.HideBySig,
Assembly.MainModule.TypeSystem.Void);

type.Methods.Add (method);

processor = method.Body.GetLinkerILProcessor ();
ret = Instruction.Create (OpCodes.Ret);
processor.Append (ret);
} else {
var body = cctor.Body;
#pragma warning disable RS0030 // After MarkStep all methods should be processed and thus accessing Cecil directly is the right approach
var instructions = body.Instructions;
#pragma warning restore RS0030
ret = instructions.Last (l => l.OpCode.Code == Code.Ret);
processor = body.GetLinkerILProcessor ();

for (int i = 0; i < instructions.Count; ++i) {
var instr = instructions[i];
if (instr.OpCode.Code != Code.Stsfld)
continue;

var field = (FieldReference) instr.Operand;
if (!Annotations.HasSubstitutedInit (field.Resolve ()))
continue;

processor.Replace (instr, Instruction.Create (OpCodes.Pop));
}
}

foreach (var field in type.Fields) {
if (!Annotations.HasSubstitutedInit (field))
continue;

Context.Annotations.TryGetFieldUserValue (field, out object? value);

var valueInstr = CreateConstantResultInstruction (Context, field.FieldType, value);
if (valueInstr == null)
throw new NotImplementedException (field.FieldType.ToString ());

processor.InsertBefore (ret, valueInstr);
processor.InsertBefore (ret, Instruction.Create (OpCodes.Stsfld, field));
}
}

void ProcessMethod (MethodDefinition method)
{
switch (Annotations.GetAction (method)) {
Expand Down
5 changes: 0 additions & 5 deletions src/tools/illink/src/linker/Linker.Steps/MarkStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1714,11 +1714,6 @@ void MarkField (FieldDefinition field, in DependencyInfo reason, in MessageOrigi
};
MarkStaticConstructor (parent, cctorReason, fieldOrigin);
}

if (Annotations.HasSubstitutedInit (field)) {
Annotations.SetPreservedStaticCtor (parent);
Annotations.SetSubstitutedInit (parent);
}
}

void ProcessAnalysisAnnotationsForField (FieldDefinition field, DependencyKind dependencyKind, in MessageOrigin origin)
Expand Down
15 changes: 0 additions & 15 deletions src/tools/illink/src/linker/Linker/Annotations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,21 +147,6 @@ public void SetStubValue (MethodDefinition method, object value)
MemberActions.PrimarySubstitutionInfo.SetMethodStubValue (method, value);
}

public bool HasSubstitutedInit (FieldDefinition field)
{
return MemberActions.HasSubstitutedInit (field);
}

public void SetSubstitutedInit (TypeDefinition type)
{
fieldType_init.Add (type);
}

public bool HasSubstitutedInit (TypeDefinition type)
{
return fieldType_init.Contains (type);
}

[Obsolete ("Mark token providers with a reason instead.")]
public void Mark (IMetadataTokenProvider provider)
{
Expand Down
11 changes: 0 additions & 11 deletions src/tools/illink/src/linker/Linker/MemberActionStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,16 +150,5 @@ public bool TryGetFieldUserValue (FieldDefinition field, out object? value)

return embeddedXml.FieldValues.TryGetValue (field, out value);
}

public bool HasSubstitutedInit (FieldDefinition field)
{
if (PrimarySubstitutionInfo.FieldInit.Contains (field))
return true;

if (!TryGetSubstitutionInfo (field, out var embeddedXml))
return false;

return embeddedXml.FieldInit.Contains (field);
}
}
}
7 changes: 0 additions & 7 deletions src/tools/illink/src/linker/Linker/SubstitutionInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,12 @@ public class SubstitutionInfo
public Dictionary<MethodDefinition, MethodAction> MethodActions { get; }
public Dictionary<MethodDefinition, object?> MethodStubValues { get; }
public Dictionary<FieldDefinition, object?> FieldValues { get; }
public HashSet<FieldDefinition> FieldInit { get; }

public SubstitutionInfo ()
{
MethodActions = new Dictionary<MethodDefinition, MethodAction> ();
MethodStubValues = new Dictionary<MethodDefinition, object?> ();
FieldValues = new Dictionary<FieldDefinition, object?> ();
FieldInit = new HashSet<FieldDefinition> ();
}

public void SetMethodAction (MethodDefinition method, MethodAction action)
Expand All @@ -35,10 +33,5 @@ public void SetFieldValue (FieldDefinition field, object? value)
{
FieldValues[field] = value;
}

public void SetFieldInit (FieldDefinition field)
{
FieldInit.Add (field);
}
}
}
49 changes: 13 additions & 36 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/FeatureSettings/FeatureSubstitutionsNested.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,67 +20,44 @@ namespace Mono.Linker.Tests.Cases.FeatureSettings
[KeptResource ("ResourceFileRemoveWhenFalse.txt")]
public class FeatureSubstitutionsNested
{
[ExpectedInstructionSequence (new[] {
"nop",
"ldc.i4.1",
"pop",
"ldc.i4.0",
"pop",
"ldc.i4.1",
"pop",
"ldc.i4.0",
"pop",
"ret"
})]
public static void Main ()
{
GlobalConditionMethod ();
AssemblyConditionMethod ();
TypeConditionMethod ();
MethodConditionMethod ();
_ = FieldConditionField;
}

[Kept]
[ExpectedInstructionSequence (new[] {
"ldc.i4.1",
"ret",
})]
Comment on lines -32 to -36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

static bool GlobalConditionMethod ()
{
throw new NotImplementedException ();
}

[Kept]
[ExpectedInstructionSequence (new[] {
"ldc.i4.0",
"ret",
})]
static bool AssemblyConditionMethod ()
{
throw new NotImplementedException ();
}

[Kept]
[ExpectedInstructionSequence (new[] {
"ldc.i4.1",
"ret",
})]
static bool TypeConditionMethod ()
{
throw new NotImplementedException ();
}

[Kept]
[ExpectedInstructionSequence (new[] {
"ldc.i4.0",
"ret",
})]
static bool MethodConditionMethod ()
{
throw new NotImplementedException ();
}

[Kept]
static readonly bool FieldConditionField;

[Kept]
[ExpectedInstructionSequence (new[] {
"nop",
"ldc.i4.1",
"stsfld System.Boolean Mono.Linker.Tests.Cases.FeatureSettings.FeatureSubstitutionsNested::FieldConditionField",
"ret"
})]
static FeatureSubstitutionsNested ()
{
}
}
}
}
4 changes: 0 additions & 4 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/FeatureSettings/FeatureSubstitutionsNested.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@
<method signature="System.Boolean MethodConditionMethod()" body="stub" value="false" feature="MethodCondition" featurevalue="false" />
<!-- Else case -->
<method signature="System.Boolean MethodConditionMethod()" body="stub" value="true" feature="MethodCondition" featurevalue="true" />
<!-- Or on the field element. -->
<field name="FieldConditionField" value="true" initialize="true" feature="FieldCondition" featurevalue="true" />
<!-- Else case -->
<field name="FieldConditionField" value="false" initialize="true" feature="FieldCondition" featurevalue="false" />
</type>
<!-- Else case for the type feature attribute -->
<type fullname="Mono.Linker.Tests.Cases.FeatureSettings.FeatureSubstitutionsNested" feature="TypeCondition" featurevalue="false">
Expand Down
11 changes: 0 additions & 11 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/Substitutions/Dependencies/ReferencedField.cs
View file
Open in desktop

This file was deleted.

7 changes: 0 additions & 7 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/Substitutions/Dependencies/ReferencedField.xml
View file
Open in desktop

This file was deleted.

29 changes: 0 additions & 29 deletions ...t/Mono.Linker.Tests.Cases/Substitutions/EmbeddedFieldSubstitutionsInReferencedAssembly.cs
View file
Open in desktop

This file was deleted.

24 changes: 0 additions & 24 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/Substitutions/InitField.cs
View file
Open in desktop

This file was deleted.

8 changes: 0 additions & 8 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/Substitutions/InitField.xml
View file
Open in desktop

This file was deleted.

Loading