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

Fix branch removal in compiler generated code #3088

Merged
merged 6 commits into from
Nov 1, 2022
Merged

Fix branch removal in compiler generated code #3088

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
84e8574
Fix branch removal in compiler generated code
vitek-karas Oct 27, 2022
67aeb3b
Formatting
vitek-karas Oct 27, 2022
69dcb6f
Rework the change to guarantee that all accessed to Body are after th…
vitek-karas Oct 31, 2022
d96a99b
Merge remote-tracking branch 'mono/main' into FixBranchRemovalInCompi…
vitek-karas Oct 31, 2022
e63c115
PR feedback and other cleanup
vitek-karas Nov 1, 2022
1337c34
PR feedback
vitek-karas Nov 1, 2022
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
3 changes: 3 additions & 0 deletions src/linker/BannedSymbols.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,6 @@ M:Mono.Cecil.MethodReference.Resolve();Use LinkContext.Resolve and LinkContext.T
M:Mono.Cecil.ExportedType.Resolve();Use LinkContext.Resolve and LinkContext.TryResolve helpers instead
P:Mono.Collections.Generic.Collection`1{Mono.Cecil.ParameterDefinition}.Item(System.Int32); use x
P:Mono.Cecil.ParameterDefinitionCollection.Item(System.Int32); use x
P:Mono.Cecil.Cil.MethodBody.Instructions;Use LinkContext.MethodBodyInstructionProvider instead
P:Mono.Cecil.Cil.MethodBody.ExceptionHandlers;Use LinkContext.MethodBodyInstructionProvider instead
P:Mono.Cecil.Cil.MethodBody.Variable;Use LinkContext.MethodBodyInstructionProvider instead
4 changes: 2 additions & 2 deletions src/linker/Linker.Dataflow/CompilerGeneratedState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ void ProcessMethod (MethodDefinition method)
// Discover calls or references to lambdas or local functions. This includes
// calls to local functions, and lambda assignments (which use ldftn).
if (method.Body != null) {
foreach (var instruction in method.Body.Instructions) {
foreach (var instruction in _context.MethodBodyInstructionsProvider.GetMethodBody (method.Body).Instructions) {
switch (instruction.OpCode.OperandType) {
case OperandType.InlineMethod: {
MethodDefinition? referencedMethod = _context.TryResolve ((MethodReference) instruction.Operand);
Expand Down Expand Up @@ -354,7 +354,7 @@ void MapGeneratedTypeTypeParameters (TypeDefinition generatedType)

GenericInstanceType? ScanForInit (TypeDefinition compilerGeneratedType, MethodBody body)
{
foreach (var instr in body.Instructions) {
foreach (var instr in _context.MethodBodyInstructionsProvider.GetMethodBody (body).Instructions) {
bool handled = false;
switch (instr.OpCode.Code) {
case Code.Initobj:
Expand Down
2 changes: 1 addition & 1 deletion src/linker/Linker.Dataflow/FlowAnnotations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ bool ScanMethodBodyForFieldAccess (MethodBody body, bool write, out FieldDefinit

FieldReference? foundReference = null;

foreach (Instruction instruction in body.Instructions) {
foreach (Instruction instruction in _context.MethodBodyInstructionsProvider.GetMethodBody (body).Instructions) {
switch (instruction.OpCode.Code) {
case Code.Ldsfld when !write:
case Code.Ldfld when !write:
Expand Down
16 changes: 12 additions & 4 deletions src/linker/Linker.Dataflow/InterproceduralState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using ILLink.Shared.DataFlow;
using Mono.Cecil;
using Mono.Cecil.Cil;
using Mono.Linker.Steps;
using HoistedLocalState = ILLink.Shared.DataFlow.DefaultValueDictionary<
Mono.Linker.Dataflow.HoistedLocalKey,
ILLink.Shared.DataFlow.ValueSet<ILLink.Shared.DataFlow.SingleValue>>;
Expand All @@ -15,7 +16,7 @@
namespace Mono.Linker.Dataflow
{
// Wrapper that implements IEquatable for MethodBody.
readonly record struct MethodBodyValue (MethodBody MethodBody);
readonly record struct MethodBodyValue (MethodBodyInstructionsProvider.ProcessedMethodBody MethodBody);

// Tracks the set of methods which get analyzer together during interprocedural analysis,
// and the possible states of hoisted locals in state machine methods and lambdas/local functions.
Expand Down Expand Up @@ -43,6 +44,11 @@ public void TrackMethod (MethodDefinition method)
}

public void TrackMethod (MethodBody methodBody)
{
TrackMethod (lattice.Context.MethodBodyInstructionsProvider.GetMethodBody (methodBody));
}

public void TrackMethod (MethodBodyInstructionsProvider.ProcessedMethodBody methodBody)
{
// Work around the fact that ValueSet is readonly
var methodsList = new List<MethodBodyValue> (MethodBodies);
Expand All @@ -55,7 +61,7 @@ public void TrackMethod (MethodBody methodBody)
foreach (var stateMachineMethod in stateMachineType.Methods) {
Debug.Assert (!CompilerGeneratedNames.IsLambdaOrLocalFunction (stateMachineMethod.Name));
if (stateMachineMethod.Body is MethodBody stateMachineMethodBody)
methodsList.Add (new MethodBodyValue (stateMachineMethodBody));
methodsList.Add (new MethodBodyValue (lattice.Context.MethodBodyInstructionsProvider.GetMethodBody(stateMachineMethodBody)));
}
}

Expand All @@ -80,11 +86,13 @@ struct InterproceduralStateLattice : ILattice<InterproceduralState>
{
public readonly ValueSetLattice<MethodBodyValue> MethodBodyLattice;
public readonly DictionaryLattice<HoistedLocalKey, MultiValue, ValueSetLattice<SingleValue>> HoistedLocalsLattice;
public readonly LinkContext Context;

public InterproceduralStateLattice (
ValueSetLattice<MethodBodyValue> methodBodyLattice,
DictionaryLattice<HoistedLocalKey, MultiValue, ValueSetLattice<SingleValue>> hoistedLocalsLattice)
=> (MethodBodyLattice, HoistedLocalsLattice) = (methodBodyLattice, hoistedLocalsLattice);
DictionaryLattice<HoistedLocalKey, MultiValue, ValueSetLattice<SingleValue>> hoistedLocalsLattice,
LinkContext context)
=> (MethodBodyLattice, HoistedLocalsLattice, Context) = (methodBodyLattice, hoistedLocalsLattice, context);

public InterproceduralState Top => new InterproceduralState (MethodBodyLattice.Top, HoistedLocalsLattice.Top, this);

Expand Down
18 changes: 10 additions & 8 deletions src/linker/Linker.Dataflow/MethodBodyScanner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using Mono.Cecil;
using Mono.Cecil.Cil;
using Mono.Collections.Generic;
using Mono.Linker.Steps;
using static Mono.Linker.ParameterHelpers;
using LocalVariableStore = System.Collections.Generic.Dictionary<
Mono.Cecil.Cil.VariableDefinition,
Expand Down Expand Up @@ -52,7 +53,7 @@ abstract partial class MethodBodyScanner
protected MethodBodyScanner (LinkContext context)
{
this._context = context;
this.InterproceduralStateLattice = default;
this.InterproceduralStateLattice = new InterproceduralStateLattice(default, default, context);
}

internal MultiValue ReturnValue { private set; get; }
Expand Down Expand Up @@ -152,7 +153,7 @@ private struct BasicBlockIterator
int _currentBlockIndex;
bool _foundEndOfPrevBlock;

public BasicBlockIterator (MethodBody methodBody)
public BasicBlockIterator (MethodBodyInstructionsProvider.ProcessedMethodBody methodBody)
{
_methodBranchTargets = methodBody.ComputeBranchTargets ();
_currentBlockIndex = -1;
Expand Down Expand Up @@ -227,7 +228,7 @@ protected static void StoreMethodLocalValue<KeyType> (

// Scans the method as well as any nested functions (local functions or lambdas) and state machines
// reachable from it.
public virtual void InterproceduralScan (MethodBody startingMethodBody)
public virtual void InterproceduralScan (MethodBodyInstructionsProvider.ProcessedMethodBody startingMethodBody)
{
MethodDefinition startingMethod = startingMethodBody.Method;

Expand Down Expand Up @@ -275,21 +276,22 @@ void TrackNestedFunctionReference (MethodReference referencedMethod, ref Interpr
interproceduralState.TrackMethod (method);
}

protected virtual void Scan (MethodBody methodBody, ref InterproceduralState interproceduralState)
protected virtual void Scan (MethodBodyInstructionsProvider.ProcessedMethodBody processedMethodBody, ref InterproceduralState interproceduralState)
{
MethodBody methodBody = processedMethodBody.Body;
MethodDefinition thisMethod = methodBody.Method;

LocalVariableStore locals = new (methodBody.Variables.Count);

Dictionary<int, Stack<StackSlot>> knownStacks = new Dictionary<int, Stack<StackSlot>> ();
Stack<StackSlot>? currentStack = new Stack<StackSlot> (methodBody.MaxStackSize);

ScanExceptionInformation (knownStacks, methodBody);
ScanExceptionInformation (knownStacks, processedMethodBody);

BasicBlockIterator blockIterator = new BasicBlockIterator (methodBody);
BasicBlockIterator blockIterator = new BasicBlockIterator (processedMethodBody);

ReturnValue = new ();
foreach (Instruction operation in methodBody.Instructions) {
foreach (Instruction operation in processedMethodBody.Instructions) {
int curBasicBlock = blockIterator.MoveNext (operation);

if (knownStacks.ContainsKey (operation.Offset)) {
Expand Down Expand Up @@ -700,7 +702,7 @@ protected virtual void Scan (MethodBody methodBody, ref InterproceduralState int
}
}

private static void ScanExceptionInformation (Dictionary<int, Stack<StackSlot>> knownStacks, MethodBody methodBody)
private static void ScanExceptionInformation (Dictionary<int, Stack<StackSlot>> knownStacks, MethodBodyInstructionsProvider.ProcessedMethodBody methodBody)
Copy link
Member

Choose a reason for hiding this comment

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

Just to make sure I understand, this needs to be a ProcessedMethodBody because it's static, right? And the other instance methods we are just assuming we pass the ProcessedMethodBody.Body from Scan?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think I did anything with static/instance methods on this class. The idea is that Scan gets the processed method from the outside and it passes it to all of the helpers here - including this one.
Sorry, I probably don't understand the question.

{
foreach (ExceptionHandler exceptionClause in methodBody.ExceptionHandlers) {
Stack<StackSlot> catchStack = new Stack<StackSlot> (1);
Expand Down
4 changes: 2 additions & 2 deletions src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,15 +61,15 @@ public ReflectionMethodBodyScanner (LinkContext context, MarkStep parent, Messag
TrimAnalysisPatterns = new TrimAnalysisPatternStore (MultiValueLattice, context);
}

public override void InterproceduralScan (MethodBody methodBody)
public override void InterproceduralScan (MethodBodyInstructionsProvider.ProcessedMethodBody methodBody)
{
base.InterproceduralScan (methodBody);

var reflectionMarker = new ReflectionMarker (_context, _markStep, enabled: true);
TrimAnalysisPatterns.MarkAndProduceDiagnostics (reflectionMarker, _markStep);
}

protected override void Scan (MethodBody methodBody, ref InterproceduralState interproceduralState)
protected override void Scan (MethodBodyInstructionsProvider.ProcessedMethodBody methodBody, ref InterproceduralState interproceduralState)
{
_origin = new MessageOrigin (methodBody.Method);
base.Scan (methodBody, ref interproceduralState);
Expand Down
3 changes: 2 additions & 1 deletion src/linker/Linker.Dataflow/ScannerExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using Mono.Cecil.Cil;
using Mono.Linker.Steps;

namespace Mono.Linker.Dataflow
{
Expand All @@ -16,7 +17,7 @@ public static bool IsControlFlowInstruction (in this OpCode opcode)
|| (opcode.FlowControl == FlowControl.Return && opcode.Code != Code.Ret);
}

public static HashSet<int> ComputeBranchTargets (this MethodBody methodBody)
public static HashSet<int> ComputeBranchTargets (this MethodBodyInstructionsProvider.ProcessedMethodBody methodBody)
{
HashSet<int> branchTargets = new HashSet<int> ();
foreach (Instruction operation in methodBody.Instructions) {
Expand Down
2 changes: 2 additions & 0 deletions src/linker/Linker.Steps/AddBypassNGenStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,9 @@ private void EnsureBypassNGenAttribute (ModuleDefinition targetModule)

const MethodAttributes ctorAttributes = MethodAttributes.Public | MethodAttributes.HideBySig | MethodAttributes.SpecialName | MethodAttributes.RTSpecialName;
bypassNGenAttributeDefaultConstructor = new MethodDefinition (".ctor", ctorAttributes, coreLibAssembly.MainModule.TypeSystem.Void);
#pragma warning disable RS0030 // Anything after MarkStep should use Cecil directly as all method bodies should be processed by this point
var instructions = bypassNGenAttributeDefaultConstructor.Body.Instructions;
#pragma warning restore RS0030
instructions.Add (Instruction.Create (OpCodes.Ldarg_0));
instructions.Add (Instruction.Create (OpCodes.Call, systemAttributeDefaultConstructor));
instructions.Add (Instruction.Create (OpCodes.Ret));
Expand Down
13 changes: 8 additions & 5 deletions src/linker/Linker.Steps/CodeRewriterStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,15 @@ void AddFieldsInitializations (TypeDefinition type)
ret = Instruction.Create (OpCodes.Ret);
processor.Append (ret);
} else {
ret = cctor.Body.Instructions.Last (l => l.OpCode.Code == Code.Ret);
var body = cctor.Body;
processor = cctor.Body.GetLinkerILProcessor ();

for (int i = 0; i < body.Instructions.Count; ++i) {
var instr = body.Instructions[i];
#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;

Expand Down
Loading