Skip to content

R2RDump fixes #32460

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

Merged
merged 1 commit into from
Feb 18, 2020
Merged

R2RDump fixes #32460

merged 1 commit into from
Feb 18, 2020

Conversation

cshung
Copy link
Contributor

@cshung cshung commented Feb 18, 2020

Fixes #966

This is just a reaction on a recent change to R2RDump that makes it fits for the ILSpy scenario.

  1. Avoid Console.WriteLine() in ILCompiler.Reflection.ReadyToRun. I am pretty sure these should be thrown as an exception instead.
  2. Avoid using the null coalescing assignment operator. The operator is only available in C# 8.0, which is not fully compatible with .NET standard 2.0 as required by ILSpy's use.

There are a few reasons why I think it is a good idea to make warning exceptions instead:

  1. When we see unexpected input, it is unclear if moving on will generate any good output.
  2. R2RDump output tends to be verbose, a warning hidden in the output file often go unnoticeable.
  3. Making it exception forces whoever broke it to fix it fast.
  4. We do not want to see Console.WriteLine in ILSpy.

@cshung cshung added the area-R2RDump-coreclr Ready-to-run image dump tool label Feb 18, 2020
@cshung cshung requested a review from trylek February 18, 2020 00:21
Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@cshung cshung merged commit 2d6b77f into dotnet:master Feb 18, 2020
@cshung cshung deleted the dev/andrewau/r2rdump-fixes branch February 18, 2020 02:33
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-R2RDump-coreclr Ready-to-run image dump tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle the warnings in R2RDump
2 participants