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

Jump to bottom

Reference live ILLink analyzer #93259

Merged
merged 14 commits into from
Oct 20, 2023
Merged

Reference live ILLink analyzer #93259

merged 14 commits into from
Oct 20, 2023

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Oct 9, 2023

In #91233 I inadvertently turned off the trim analyzer for libraries, because the analyzer bits don't flow to the consuming project with the ILLink.Tasks ProjectReference. This adds a separate ProjectReference to use the live analyzer. This addresses #88901 for the ILLink analyzers.

Using the live analyzer uncovered some bugs that are fixed in this change:

  • Invalid assert for field initializers with throw statements
  • Invalid cast to IMethodSymbol for the ContainingSymbol of a local. Locals can occur in field initializers, when created from out params.
  • Wrong warning code produced for assignment to annotated property in an initializer. This was being treated as a call to the setter instead of an assignment to the underlying field:
    Type AnnotatedPropertyWithSetter { get; set; } = GetUnknown ();
    (IIRC I hit this one when investigating some related behavior, not via a new warning produced when analyzing runtime)
  • Invalid assert for delegate creation where the delegate is created from anything other than a method (for example, a field that has an Action or Func type).

@sbomer
Reference live ILLink.RoslynAnalyzer
cdc1149 
- Fix some analyzer asserts
- Reference local analyzer
- Fix property initializer behavior
@sbomer
Add tests for delegate creation assert
adb72b7
@ghost ghost assigned sbomer Oct 9, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Oct 9, 2023
@ghost
Copy link

ghost commented Oct 9, 2023

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

Issue Details

In #91233 I inadvertently turned off the trim analyzer for libraries, because the analyzer bits don't flow to the consuming project with the ILLink.Tasks ProjectReference. This adds a separate ProjectReference to use the live analyzer. This addresses #88901 for the ILLink analyzers.

Using the live analyzer uncovered some bugs that are fixed in this change:

  • Invalid assert for field initializers with throw statements
  • Invalid cast to IMethodSymbol for the ContainingSymbol of a local. Locals can occur in field initializers, when created from out params.
  • Wrong warning code produced for assignment to annotated property in an initializer. This was being treated as a call to the setter instead of an assignment to the underlying field:
    Type AnnotatedPropertyWithSetter { get; set; } = GetUnknown ();
    (IIRC I hit this one when investigating some related behavior, not via a new warning produced when analyzing runtime)
  • Invalid assert for delegate creation where the delegate is created from anything other than a method (for example, a field that has an Action or Func type).
Author: sbomer
Assignees: sbomer
Labels:

area-Tools-ILLink
Milestone: -

This was referenced Oct 10, 2023
Closed
Closed
@sbomer sbomer mentioned this pull request Oct 12, 2023
Merged
@sbomer
Copy link
Member Author

sbomer commented Oct 12, 2023

CI uncovered another assert, which I'm fixing separately in #93420.

sbomer added a commit that referenced this pull request Oct 18, 2023
@sbomer
Fix analyzer treatment of flow captures of arrays (#93420)
e1e18ee 
#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.
@sbomer
Copy link
Member Author

sbomer commented Oct 19, 2023

I fixed a few more asserts that came up after #93420:

  • Assert inside GetFlowCaptureValue for the LHS of a compound assignment. I couldn't think of a case yet where we actually need to model the result of the assignment, so I implemented support for at least visiting the LHS by going through the same path as normal assignments (avoiding GetFlowCaptureValue).
  • Assert inside GetFlowCaptureValue for a flow capture reference passed as an out param.
  • Added a test for deconstruction assignment to flow capture reference.

This was referenced Oct 20, 2023
Closed
Closed
ViktorHofer
ViktorHofer approved these changes Oct 20, 2023
View reviewed changes
Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Also verified that this change doesn't require updating our solution files again in src/libraries.

@sbomer
Copy link
Member Author

sbomer commented Oct 20, 2023

Test failure looks like #90019

@sbomer sbomer merged commit de011df into dotnet:main Oct 20, 2023
186 of 191 checks passed
@sbomer sbomer mentioned this pull request Oct 21, 2023
Merged
@sbomer sbomer deleted the liveAnalyzer branch November 3, 2023 18:39
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vitek-karas vitek-karas vitek-karas approved these changes

@ViktorHofer ViktorHofer ViktorHofer approved these changes

@agocke agocke Awaiting requested review from agocke

@marek-safar marek-safar Awaiting requested review from marek-safar marek-safar is a code owner

Assignees

@sbomer sbomer

Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sbomer @ViktorHofer @vitek-karas