Skip to content

Commit

Permalink
[DAM analyzer] Support exceptional control flow (#2481)
Browse files Browse the repository at this point in the history
This includes a change to make the analyzer tests fail when an
exception is thrown.

This models exceptional control flow by maintaining an exception state
for try/catch regions, which is updated to include all possible states
encountered within that region. This exceptional state is propagated
from try regions to the beginning of catch regions, and from try or
catch regions to the beginning of finally regions.

Finally regions are represented as additional information for each
predecessor edge in the control flow graph, following the data model
given by Roslyn's CFG. Each edge includes a list of finally regions
through which control flows as part of the edge before reaching its
destination. For now, this approach inefficiently propagates the old
finally state along these chains of finally regions when it encounters
them. This has the downside that long finally regions will cause
re-analysis until each finally block sees a new state.

The finally state is also unified along all such paths, so the warning
state at the exit of a finally is a conservative approximation that
accounts for all normal control flow leading to the finally region.

Additionally, the finally regions are analyzed separately for the case
when an exception causes a finally block to be reached. Inside the
finally region, this will produce a superset of the warnings for
normal control flow inside the same region. However this exceptional
finally state does not flow out of the finally, to avoid spurious
warnings in code that is unreachable when executing finally blocks due
to exceptions.

Also, implement a wrapper type where Set automatically meets the
exception state.  The Set operation is specific to the local dataflow
analysis, so this introduces a state interface to track the
current/exception state. Only the state type for the local dataflow
analysis (LocalDataFlowState) implements this specialized behavior.
  • Loading branch information
sbomer authored Jan 13, 2022
1 parent b540d83 commit b3c58ad
Show file tree
Hide file tree
Showing 20 changed files with 1,535 additions and 159 deletions.
126 changes: 126 additions & 0 deletions src/ILLink.RoslynAnalyzer/DataFlow/ControlFlowGraphProxy.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using ILLink.Shared.DataFlow;
using Microsoft.CodeAnalysis.FlowAnalysis;

using Predecessor = ILLink.Shared.DataFlow.IControlFlowGraph<
ILLink.RoslynAnalyzer.DataFlow.BlockProxy,
ILLink.RoslynAnalyzer.DataFlow.RegionProxy
>.Predecessor;

namespace ILLink.RoslynAnalyzer.DataFlow
{
// Blocks should be usable as keys of a dictionary.
// The record equality implementation will check for reference equality
// on the underlying BasicBlock, so uses of this class should not expect
// any kind of value equality for different block instances. In practice
// this should be fine as long as we consistently use block instances from
// a single ControlFlowGraph.
public readonly record struct BlockProxy (BasicBlock Block)
{
public override string ToString ()
{
return base.ToString () + $"[{Block.Ordinal}]";
}
}

public readonly record struct RegionProxy (ControlFlowRegion Region) : IRegion<RegionProxy>
{
public RegionKind Kind => Region.Kind switch {
ControlFlowRegionKind.Try => RegionKind.Try,
ControlFlowRegionKind.Catch => RegionKind.Catch,
ControlFlowRegionKind.Finally => RegionKind.Finally,
_ => throw new InvalidOperationException ()
};
}

public readonly record struct ControlFlowGraphProxy (ControlFlowGraph ControlFlowGraph) : IControlFlowGraph<BlockProxy, RegionProxy>
{
public IEnumerable<BlockProxy> Blocks {
get {
foreach (var block in ControlFlowGraph.Blocks)
yield return new BlockProxy (block);
}
}

public BlockProxy Entry => new BlockProxy (ControlFlowGraph.Blocks[0]);

// This is implemented by getting predecessors of the underlying Roslyn BasicBlock.
// This is fine as long as the blocks come from the correct control-flow graph.
public IEnumerable<Predecessor> GetPredecessors (BlockProxy block)
{
foreach (var predecessor in block.Block.Predecessors) {
if (predecessor.FinallyRegions.IsEmpty) {
yield return new Predecessor (new BlockProxy (predecessor.Source), ImmutableArray<RegionProxy>.Empty);
continue;
}
var finallyRegions = ImmutableArray.CreateBuilder<RegionProxy> ();
foreach (var region in predecessor.FinallyRegions) {
if (region == null)
throw new InvalidOperationException ();
finallyRegions.Add (new RegionProxy (region));
}
yield return new Predecessor (new BlockProxy (predecessor.Source), finallyRegions.ToImmutable ());
}
}

public bool TryGetEnclosingTryOrCatch (BlockProxy block, out RegionProxy tryOrCatchRegion)
{
return TryGetTryOrCatch (block.Block.EnclosingRegion, out tryOrCatchRegion);
}

public bool TryGetEnclosingTryOrCatch (RegionProxy regionProxy, out RegionProxy tryOrCatchRegion)
{
return TryGetTryOrCatch (regionProxy.Region.EnclosingRegion, out tryOrCatchRegion);
}

bool TryGetTryOrCatch (ControlFlowRegion? region, out RegionProxy tryOrCatchRegion)
{
tryOrCatchRegion = default;
while (region != null) {
if (region.Kind is ControlFlowRegionKind.Try or ControlFlowRegionKind.Catch) {
tryOrCatchRegion = new RegionProxy (region);
return true;
}
region = region.EnclosingRegion;
}
return false;
}

public bool TryGetEnclosingFinally (BlockProxy block, out RegionProxy catchRegion)
{
catchRegion = default;
ControlFlowRegion? region = block.Block.EnclosingRegion;
while (region != null) {
if (region.Kind == ControlFlowRegionKind.Finally) {
catchRegion = new RegionProxy (region);
return true;
}
region = region.EnclosingRegion;
}
return false;
}

public RegionProxy GetCorrespondingTry (RegionProxy catchOrFinallyRegion)
{
if (catchOrFinallyRegion.Region.Kind is not (ControlFlowRegionKind.Finally or ControlFlowRegionKind.Catch))
throw new ArgumentException (nameof (catchOrFinallyRegion));

foreach (var nested in catchOrFinallyRegion.Region.EnclosingRegion!.NestedRegions) {
// Note that for try+catch+finally, the try corresponding to the finally will not be the same as
// the try corresponding to the catch, because Roslyn represents this region hierarchy the same as
// a try+catch nested inside the try block of a try+finally.
if (nested.Kind == ControlFlowRegionKind.Try)
return new (nested);
}
throw new InvalidOperationException ();
}

public BlockProxy FirstBlock (RegionProxy region) =>
new BlockProxy (ControlFlowGraph.Blocks[region.Region.FirstBlockOrdinal]);

public BlockProxy LastBlock (RegionProxy region) =>
new BlockProxy (ControlFlowGraph.Blocks[region.Region.LastBlockOrdinal]);
}
}
34 changes: 34 additions & 0 deletions src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowState.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using ILLink.Shared.DataFlow;

namespace ILLink.RoslynAnalyzer.DataFlow
{
public class LocalDataFlowState<TValue, TValueLattice>
: IDataFlowState<LocalState<TValue>, LocalStateLattice<TValue, TValueLattice>>
where TValue : struct, IEquatable<TValue>
where TValueLattice : ILattice<TValue>
{
LocalState<TValue> current;
public LocalState<TValue> Current {
get => current;
set => current = value;
}

public Box<LocalState<TValue>>? Exception { get; set; }

public LocalStateLattice<TValue, TValueLattice> Lattice { get; init; }

public void Set (LocalKey key, TValue value)
{
current.Set (key, value);
if (Exception != null)
// TODO: optimize this to not meet the whole value, but just modify one value without copying.
Exception.Value = Lattice.Meet (Exception.Value, current);
}

public TValue Get (LocalKey key) => current.Get (key);
}
}
74 changes: 30 additions & 44 deletions src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

using System;
using System.Diagnostics;
using ILLink.RoslynAnalyzer.TrimAnalysis;
using ILLink.Shared.DataFlow;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
Expand All @@ -17,8 +16,8 @@ namespace ILLink.RoslynAnalyzer.DataFlow
// - field
// - parameter
// - method return
public abstract class LocalDataFlowVisitor<TValue, TValueLattice> : OperationWalker<LocalState<TValue>, TValue>,
ITransfer<BlockProxy, LocalState<TValue>, LocalStateLattice<TValue, TValueLattice>>
public abstract class LocalDataFlowVisitor<TValue, TValueLattice> : OperationWalker<LocalDataFlowState<TValue, TValueLattice>, TValue>,
ITransfer<BlockProxy, LocalState<TValue>, LocalDataFlowState<TValue, TValueLattice>, LocalStateLattice<TValue, TValueLattice>>
// This struct constraint prevents warnings due to possible null returns from the visitor methods.
// Note that this assumes that default(TValue) is equal to the TopValue.
where TValue : struct, IEquatable<TValue>
Expand All @@ -33,47 +32,35 @@ public abstract class LocalDataFlowVisitor<TValue, TValueLattice> : OperationWal
public LocalDataFlowVisitor (LocalStateLattice<TValue, TValueLattice> lattice, OperationBlockAnalysisContext context) =>
(LocalStateLattice, Context) = (lattice, context);

public void Transfer (BlockProxy block, LocalState<TValue> state)
public void Transfer (BlockProxy block, LocalDataFlowState<TValue, TValueLattice> state)
{
foreach (IOperation operation in block.Block.Operations)
Visit (operation, state);

// Blocks may end with a BranchValue computation. Visit the BranchValue operation after all others.
IOperation? branchValueOperation = block.Block.BranchValue;
if (branchValueOperation != null) {
var branchValue = Visit (branchValueOperation, state);
if (branchValueOperation == null)
return;

// BranchValue may represent a value used in a conditional branch to the ConditionalSuccessor - if so, we are done.
var branchValue = Visit (branchValueOperation, state);

// If not, the BranchValue represents a return or throw value associated with the FallThroughSuccessor of this block.
// (ConditionalSuccessor == null iff ConditionKind == None).
if (block.Block.ConditionKind == ControlFlowConditionKind.None) {
// This means it's a return value or throw value associated with the fall-through successor.
// BranchValue may represent a value used in a conditional branch to the ConditionalSuccessor - if so, we are done.
if (block.Block.ConditionKind != ControlFlowConditionKind.None)
return;

// Return statements with return values are represented in the control flow graph as
// a branch value operation that computes the return value. The return operation itself is the parent
// of the branch value operation.
// If not, the BranchValue represents a return or throw value associated with the FallThroughSuccessor of this block.
// (ConditionalSuccessor == null iff ConditionKind == None).

// Get the actual "return Foo;" operation (not the branch value operation "Foo").
// This should be used as the location of the warning. This is important to provide the right
// warning location and because warnings are disambiguated based on the operation.
var parentSyntax = branchValueOperation.Syntax.Parent;
if (parentSyntax == null)
throw new InvalidOperationException ();
// The BranchValue for a thrown value is not involved in dataflow tracking.
if (block.Block.FallThroughSuccessor?.Semantics == ControlFlowBranchSemantics.Throw)
return;

var parentOperation = Context.Compilation.GetSemanticModel (branchValueOperation.Syntax.SyntaxTree).GetOperation (parentSyntax);
// Return statements with return values are represented in the control flow graph as
// a branch value operation that computes the return value.

// Analyzer doesn't support exceptional control-flow:
// https://github.com/dotnet/linker/issues/2273
if (parentOperation is IThrowOperation)
throw new NotImplementedException ();

if (parentOperation is not IReturnOperation returnOperation)
throw new InvalidOperationException ();

HandleReturnValue (branchValue, returnOperation);
}
}
// Use the branch value operation as the key for the warning store and the location of the warning.
// We don't want the return operation because this might have multiple possible return values in general.
HandleReturnValue (branchValue, branchValueOperation);
}

public abstract void HandleAssignment (TValue source, TValue target, IOperation operation);
Expand All @@ -92,12 +79,12 @@ public void Transfer (BlockProxy block, LocalState<TValue> state)
// may (must?) come from BranchValue of an operation whose FallThroughSuccessor is the exit block.
public abstract void HandleReturnValue (TValue returnValue, IOperation operation);

public override TValue VisitLocalReference (ILocalReferenceOperation operation, LocalState<TValue> state)
public override TValue VisitLocalReference (ILocalReferenceOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
{
return state.Get (new LocalKey (operation.Local));
}

public override TValue VisitSimpleAssignment (ISimpleAssignmentOperation operation, LocalState<TValue> state)
public override TValue VisitSimpleAssignment (ISimpleAssignmentOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
{
var targetValue = Visit (operation.Target, state);
var value = Visit (operation.Value, state);
Expand Down Expand Up @@ -131,28 +118,28 @@ public override TValue VisitSimpleAssignment (ISimpleAssignmentOperation operati
}

// Similar to VisitLocalReference
public override TValue VisitFlowCaptureReference (IFlowCaptureReferenceOperation operation, LocalState<TValue> state)
public override TValue VisitFlowCaptureReference (IFlowCaptureReferenceOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
{
return state.Get (new LocalKey (operation.Id));
}

// Similar to VisitSimpleAssignment when assigning to a local, but for values which are captured without a
// corresponding local variable. The "flow capture" is like a local assignment, and the "flow capture reference"
// is like a local reference.
public override TValue VisitFlowCapture (IFlowCaptureOperation operation, LocalState<TValue> state)
public override TValue VisitFlowCapture (IFlowCaptureOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
{
TValue value = Visit (operation.Value, state);
state.Set (new LocalKey (operation.Id), value);
return value;
}

public override TValue VisitExpressionStatement (IExpressionStatementOperation operation, LocalState<TValue> state)
public override TValue VisitExpressionStatement (IExpressionStatementOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
{
Visit (operation.Operation, state);
return TopValue;
}

public override TValue VisitInvocation (IInvocationOperation operation, LocalState<TValue> state)
public override TValue VisitInvocation (IInvocationOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
{
if (operation.Instance != null) {
var instanceValue = Visit (operation.Instance, state);
Expand All @@ -170,8 +157,7 @@ public static IMethodSymbol GetPropertyMethod (IPropertyReferenceOperation opera
// The IPropertyReferenceOperation doesn't tell us whether this reference is to the getter or setter.
// For this we need to look at the containing operation.
var parent = operation.Parent;
Debug.Assert (parent != null);
if (parent!.Kind == OperationKind.SimpleAssignment) {
if (parent?.Kind == OperationKind.SimpleAssignment) {
var assignment = (ISimpleAssignmentOperation) parent;
if (assignment.Target == operation) {
var setMethod = operation.Property.SetMethod;
Expand All @@ -186,7 +172,7 @@ public static IMethodSymbol GetPropertyMethod (IPropertyReferenceOperation opera
return getMethod!;
}

public override TValue VisitPropertyReference (IPropertyReferenceOperation operation, LocalState<TValue> state)
public override TValue VisitPropertyReference (IPropertyReferenceOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
{
if (operation.Instance != null) {
var instanceValue = Visit (operation.Instance, state);
Expand All @@ -196,14 +182,14 @@ public override TValue VisitPropertyReference (IPropertyReferenceOperation opera
return TopValue;
}

public override TValue VisitArgument (IArgumentOperation operation, LocalState<TValue> state)
public override TValue VisitArgument (IArgumentOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
{
var value = Visit (operation.Value, state);
HandleArgument (value, operation);
return value;
}

public override TValue VisitReturn (IReturnOperation operation, LocalState<TValue> state)
public override TValue VisitReturn (IReturnOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
{
if (operation.ReturnedValue != null) {
var value = Visit (operation.ReturnedValue, state);
Expand All @@ -214,7 +200,7 @@ public override TValue VisitReturn (IReturnOperation operation, LocalState<TValu
return TopValue;
}

public override TValue VisitConversion (IConversionOperation operation, LocalState<TValue> state)
public override TValue VisitConversion (IConversionOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
{
var operandValue = Visit (operation.Operand, state);
return operation.OperatorMethod == null ? operandValue : TopValue;
Expand Down
10 changes: 4 additions & 6 deletions src/ILLink.RoslynAnalyzer/DataFlow/LocalStateLattice.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,12 @@ public override string ToString ()
{
if (Local != null)
return Local.ToString ();
return $"capture {CaptureId.GetHashCode ().ToString ().Substring (0, 3)}";
return $"capture {CaptureId.GetHashCode ()}";
}
}

// Wrapper class exists purely to substitute a concrete LocalKey for TKey of DefaultValueDictionary
// This is a class because it is passed to the transfer functions and expected to be modified in a
// way that is visible to the caller.
public class LocalState<TValue> : IEquatable<LocalState<TValue>>
// Wrapper struct exists purely to substitute a concrete LocalKey for TKey of DefaultValueDictionary
public struct LocalState<TValue> : IEquatable<LocalState<TValue>>
where TValue : IEquatable<TValue>
{
public DefaultValueDictionary<LocalKey, TValue> Dictionary;
Expand All @@ -50,7 +48,7 @@ public class LocalState<TValue> : IEquatable<LocalState<TValue>>

// Wrapper struct exists purely to substitute a concrete LocalKey for TKey of DictionaryLattice
public readonly struct LocalStateLattice<TValue, TValueLattice> : ILattice<LocalState<TValue>>
where TValue : IEquatable<TValue>
where TValue : struct, IEquatable<TValue>
where TValueLattice : ILattice<TValue>
{
public readonly DictionaryLattice<LocalKey, TValue, TValueLattice> Lattice;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@

using System.Collections;
using System.Collections.Generic;
using System.Diagnostics;
using ILLink.Shared.DataFlow;
using Microsoft.CodeAnalysis;

namespace ILLink.RoslynAnalyzer.TrimAnalysis
Expand All @@ -17,21 +15,7 @@ namespace ILLink.RoslynAnalyzer.TrimAnalysis

public void Add (TrimAnalysisPattern trimAnalysisPattern)
{
// If we already stored a trim analysis pattern for this operation,
// it needs to be updated. The dataflow analysis should result in purely additive
// changes to the trim analysis patterns generated for a given operation,
// so we can just replace the original analysis pattern here.
#if DEBUG
// Validate this in debug mode.
if (TrimAnalysisPatterns.TryGetValue (trimAnalysisPattern.Operation, out var existingTrimAnalysisPattern)) {
// The existing pattern source/target should be a subset of the new source/target.
foreach (SingleValue source in existingTrimAnalysisPattern.Source)
Debug.Assert (trimAnalysisPattern.Source.Contains (source));

foreach (SingleValue target in existingTrimAnalysisPattern.Target)
Debug.Assert (trimAnalysisPattern.Target.Contains (target));
}
#endif
// TODO: check that this doesn't lose warnings (or add instead of replace)
TrimAnalysisPatterns[trimAnalysisPattern.Operation] = trimAnalysisPattern;
}

Expand Down
Loading

0 comments on commit b3c58ad

Please sign in to comment.