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

Warn on message formatting failure instead of crash, and fix Native AOT MakeGenericType failure #110330

Merged
merged 9 commits into from
Jan 2, 2025
Merged

Warn on message formatting failure instead of crash, and fix Native AOT MakeGenericType failure #110330

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ae67ab6
Fix crash in message formatting with try/catch and fix NAOT failure.
jtschuster Dec 2, 2024
4aeb116
Merge branch 'main' of https://github.com/dotnet/runtime into TestNAO…
jtschuster Dec 3, 2024
c883da8
Remove extra using
jtschuster Dec 3, 2024
825344e
Extract instantiation to a local for readability
jtschuster Dec 4, 2024
18c20b8
Merge branch 'main' into TestNAOTFailure
jtschuster Dec 9, 2024
999fc82
Merge branch 'main' into TestNAOTFailure
jtschuster Dec 10, 2024
eae0efd
Remove test and formatting exception handling
jtschuster Dec 11, 2024
1aaac68
Merge branch 'TestNAOTFailure' of https://github.com/jtschuster/runti…
jtschuster Dec 11, 2024
826c7ee
Merge branch 'main' into TestNAOTFailure
jtschuster Dec 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,7 +711,8 @@ private sealed class MakeGenericMethodSite : INodeWithRuntimeDeterminedDependenc
public IEnumerable<DependencyNodeCore<NodeFactory>.DependencyListEntry> InstantiateDependencies(NodeFactory factory, Instantiation typeInstantiation, Instantiation methodInstantiation)
{
var list = new DependencyList();
RootingHelpers.TryGetDependenciesForReflectedMethod(ref list, factory, _method.InstantiateSignature(typeInstantiation, methodInstantiation), "MakeGenericMethod");
if (_method.InstantiateSignature(typeInstantiation, methodInstantiation).CheckConstraints(new InstantiationContext(typeInstantiation, methodInstantiation)))
jtschuster marked this conversation as resolved.
Show resolved Hide resolved
RootingHelpers.TryGetDependenciesForReflectedMethod(ref list, factory, _method.InstantiateSignature(typeInstantiation, methodInstantiation), "MakeGenericMethod");
return list;
}
}
Expand All @@ -725,7 +726,8 @@ private sealed class MakeGenericTypeSite : INodeWithRuntimeDeterminedDependencie
public IEnumerable<DependencyNodeCore<NodeFactory>.DependencyListEntry> InstantiateDependencies(NodeFactory factory, Instantiation typeInstantiation, Instantiation methodInstantiation)
{
var list = new DependencyList();
RootingHelpers.TryGetDependenciesForReflectedType(ref list, factory, _type.InstantiateSignature(typeInstantiation, methodInstantiation), "MakeGenericType");
if (_type.InstantiateSignature(typeInstantiation, methodInstantiation).CheckConstraints(new InstantiationContext(typeInstantiation, methodInstantiation)))
RootingHelpers.TryGetDependenciesForReflectedType(ref list, factory, _type.InstantiateSignature(typeInstantiation, methodInstantiation), "MakeGenericType");
return list;
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/tools/illink/src/ILLink.Shared/DiagnosticString.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@ public DiagnosticString (string diagnosticResourceStringName)
}

public string GetMessage (params string[] args) =>
string.Format (_messageFormat, args);
MessageFormat.TryFormat (_messageFormat, args);

public string GetMessageFormat () => _messageFormat;

public string GetTitle (params string[] args) =>
string.Format (_titleFormat, args);
MessageFormat.TryFormat (_titleFormat, args);

public string GetTitleFormat () => _titleFormat;
}
Expand Down
21 changes: 20 additions & 1 deletion src/tools/illink/src/ILLink.Shared/MessageFormat.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
// This is needed due to NativeAOT which doesn't enable nullable globally yet
#nullable enable

using System;

namespace ILLink.Shared
{
internal static class MessageFormat
Expand Down Expand Up @@ -33,7 +35,24 @@ public static string FormatRequiresAttributeMismatch (bool memberHasAttribute, b
(true, false) => SharedStrings.DerivedRequiresMismatchMessage
};

return string.Format (format, args);
return TryFormat (format, args);
}

public static string TryFormat (string format, params object[] args)
{
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;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think this PR can just be about the HandleCallAction, it's fine without test since we'll get the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted the test and formatting changes.

}
}
}
63 changes: 63 additions & 0 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/NullableAnnotations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ public static void Main ()
GetUnderlyingTypeOnNonNullableKnownType.Test ();
MakeGenericTypeWithUnknownValue (new object[2] { 1, 2 });
MakeGenericTypeWithKnowAndUnknownArray ();
RequiresOnNullableMakeGenericType.Test();
}

[Kept]
Expand Down Expand Up @@ -97,6 +98,68 @@ static void RequireAllFromMadeGenericNullableOfTypeWithMethodWithRuc ()
typeof (Nullable<>).MakeGenericType (typeof (TestStructWithRucMethod)).RequiresAll ();
}

public class RequiresOnNullableMakeGenericType
{
[Kept]
static Type UnannotatedField;
[Kept]
[KeptAttributeAttribute(typeof(DynamicallyAccessedMembersAttribute))]
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
static Type FieldWithMethods;
[Kept]
[UnexpectedWarning("IL2090", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/93800")]
static void Field()
{
typeof (Nullable<>).MakeGenericType (UnannotatedField).GetMethods ();
typeof (Nullable<>).MakeGenericType (FieldWithMethods).GetMethods ();
}

[Kept]
[UnexpectedWarning("IL2090", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/93800")]
static void Parameter(
Type unannotated,
[KeptAttributeAttribute(typeof(DynamicallyAccessedMembersAttribute))]
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type annotated)
{
typeof (Nullable<>).MakeGenericType (unannotated).GetMethods ();
typeof (Nullable<>).MakeGenericType (annotated).GetMethods ();
}

[Kept]
[ExpectedWarning("IL2090", "TUnannotated")]
static void TypeParameter<
TUnannotated,
[KeptAttributeAttribute(typeof(DynamicallyAccessedMembersAttribute))]
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] TAnnotated>()
{
typeof (Nullable<>).MakeGenericType (typeof(TUnannotated)).GetMethods ();
typeof (Nullable<>).MakeGenericType (typeof(TAnnotated)).GetMethods ();
}

[Kept]
[UnexpectedWarning("IL2090", Tool.TrimmerAnalyzerAndNativeAot, "https://github.com/dotnet/runtime/issues/93800")]
static void ReturnValue()
{
typeof (Nullable<>).MakeGenericType (GetUnannotated()).GetMethods ();
typeof (Nullable<>).MakeGenericType (GetAnnotated()).GetMethods ();
}
[Kept]
static Type GetUnannotated() => null;
[Kept]
[return: KeptAttributeAttribute(typeof(DynamicallyAccessedMembersAttribute))]
[return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)]
static Type GetAnnotated() => null;

[Kept]
public static void Test()
{
Field();
Parameter(null, null);
TypeParameter<object, object>();
ReturnValue();
}
}

[Kept]
static void TestRequireRucMethodThroughNullable ()
{
Expand Down
Loading