Skip to content

Commit

Permalink
Fix analyzer treatment of flow captures of arrays (#93420)
Browse files Browse the repository at this point in the history 
#93259 uncovered an issue
around how the analyzer tracks arrays. It hit an analyzer assert
while trying to create an l-value flow capture of another flow
capture reference, which should never happen as far as I
understand. The assert was firing for
https://github.com/dotnet/runtime/blob/946c5245aea149d9ece53fd0debc18328c29083b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs#L511

Now tested in TestNullCoalescingAssignment:
```csharp
(arr ??= arr2)[0] = typeof (V);
```

The CFG had three flow captures:

- capture 0: an l-value flow capture of `arr`. Used later in the
  branch that assigns `arr2` to `arr`.

- capture 1: an r-value flow capture of capture 0. This was
  checked for null.

- capture 2: an l-value flow capture representing `arr ??= arr2`,
  used to write index 0 of the array.

  - In the == null branch, this captured the result of an
    assignment (capture 0 = `arr2`)

  - In the other branch, it captured "capture 1". This is where
    the assert was hit.

The bug, I believe, is that capture 2 should have been an r-value
flow capture instead. Even though it's used for writing to the
array, the assignment doesn't modify the array pointer
represented by this capture - it dereferences this pointer and
modifies the array. This was introduced by my modifications to
the l-value detection logic in
#90287.

This undoes that portion of the change so that capture 2 is now
treated as an r-value capture. This simplifies the array element
assignment logic so that it no longer can see an assignment where
the array is an l-value.

Fixes #93420 by adding an
explicit check for `IsInitialization` so that we don't hit the
related asserts for string interpolation handlers.
  • Loading branch information
@sbomer
sbomer authored Oct 18, 2023
1 parent f16e8bd commit e1e18ee
Show file tree
Hide file tree
Showing 7 changed files with 238 additions and 124 deletions.
6 changes: 0 additions & 6 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LValueFlowCaptureProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,6 @@ static bool IsLValueFlowCapture (IFlowCaptureReferenceOperation flowCaptureRefer
if (assignment?.Target == flowCaptureReference)
return true;

if (flowCaptureReference.Parent is IArrayElementReferenceOperation arrayAlementRef) {
assignment = arrayAlementRef.Parent as IAssignmentOperation;
if (assignment?.Target == arrayAlementRef)
return true;
}

assignment = null;
return flowCaptureReference.IsInLeftOfDeconstructionAssignment (out _);
}
Expand Down
112 changes: 49 additions & 63 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,45 +235,11 @@ TValue ProcessSingleTargetAssignment (IOperation targetOperation, ISimpleAssignm
if (arrayElementRef.Indices.Length != 1)
break;

// Similarly to VisitSimpleAssignment, this needs to handle cases where the array reference
// is a captured variable, even if the target of the assignment (the array element reference) is not.

TValue arrayRef;
TValue index;
TValue value;
if (arrayElementRef.ArrayReference is not IFlowCaptureReferenceOperation captureReference) {
arrayRef = Visit (arrayElementRef.ArrayReference, state);
index = Visit (arrayElementRef.Indices[0], state);
value = Visit (operation.Value, state);
HandleArrayElementWrite (arrayRef, index, value, operation, merge: merge);
return value;
}

index = Visit (arrayElementRef.Indices[0], state);
value = Visit (operation.Value, state);

var capturedReferences = state.Current.CapturedReferences.Get (captureReference.Id);
if (!capturedReferences.HasMultipleValues) {
// Single captured reference. Treat this as an overwriting assignment,
// unless the caller already told us to merge values because this is an
// assignment to one of multiple captured array element references.
var enumerator = capturedReferences.GetEnumerator ();
enumerator.MoveNext ();
var capture = enumerator.Current;
arrayRef = Visit (capture.Reference, state);
HandleArrayElementWrite (arrayRef, index, value, operation, merge: merge);
return value;
}

// The capture id may have captured multiple references, as in:
// (b ? arr1 : arr2)[0] = value;
// We treat this as possible write to each of the captured references,
// which requires merging with the previous values of each.

foreach (var capture in state.Current.CapturedReferences.Get (captureReference.Id)) {
arrayRef = Visit (capture.Reference, state);
HandleArrayElementWrite (arrayRef, index, value, operation, merge: true);
}
TValue arrayRef = Visit (arrayElementRef.ArrayReference, state);
TValue index = Visit (arrayElementRef.Indices[0], state);
TValue value = Visit (operation.Value, state);
HandleArrayElementWrite (arrayRef, index, value, operation, merge: merge);
return value;
}
case IInlineArrayAccessOperation inlineArrayAccess: {
Expand Down Expand Up @@ -371,26 +337,31 @@ public override TValue VisitSimpleAssignment (ISimpleAssignmentOperation operati

TValue GetFlowCaptureValue (IFlowCaptureReferenceOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
{
if (!operation.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Read)) {
// There are known cases where this assert doesn't hold, because LValueFlowCaptureProvider
// produces the wrong result in some cases for flow captures with IsInitialization = true.
// https://github.com/dotnet/linker/issues/2749
// Debug.Assert (IsLValueFlowCapture (operation.Id));
return TopValue;
}

// This assert is incorrect for cases like (b ? arr1 : arr2)[0] = v;
// Here the ValueUsageInfo shows that the value usage is for reading (this is probably wrong!)
// but the value is actually an LValueFlowCapture.
// Let's just disable the assert for now.
// Debug.Assert (IsRValueFlowCapture (operation.Id));
Debug.Assert (!IsLValueFlowCapture (operation.Id),
$"{operation.Syntax.GetLocation ().GetLineSpan ()}");
Debug.Assert (operation.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Read),
$"{operation.Syntax.GetLocation ().GetLineSpan ()}");

return state.Get (new LocalKey (operation.Id));
}

// Similar to VisitLocalReference
public override TValue VisitFlowCaptureReference (IFlowCaptureReferenceOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
{
if (operation.IsInitialization) {
// This capture reference is a temporary byref. This can happen for string
// interpolation handlers: https://github.com/dotnet/roslyn/issues/57484.
// Should really be treated as creating a new l-value flow capture,
// but this is likely irrelevant for dataflow analysis.

// LValueFlowCaptureProvider doesn't take into account IsInitialization = true,
// so it doesn't properly detect this as an l-value capture.
// Context: https://github.com/dotnet/roslyn/issues/60757
// Debug.Assert (IsLValueFlowCapture (operation.Id));
Debug.Assert (operation.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Write),
$"{operation.Syntax.GetLocation ().GetLineSpan ()}");
return TopValue;
}
return GetFlowCaptureValue (operation, state);
}

Expand All @@ -399,26 +370,41 @@ public override TValue VisitFlowCaptureReference (IFlowCaptureReferenceOperation
// is like a local reference.
public override TValue VisitFlowCapture (IFlowCaptureOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
{
// If the captured value is a property reference, we can't easily tell inside of
// VisitPropertyReference whether it is accessed for reads or writes.
// https://github.com/dotnet/roslyn/issues/25057
// Avoid visiting the captured value unless it is an RValue.
if (IsLValueFlowCapture (operation.Id)) {
// Should never see an l-value flow capture of another flow capture.
Debug.Assert (operation.Value is not IFlowCaptureReferenceOperation);
if (operation.Value is IFlowCaptureReferenceOperation)
return TopValue;

// Note: technically we should save some information about the value for LValue flow captures
// (for example, the object instance of a property reference) and avoid re-computing it when
// assigning to the FlowCaptureReference.
var capturedRef = new CapturedReferenceValue (operation.Value);
var currentState = state.Current;
currentState.CapturedReferences.Set (operation.Id, new CapturedReferenceValue (operation.Value));
currentState.CapturedReferences.Set (operation.Id, capturedRef);
state.Current = currentState;
}
return TopValue;
} else {
TValue capturedValue;
if (operation.Value is IFlowCaptureReferenceOperation captureRef) {
if (IsLValueFlowCapture (captureRef.Id)) {
// If an r-value captures an l-value, we must dereference the l-value
// and copy out the value to capture.
capturedValue = TopValue;
foreach (var capturedReference in state.Current.CapturedReferences.Get (captureRef.Id)) {
var value = Visit (capturedReference.Reference, state);
capturedValue = LocalStateLattice.Lattice.ValueLattice.Meet (capturedValue, value);
}
} else {
capturedValue = state.Get (new LocalKey (captureRef.Id));
}
} else {
capturedValue = Visit (operation.Value, state);
}

if (IsRValueFlowCapture (operation.Id)) {
TValue value = Visit (operation.Value, state);
state.Set (new LocalKey (operation.Id), value);
return value;
state.Set (new LocalKey (operation.Id), capturedValue);
return capturedValue;
}

return TopValue;
}

public override TValue VisitExpressionStatement (IExpressionStatementOperation operation, LocalDataFlowState<TValue, TValueLattice> state)
Expand Down
6 changes: 6 additions & 0 deletions src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/DataFlowTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,12 @@ public Task InlineArrayDataflow ()
return RunTest ();
}

[Fact]
public Task InterpolatedStringHandlerDataFlow ()
{
return RunTest ();
}

[Fact]
public Task MakeGenericDataFlow ()
{
Expand Down
118 changes: 118 additions & 0 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ArrayDataFlow.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

using System;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using Mono.Linker.Tests.Cases.Expectations.Assertions;
using Mono.Linker.Tests.Cases.Expectations.Helpers;

Expand Down Expand Up @@ -30,6 +32,7 @@ public static void Main ()
TestArraySetElementAndInitializerMultipleElementsMix<TestType> (typeof (TestType));

TestGetElementAtUnknownIndex ();
TestGetMergedArrayElement ();
TestMergedArrayElementWithUnknownIndex (0);

// Array reset - certain operations on array are not tracked fully (or impossible due to unknown inputs)
Expand All @@ -47,6 +50,8 @@ public static void Main ()

WriteCapturedArrayElement.Test ();

WriteElementOfCapturedArray.Test ();

ConstantFieldValuesAsIndex.Test ();

HoistedArrayMutation.Test ();
Expand Down Expand Up @@ -205,6 +210,23 @@ static void TestGetElementAtUnknownIndex (int i = 0)
arr[i].RequiresPublicFields ();
}

// https://github.com/dotnet/runtime/issues/93416 tracks the discrepancy between
// the analyzer and ILLink/ILCompiler.
[ExpectedWarning ("IL2062", nameof (DataFlowTypeExtensions.RequiresAll),
ProducedBy = Tool.Trimmer | Tool.NativeAot)]
[ExpectedWarning ("IL2072", nameof (GetMethods), nameof (DataFlowTypeExtensions.RequiresAll),
ProducedBy = Tool.Analyzer)]
[ExpectedWarning ("IL2072", nameof (GetFields), nameof (DataFlowTypeExtensions.RequiresAll),
ProducedBy = Tool.Analyzer)]
static void TestGetMergedArrayElement (bool b = true)
{
Type[] arr = new Type[] { GetMethods () };
Type[] arr2 = new Type[] { GetFields () };
if (b)
arr = arr2;
arr[0].RequiresAll ();
}

// Trimmer code doesnt handle locals from different branches separetely, therefore merges incorrectly GetMethods with Unknown producing both warnings
[ExpectedWarning ("IL2072", nameof (ArrayDataFlow.GetMethods), ProducedBy = Tool.Trimmer | Tool.NativeAot)]
[ExpectedWarning ("IL2062", nameof (DataFlowTypeExtensions.RequiresAll))]
Expand Down Expand Up @@ -614,6 +636,102 @@ public static void Test ()
}
}

class WriteElementOfCapturedArray
{
[Kept]
[ExpectedWarning ("IL2072", nameof (GetUnknownType), nameof (DataFlowTypeExtensions.RequiresAll))]
[ExpectedWarning ("IL2072", nameof (GetTypeWithPublicConstructors), nameof (DataFlowTypeExtensions.RequiresAll))]
// Analysis hole: https://github.com/dotnet/runtime/issues/90335
// The array element assignment assigns to a temp array created as a copy of
// arr1 or arr2, and writes to it aren't reflected back in arr1/arr2.
static void TestArrayElementAssignment (bool b = true)
{
var arr1 = new Type[] { GetUnknownType () };
var arr2 = new Type[] { GetTypeWithPublicConstructors () };
(b ? arr1 : arr2)[0] = GetWithPublicMethods ();
arr1[0].RequiresAll ();
arr2[0].RequiresAll ();
}

[Kept]
[KeptAttributeAttribute (typeof (InlineArrayAttribute))]
[InlineArray (8)]
public struct InlineTypeArray
{
[Kept]
public Type t;
}

[Kept]
[ExpectedWarning ("IL2062", nameof (DataFlowTypeExtensions.RequiresAll))]
[ExpectedWarning ("IL2062", nameof (DataFlowTypeExtensions.RequiresAll))]
static void TestInlineArrayElementReferenceAssignment (bool b = true)
{
var arr1 = new InlineTypeArray ();
arr1[0] = GetUnknownType ();
var arr2 = new InlineTypeArray ();
arr2[0] = GetTypeWithPublicConstructors ();
(b ? ref arr1[0] : ref arr2[0]) = GetTypeWithPublicConstructors ();
arr1[0].RequiresAll ();
arr2[0].RequiresAll ();
}

// Inline array references are not allowed in conditionals, unlike array references.
// static void TestInlineArrayElementAssignment (bool b = true)
// {
// var arr1 = new InlineTypeArray ();
// arr1[0] = GetUnknownType ();
// var arr2 = new InlineTypeArray ();
// arr2[0] = GetTypeWithPublicConstructors ();
// (b ? arr1 : arr2)[0] = GetWithPublicMethods ();
// arr1[0].RequiresAll ();
// arr2[0].RequiresAll ();
// }

[ExpectedWarning ("IL2087", nameof (T), nameof (DataFlowTypeExtensions.RequiresAll))]
[ExpectedWarning ("IL2087", nameof (U), nameof (DataFlowTypeExtensions.RequiresPublicFields))]
// Missing warnings for 'V' possibly assigned to arr or arr2 because write to temp
// array isn't reflected back in the local variables. https://github.com/dotnet/linker/issues/2158
static void TestNullCoalesce<T, U, V> (bool b = false)
{
Type[]? arr = new Type[1] { typeof (T) };
Type[] arr2 = new Type[1] { typeof (U) };

(arr ?? arr2)[0] = typeof (V);
arr[0].RequiresAll ();
arr2[0].RequiresPublicFields ();
}

[ExpectedWarning ("IL2087", nameof (T), nameof (DataFlowTypeExtensions.RequiresAll), ProducedBy = Tool.Analyzer)]
[ExpectedWarning ("IL2087", nameof (U), nameof (DataFlowTypeExtensions.RequiresPublicFields))]
// Missing warnings for 'V' possibly assigned to arr or arr2 because write to temp
// array isn't reflected back in the local variables. https://github.com/dotnet/linker/issues/2158
// This also causes an extra analyzer warning for 'U' in 'arr', because the analyzer models the
// possible assignment of arr2 to arr, without overwriting index '0'. And it produces a warning
// for each possible value, unlike ILLink/ILCompiler, which produce an unknown value for a merged
// array value: https://github.com/dotnet/runtime/issues/93416
[ExpectedWarning ("IL2087", nameof (U), nameof (DataFlowTypeExtensions.RequiresAll), ProducedBy = Tool.Analyzer)]
[ExpectedWarning ("IL2062", nameof (DataFlowTypeExtensions.RequiresAll), ProducedBy = Tool.Trimmer | Tool.NativeAot)]
static void TestNullCoalescingAssignment<T, U, V> (bool b = true)
{
Type[]? arr = new Type[1] { typeof (T) };
Type[] arr2 = new Type[1] { typeof (U) };

(arr ??= arr2)[0] = typeof (V);
arr[0].RequiresAll ();
arr2[0].RequiresPublicFields ();
}

public static void Test ()
{
TestArrayElementAssignment ();
TestInlineArrayElementReferenceAssignment ();
// TestInlineArrayElementAssignment ();
TestNullCoalesce<int, int, int> ();
TestNullCoalescingAssignment<int, int, int> ();
}
}

class ConstantFieldValuesAsIndex
{
private const sbyte ConstSByte = 1;
Expand Down
Loading

0 comments on commit e1e18ee

Please sign in to comment.