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.

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

[Port] Fix the incorrect merge logic for back edges for GlobalFlowStateAnalysis #6038

Merged
merged 3 commits into from Jun 30, 2022

Conversation

mavasani
Copy link
Member

@mavasani mavasani commented Jun 28, 2022

Ports #6029 to .NET6 release branch.

Customer Impact

A customer reported the issue in VS feedbacks which transferred to github: #6015 and it is reproduceable with the linked project.

When builds the project with CA1416 Platform Compatibility Analyzer enabled then the build never ends and memory grows continually, even stopping the build and closed the VS the VBCSCompiler process stays alive and memory keep growing, only option is kill the process

CA1416 is included in SDK and enabled by default, therefore it could happen for any customer's project that happens to have a code section that reproes the issue.

Testing

  • Unit tested verified that prior to this fix, the added unit test hang due to this issue.
  • Manually tested with the repro: verified that building the github repo cited in the feedback ticket with platform compat analyzer also leads to a hang before this fix. Both these repros are fixed after this PR.

Risk

Very low - GlobalFlowStateAnalysisValueSet.Unset is used to represent unanalyzed flow state, GlobalFlowStateAnalysisValueSet.Empty is used to represent analyzed flow state with no analysis values. We initialize the flow states for the entry basic block of the CFG with Unset value, but never replace the Unset with Empty for subsequent passes. This in turn leads to incorrect merge when a Known value flows from the back edge to the loop start and ends up overriding the Unset value instead of being overridden by the Empty value. Now we correctly perform this merge logic.

Fixes dotnet#6015

`GlobalFlowStateAnalysisValueSet.Unset` is used to represent unanalyzed flow state, `GlobalFlowStateAnalysisValueSet.Empty` is used to represent analyzed flow state with no analysis values. We initialize the flow states for the entry basic block of the CFG with `Unset` value, but never replace the `Unset` with `Empty` for subsequent passes. This in turn leads to incorrect merge when a `Known` value flows from the back edge to the loop start and ends up overriding the `Unset` value instead of being overridden by the `Empty` value. Now we correctly perform this merge logic.

I verified that prior to this fix, the added unit test hang due to this issue. I also verified that building the github repo cited in the feedback ticket with platform compat analyzer also leads to a hang before this fix. Both these repros are fixed after this PR.
@mavasani mavasani requested a review from a team as a code owner June 28, 2022 09:01
@mavasani
Copy link
Member Author

mavasani commented Jun 28, 2022

@JoeRobich @jmarolf - seems like we need to port #6032 to release/6.0.4xx branch to fix the build errors about duplicate package references.

@jeffhandley
Copy link
Member

jeffhandley commented Jun 28, 2022

Can those changes be merged into this branch/PR since otherwise they wouldn't need to be ported?

@mavasani
Copy link
Member Author

Thanks @JoeRobich!

@buyaa-n
Copy link
Member

buyaa-n commented Jun 28, 2022

Thanks @mavasani I have updated the description by the 6.0 servicing template, please check and updates as needed, especially the Risk part might need more update

@mavasani
Copy link
Member Author

Thanks @buyaa-n - I believe your description is appropriate, we don't need further updates.

@mavasani
Copy link
Member Author

@JoeRobich @jmarolf Do you know what is causing the build failures? Are we using the correct .NET SDK/compiler for the build?

.dotnet\sdk\6.0.100-rc.1.21430.12\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets#L1088
.dotnet\sdk\6.0.100-rc.1.21430.12\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(1088,5): error MSB4018: (NETCORE_ENGINEERING_TELEMETRY=Build) The "ValidateExecutableReferences" task failed unexpectedly.
System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.NET.Build.Tasks.ValidateExecutableReferences.ExecuteCore()
   at Microsoft.NET.Build.Tasks.TaskBase.Execute()
   at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute()
   at Microsoft.Build.BackEnd.TaskBuilder.<ExecuteInstantiatedTask>d__26.MoveNext()

@codecov
Copy link

codecov bot commented Jun 29, 2022

Codecov Report

Merging #6038 (a7abd5e) into release/6.0.4xx (2b525af) will increase coverage by 0.00%.
The diff coverage is 92.10%.

@@               Coverage Diff                @@
##           release/6.0.4xx    #6038   +/-   ##
================================================
  Coverage            95.53%   95.53%           
================================================
  Files                 1275     1275           
  Lines               292697   292730   +33     
  Branches             17686    17688    +2     
================================================
+ Hits                279634   279671   +37     
- Misses               10655    10657    +2     
+ Partials              2408     2402    -6     

@buyaa-n buyaa-n added this to the .NET 6.x milestone Jun 29, 2022
@buyaa-n
Copy link
Member

buyaa-n commented Jun 29, 2022

Thanks @mavasani @JoeRobich the 6.0 servicing request approved over e-mail. Feel free to merge!

@mavasani mavasani merged commit a95ffe8 into dotnet:release/6.0.4xx Jun 30, 2022
@mavasani mavasani deleted the PortFix branch June 30, 2022 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants