Warn on message formatting failure instead of crash, and fix Native AOT MakeGenericType failure #110330
Conversation
dotnet-issue-labeler
bot
added
the
area-Tools-ILLink
dotnet-policy-service
bot
added
the
linkable-framework
jtschuster
added
NO-REVIEW
dotnet-policy-service
bot
added
the
linkable-framework
- Wrap message formatting in try/catch and give warning if format fails - Guard MakeGenericMethodSite dependency creation with call to CheckConstraints
jtschuster
force-pushed
the
TestNAOTFailure
branch
from
00717fe to
ae67ab6
Compare
jtschuster
changed the title
jtschuster
added
area-NativeAOT-coreclr
area-Tools-ILLink
jtschuster
requested review from
marek-safar and
MichalStrehovsky
as code owners
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs
Outdated
Show resolved
Hide resolved
| string formattedString; | ||
| try | ||
| { | ||
| formattedString = string.Format(format, args); | ||
| } | ||
| catch (FormatException) | ||
| { | ||
| #pragma warning disable RS1035 // Do not use APIs banned for analyzers - We just need a newline | ||
| formattedString = "Internal error formatting diagnostic. Please report the issue at https://aka.ms/report-illink" + Environment.NewLine | ||
| + $"'{format}', " + $"[{string.Join(", ", args)}]"; | ||
| #pragma warning restore RS1035 // Do not use APIs banned for analyzers | ||
| } | ||
| return formattedString; |
There was a problem hiding this comment.
We do resource string formatting all over the product but I haven't seen a place that would have a try/catch to transform internal errors to different internal errors. Is there more context on this?
Unhandled exceptions are often logged in various crash telemetries (not that I know of anyone actually looking at it in the past years, but in principle). This will swallow the possible telemetry.
There was a problem hiding this comment.
This string formatting caused errors since the number of arguments expected and provided don't always match for DAM warnings when Nullable is the source of the value. The proper fix to avoid the exception is in #110229, after which we could remove this try/catch. It still hits this exception in this PR because I wanted to separate the NativeAOT fix MakeGenericType fix and the DAM warning/arguments mismatch fix.
We could remove the tests and the try/catch from #110229 before merging this PR if you'd rather avoid merging the try/catch.
There was a problem hiding this comment.
Yes, I think this PR can just be about the HandleCallAction, it's fine without test since we'll get the test.
There was a problem hiding this comment.
Reverted the test and formatting changes.
The tests in #110229 exposed an issue in NativeAOT around MakeGenericType/Method. ILC doesn't make sure that an instantiation from MakeGenericType/Method fulfills the constraints of the generic type/method, which can create to nonsense types that fail asserts in other places. The fix is to ensure that the constraints are valid before determining the dependencies of the instantiated generic.
This PR also includes the formatting try/catch in #110229 so that the tests that expose the issue can run without crashing the trimmer.
Fixes #109536