Fix analyzer treatment of flow captures of arrays #93420

sbomer · 2023-10-12T21:52:09Z

#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

runtime/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs

Line 511 in 946c524

(handlesToDispose ??= new MemoryHandle[buffersCount])[i] = handle;

Now tested in TestNullCoalescingAssignment:

(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.

flowchart TD
style 0 text-align: left;
0("<code></code>")
style 1 text-align: left;
1("<code>&nbsp;&nbsp;LocalReferenceOperation: arr = new Type[1] { typeof (T) }
&nbsp;&nbsp;&nbsp;&nbsp;LiteralOperation: 1
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;TypeOfOperation: typeof (T)
&nbsp;&nbsp;&nbsp;&nbsp;ArrayInitializerOperation: { typeof (T) }
&nbsp;&nbsp;ArrayCreationOperation: new Type[1] { typeof (T) }
SimpleAssignmentOperation: arr = new Type[1] { typeof (T) }
&nbsp;&nbsp;LocalReferenceOperation: arr2 = new Type[1] { typeof (U) }
&nbsp;&nbsp;&nbsp;&nbsp;LiteralOperation: 1
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;TypeOfOperation: typeof (U)
&nbsp;&nbsp;&nbsp;&nbsp;ArrayInitializerOperation: { typeof (U) }
&nbsp;&nbsp;ArrayCreationOperation: new Type[1] { typeof (U) }
SimpleAssignmentOperation: arr2 = new Type[1] { typeof (U) }
</code>")
0 --> 1
style 2 text-align: left;
2("<code>&nbsp;&nbsp;LocalReferenceOperation: arr
FlowCaptureOperation: arr (0)
</code>")
1 --> 2
style 3 text-align: left;
3("<code>&nbsp;&nbsp;FlowCaptureReferenceOperation: arr (0)
FlowCaptureOperation: arr (1)
&nbsp;&nbsp;FlowCaptureReferenceOperation: arr (1)
IsNullOperation: arr
</code>")
2 --> 3
style 4 text-align: left;
4("<code>&nbsp;&nbsp;FlowCaptureReferenceOperation: arr (1)
FlowCaptureOperation: arr ??= arr2 (2)
</code>")
3 --> 4
style 5 text-align: left;
5("<code>&nbsp;&nbsp;&nbsp;&nbsp;FlowCaptureReferenceOperation: arr (0)
&nbsp;&nbsp;&nbsp;&nbsp;LocalReferenceOperation: arr2
&nbsp;&nbsp;SimpleAssignmentOperation: arr ??= arr2
FlowCaptureOperation: arr ??= arr2 (2)
</code>")
3 --> 5
style 6 text-align: left;
6("<code>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;FlowCaptureReferenceOperation: arr ??= arr2 (2)
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;LiteralOperation: 0
&nbsp;&nbsp;&nbsp;&nbsp;ArrayElementReferenceOperation: (arr ??= arr2)[0]
&nbsp;&nbsp;&nbsp;&nbsp;TypeOfOperation: typeof (V)
&nbsp;&nbsp;SimpleAssignmentOperation: (arr ??= arr2)[0] = typeof (V)
ExpressionStatementOperation: (arr ??= arr2)[0] = typeof (V);
</code>")
4 --> 6
5 --> 6
style 7 text-align: left;
7("<code>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;LocalReferenceOperation: arr
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;LiteralOperation: 0
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;ArrayElementReferenceOperation: arr[0]
&nbsp;&nbsp;&nbsp;&nbsp;ArgumentOperation: arr[0]
&nbsp;&nbsp;InvocationOperation: arr[0].RequiresAll ()
ExpressionStatementOperation: arr[0].RequiresAll ();
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;LocalReferenceOperation: arr2
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;LiteralOperation: 0
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;ArrayElementReferenceOperation: arr2[0]
&nbsp;&nbsp;&nbsp;&nbsp;ArgumentOperation: arr2[0]
&nbsp;&nbsp;InvocationOperation: arr2[0].RequiresPublicFields ()
ExpressionStatementOperation: arr2[0].RequiresPublicFields ();
</code>")
6 --> 7
style 8 text-align: left;
8("<code></code>")
7 --> 8

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.

ghost · 2023-10-12T21:52:31Z

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

#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

runtime/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs

Line 511 in 946c524

(handlesToDispose ??= new MemoryHandle[buffersCount])[i] = handle;

Now tested in TestNullCoalescingAssignment:

(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 0". This is where the assert was hit.

flowchart TD
style 0 text-align: left;
0("<code></code>")
style 1 text-align: left;
1("<code>&nbsp;&nbsp;LocalReferenceOperation: arr = new Type[1] { typeof (T) }
&nbsp;&nbsp;&nbsp;&nbsp;LiteralOperation: 1
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;TypeOfOperation: typeof (T)
&nbsp;&nbsp;&nbsp;&nbsp;ArrayInitializerOperation: { typeof (T) }
&nbsp;&nbsp;ArrayCreationOperation: new Type[1] { typeof (T) }
SimpleAssignmentOperation: arr = new Type[1] { typeof (T) }
&nbsp;&nbsp;LocalReferenceOperation: arr2 = new Type[1] { typeof (U) }
&nbsp;&nbsp;&nbsp;&nbsp;LiteralOperation: 1
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;TypeOfOperation: typeof (U)
&nbsp;&nbsp;&nbsp;&nbsp;ArrayInitializerOperation: { typeof (U) }
&nbsp;&nbsp;ArrayCreationOperation: new Type[1] { typeof (U) }
SimpleAssignmentOperation: arr2 = new Type[1] { typeof (U) }
</code>")
0 --> 1
style 2 text-align: left;
2("<code>&nbsp;&nbsp;LocalReferenceOperation: arr
FlowCaptureOperation: arr (0)
</code>")
1 --> 2
style 3 text-align: left;
3("<code>&nbsp;&nbsp;FlowCaptureReferenceOperation: arr (0)
FlowCaptureOperation: arr (1)
&nbsp;&nbsp;FlowCaptureReferenceOperation: arr (1)
IsNullOperation: arr
</code>")
2 --> 3
style 4 text-align: left;
4("<code>&nbsp;&nbsp;FlowCaptureReferenceOperation: arr (1)
FlowCaptureOperation: arr ??= arr2 (2)
</code>")
3 --> 4
style 5 text-align: left;
5("<code>&nbsp;&nbsp;&nbsp;&nbsp;FlowCaptureReferenceOperation: arr (0)
&nbsp;&nbsp;&nbsp;&nbsp;LocalReferenceOperation: arr2
&nbsp;&nbsp;SimpleAssignmentOperation: arr ??= arr2
FlowCaptureOperation: arr ??= arr2 (2)
</code>")
3 --> 5
style 6 text-align: left;
6("<code>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;FlowCaptureReferenceOperation: arr ??= arr2 (2)
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;LiteralOperation: 0
&nbsp;&nbsp;&nbsp;&nbsp;ArrayElementReferenceOperation: (arr ??= arr2)[0]
&nbsp;&nbsp;&nbsp;&nbsp;TypeOfOperation: typeof (V)
&nbsp;&nbsp;SimpleAssignmentOperation: (arr ??= arr2)[0] = typeof (V)
ExpressionStatementOperation: (arr ??= arr2)[0] = typeof (V);
</code>")
4 --> 6
5 --> 6
style 7 text-align: left;
7("<code>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;LocalReferenceOperation: arr
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;LiteralOperation: 0
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;ArrayElementReferenceOperation: arr[0]
&nbsp;&nbsp;&nbsp;&nbsp;ArgumentOperation: arr[0]
&nbsp;&nbsp;InvocationOperation: arr[0].RequiresAll ()
ExpressionStatementOperation: arr[0].RequiresAll ();
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;LocalReferenceOperation: arr2
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;LiteralOperation: 0
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;ArrayElementReferenceOperation: arr2[0]
&nbsp;&nbsp;&nbsp;&nbsp;ArgumentOperation: arr2[0]
&nbsp;&nbsp;InvocationOperation: arr2[0].RequiresPublicFields ()
ExpressionStatementOperation: arr2[0].RequiresPublicFields ();
</code>")
6 --> 7
style 8 text-align: left;
8("<code></code>")
7 --> 8

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.

Author:	sbomer
Assignees:	-
Labels:	`area-Tools-ILLink`
Milestone:	-

sbomer · 2023-10-12T21:58:25Z

src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs

@@ -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


This logic is no longer needed now that the array reference should never be an l-value capture. If the array reference is a flow capture, it should be an r-value capture, handled in the normal visitor logic.

By this you mean, that the array (LHS) part of the ArrayReference should be an rvalue, right? Not the thing as a whole?

Exactly - the (arr ?? arr2) temp should be an r-value, while (arr ?? arr2)[0] should be an l-value. At least that's how I think it should work after pondering this for a while, and is the behavior in the PR. ;)

sbomer · 2023-10-12T22:02:56Z

src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ArrayDataFlow.cs

+			// 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)


Test moved from ByRefDataflow. Note this introduces a behavior change: analyzer no longer produces warnings for the GetWithPublicMethods value, bringing it closer to the ILLink/ILCompiler behavior. Once we fix the tracking of reference types, it should fix all three.

vitek-karas

I must admit this is a bit too much compiler theory for me :-)

I think it does make sense, but it would be really good if @agocke could also take a look at this.

src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs

agocke · 2023-10-17T20:48:29Z

Looking....

Also, cool Mermaid graph :)

agocke

This LGTM, but I can't promise I'm not missing something. Abstractly, "lifting into a temporary" seems like it can encompass a lot of things. I'm not sure I've thought of all the cases.

agocke · 2023-10-17T21:29:42Z

src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs

@@ -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


By this you mean, that the array (LHS) part of the ArrayReference should be an rvalue, right? Not the thing as a whole?

sbomer · 2023-10-18T17:45:11Z

The browser-wasm monointerpreter leg is hitting #93134.

Fix analyzer treatment of flow captures of arrays

01dc9b7

sbomer requested a review from marek-safar as a code owner October 12, 2023 21:52

dotnet-issue-labeler bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Oct 12, 2023

ghost added the linkable-framework Issues associated with delivering a linker friendly framework label Oct 12, 2023

ghost assigned sbomer Oct 12, 2023

sbomer commented Oct 12, 2023

View reviewed changes

sbomer added 2 commits October 12, 2023 22:04

Add back comment

daca428

Fix expected warning

3627e69

sbomer requested review from agocke and vitek-karas October 12, 2023 22:08

sbomer mentioned this pull request Oct 12, 2023

Reference live ILLink analyzer #93259

Merged

vitek-karas reviewed Oct 13, 2023

View reviewed changes

src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs Outdated Show resolved Hide resolved

Add interpolated string handler test, fix asserts

c5c4aeb

sbomer mentioned this pull request Oct 17, 2023

Analyzer assert on interpolation handler in new versions of Roslyn dotnet/linker#2749

Closed

Keep disabled assert as comment, add assert location

f0726d2

agocke approved these changes Oct 17, 2023

View reviewed changes

sbomer merged commit e1e18ee into dotnet:main Oct 18, 2023
59 of 62 checks passed

sbomer deleted the fixArrayCapture branch November 3, 2023 18:39

ghost locked as resolved and limited conversation to collaborators Dec 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix analyzer treatment of flow captures of arrays #93420

Fix analyzer treatment of flow captures of arrays #93420

sbomer commented Oct 12, 2023 •

edited

Loading

ghost commented Oct 12, 2023

sbomer Oct 12, 2023

agocke Oct 17, 2023

sbomer Oct 17, 2023

sbomer Oct 12, 2023

vitek-karas left a comment

agocke commented Oct 17, 2023

agocke left a comment

agocke Oct 17, 2023

sbomer commented Oct 18, 2023

Fix analyzer treatment of flow captures of arrays #93420

Fix analyzer treatment of flow captures of arrays #93420

Conversation

sbomer commented Oct 12, 2023 • edited Loading

ghost commented Oct 12, 2023

sbomer Oct 12, 2023

Choose a reason for hiding this comment

agocke Oct 17, 2023

Choose a reason for hiding this comment

sbomer Oct 17, 2023

Choose a reason for hiding this comment

sbomer Oct 12, 2023

Choose a reason for hiding this comment

vitek-karas left a comment

Choose a reason for hiding this comment

agocke commented Oct 17, 2023

agocke left a comment

Choose a reason for hiding this comment

agocke Oct 17, 2023

Choose a reason for hiding this comment

sbomer commented Oct 18, 2023

sbomer commented Oct 12, 2023 •

edited

Loading