Skip to content

Commit

Permalink
NativeAOT implement warning as errors as a compiler feature (dotnet#9…
Browse files Browse the repository at this point in the history
…6567)

Both roslyn and trimmer implement "warn as error" as a compiler feature - specifically the `TreatWarningsAsErrors` property only affects tools which explicitly react to it. There's a related MSBuild command line option which will affect all warnings, but that's not the one VS sets from the UI and what most customers use. See the comments in dotnet#94178.

This implements the same behavior as the trimmer. It adds 3 new command line options to `ilc` and enable "warn as error" globally and to enable/disable specific warning codes. Note that `illink` implements a more complex command line parsing where order of parameters matters and sometimes the later ones can override the earlier ones. This was trying to emulate `csc` behavior. For `ilc` I don't see a reason to emulate that because running `ilc` on its own is not supported, only going through the SDK, which will never pass the new parameters multiple times.

For testing this enables the existing trimmer tests for this functionality. This uncovered a small issue in substitution parser, which is also fixed here. For simplicity the test infra emulates the `illink` command line behavior over `ilc` (which is easy as they're very similar).
  • Loading branch information
vitek-karas authored and tmds committed Jan 23, 2024
1 parent 73c822d commit e7d04e5
Show file tree
Hide file tree
Showing 18 changed files with 180 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ The .NET Foundation licenses this file to you under the MIT license.
<PublishTrimmed Condition="'$(PublishTrimmed)' == ''">true</PublishTrimmed>
<IlcPgoOptimize Condition="'$(IlcPgoOptimize)' == '' and '$(OptimizationPreference)' == 'Speed'">true</IlcPgoOptimize>
<RunILLink>false</RunILLink>
<IlcTreatWarningsAsErrors Condition="'$(IlcTreatWarningsAsErrors)' == ''">$(TreatWarningsAsErrors)</IlcTreatWarningsAsErrors>
<_IsiOSLikePlatform Condition="'$(_targetOS)' == 'maccatalyst' or $(_targetOS.StartsWith('ios')) or $(_targetOS.StartsWith('tvos'))">true</_IsiOSLikePlatform>
<_IsApplePlatform Condition="'$(_targetOS)' == 'osx' or '$(_IsiOSLikePlatform)' == 'true'">true</_IsApplePlatform>
</PropertyGroup>
Expand Down Expand Up @@ -268,6 +269,9 @@ The .NET Foundation licenses this file to you under the MIT license.
<IlcArg Condition="'$(_targetOS)' == 'win' and $(IlcMultiModule) != 'true' and '$(IlcGenerateWin32Resources)' != 'false' and '$(NativeLib)' != 'Static'" Include="--win32resourcemodule:%(ManagedBinary.Filename)" />
<IlcArg Condition="$(IlcDumpIL) == 'true'" Include="--ildump:$(NativeIntermediateOutputPath)%(ManagedBinary.Filename).il" />
<IlcArg Condition="$(NoWarn) != ''" Include='--nowarn:"$([MSBuild]::Escape($(NoWarn)).Replace(`%0A`, ``).Replace(`%0D`, ``))"' />
<IlcArg Condition="$(IlcTreatWarningsAsErrors) == 'true'" Include="--warnaserror" />
<IlcArg Condition="$(WarningsAsErrors) != ''" Include='--warnaserr:"$([MSBuild]::Escape($(WarningsAsErrors)).Replace(`%0A`, ``).Replace(`%0D`, ``))"' />
<IlcArg Condition="$(WarningsNotAsErrors) != ''" Include='--nowarnaserr:"$([MSBuild]::Escape($(WarningsNotAsErrors)).Replace(`%0A`, ``).Replace(`%0D`, ``))"' />
<IlcArg Condition="$(TrimmerSingleWarn) == 'true'" Include="--singlewarn" />
<IlcArg Condition="$(SuppressTrimAnalysisWarnings) == 'true'" Include="--notrimwarn" />
<IlcArg Condition="$(SuppressAotAnalysisWarnings) == 'true'" Include="--noaotwarn" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,15 @@ protected override void ProcessMethod(TypeDesc type, XPathNavigator methodNav, o
_methodSubstitutions.Add(method, BodySubstitution.ThrowingBody);
break;
case "stub":
BodySubstitution stubBody;
BodySubstitution stubBody = null;
if (method.Signature.ReturnType.IsVoid)
stubBody = BodySubstitution.EmptyBody;
else
stubBody = BodySubstitution.Create(TryCreateSubstitution(method.Signature.ReturnType, GetAttribute(methodNav, "value")));
{
object substitution = TryCreateSubstitution(method.Signature.ReturnType, GetAttribute(methodNav, "value"));
if (substitution != null)
stubBody = BodySubstitution.Create(substitution);
}

if (stubBody != null)
{
Expand Down
24 changes: 17 additions & 7 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Logger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ public class Logger
private readonly HashSet<string> _trimWarnedAssemblies = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
private readonly HashSet<string> _aotWarnedAssemblies = new HashSet<string>(StringComparer.OrdinalIgnoreCase);

private readonly bool _treatWarningsAsErrors;
private readonly Dictionary<int, bool> _warningsAsErrors;

public static Logger Null = new Logger(new TextLogWriter(TextWriter.Null), null, false);

public bool IsVerbose { get; }
Expand All @@ -46,7 +49,9 @@ public Logger(
bool singleWarn,
IEnumerable<string> singleWarnEnabledModules,
IEnumerable<string> singleWarnDisabledModules,
IEnumerable<string> suppressedCategories)
IEnumerable<string> suppressedCategories,
bool treatWarningsAsErrors,
IDictionary<int, bool> warningsAsErrors)
{
_logWriter = writer;
IsVerbose = isVerbose;
Expand All @@ -55,17 +60,19 @@ public Logger(
_singleWarnEnabledAssemblies = new HashSet<string>(singleWarnEnabledModules, StringComparer.OrdinalIgnoreCase);
_singleWarnDisabledAssemblies = new HashSet<string>(singleWarnDisabledModules, StringComparer.OrdinalIgnoreCase);
_suppressedCategories = new HashSet<string>(suppressedCategories, StringComparer.Ordinal);
_treatWarningsAsErrors = treatWarningsAsErrors;
_warningsAsErrors = new Dictionary<int, bool>(warningsAsErrors);
_compilerGeneratedState = ilProvider == null ? null : new CompilerGeneratedState(ilProvider, this);
_unconditionalSuppressMessageAttributeState = new UnconditionalSuppressMessageAttributeState(_compilerGeneratedState, this);
}

public Logger(TextWriter writer, ILProvider ilProvider, bool isVerbose, IEnumerable<int> suppressedWarnings, bool singleWarn, IEnumerable<string> singleWarnEnabledModules, IEnumerable<string> singleWarnDisabledModules, IEnumerable<string> suppressedCategories)
: this(new TextLogWriter(writer), ilProvider, isVerbose, suppressedWarnings, singleWarn, singleWarnEnabledModules, singleWarnDisabledModules, suppressedCategories)
public Logger(TextWriter writer, ILProvider ilProvider, bool isVerbose, IEnumerable<int> suppressedWarnings, bool singleWarn, IEnumerable<string> singleWarnEnabledModules, IEnumerable<string> singleWarnDisabledModules, IEnumerable<string> suppressedCategories, bool treatWarningsAsErrors, IDictionary<int, bool> warningsAsErrors)
: this(new TextLogWriter(writer), ilProvider, isVerbose, suppressedWarnings, singleWarn, singleWarnEnabledModules, singleWarnDisabledModules, suppressedCategories, treatWarningsAsErrors, warningsAsErrors)
{
}

public Logger(ILogWriter writer, ILProvider ilProvider, bool isVerbose)
: this(writer, ilProvider, isVerbose, Array.Empty<int>(), singleWarn: false, Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>())
: this(writer, ilProvider, isVerbose, Array.Empty<int>(), singleWarn: false, Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), false, new Dictionary<int, bool>())
{
}

Expand Down Expand Up @@ -155,10 +162,13 @@ internal bool IsWarningSuppressed(int code, MessageOrigin origin)
return _unconditionalSuppressMessageAttributeState.IsSuppressed(code, origin);
}

internal static bool IsWarningAsError(int _/*code*/)
internal bool IsWarningAsError(int warningCode)
{
// TODO: warnaserror
return false;
bool value;
if (_treatWarningsAsErrors)
return !_warningsAsErrors.TryGetValue(warningCode, out value) || value;

return _warningsAsErrors.TryGetValue(warningCode, out value) && value;
}

internal bool IsSingleWarn(ModuleDesc owningModule, string messageSubcategory)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ internal static MessageContainer CreateErrorMessage(MessageOrigin? origin, Diagn
if (TryLogSingleWarning(context, code, origin, subcategory))
return null;

if (Logger.IsWarningAsError(code))
if (context.IsWarningAsError(code))
return new MessageContainer(MessageCategory.WarningAsError, text, code, subcategory, origin);

return new MessageContainer(MessageCategory.Warning, text, code, subcategory, origin);
Expand All @@ -148,7 +148,7 @@ internal static MessageContainer CreateErrorMessage(MessageOrigin? origin, Diagn
if (TryLogSingleWarning(context, (int)id, origin, subcategory))
return null;

if (Logger.IsWarningAsError((int)id))
if (context.IsWarningAsError((int)id))
return new MessageContainer(MessageCategory.WarningAsError, id, subcategory, origin, args);

return new MessageContainer(MessageCategory.Warning, id, subcategory, origin, args);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,8 @@ public class ILCompilerOptions
public List<string> Descriptors = new List<string> ();
public bool FrameworkCompilation;
public bool SingleWarn;
public List<string> SubstitutionFiles = new List<string> ();
public bool TreatWarningsAsErrors;
public Dictionary<int, bool> WarningsAsErrors = new Dictionary<int, bool> ();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ public virtual void AddStripLinkAttributes (bool stripLinkAttributes)

public virtual void AddSubstitutions (string file)
{
Options.SubstitutionFiles.Add (file);
}

public virtual void AddLinkAttributes (string file)
Expand All @@ -161,6 +162,30 @@ public virtual void AddAdditionalArgument (string flag, string[] values)
else if (flag == "--singlewarn") {
Options.SingleWarn = true;
}
else if (flag.StartsWith("--warnaserror"))
{
if (flag == "--warnaserror" || flag == "--warnaserror+")
{
if (values.Length == 0)
Options.TreatWarningsAsErrors = true;
else
{
foreach (int warning in ProcessWarningCodes(values))
Options.WarningsAsErrors[warning] = true;
}

}
else if (flag == "--warnaserror-")
{
if (values.Length == 0)
Options.TreatWarningsAsErrors = false;
else
{
foreach (int warning in ProcessWarningCodes(values))
Options.WarningsAsErrors[warning] = false;
}
}
}
}

public virtual void ProcessTestInputAssembly (NPath inputAssemblyPath)
Expand Down Expand Up @@ -261,6 +286,23 @@ private static void AppendExpandedPaths (Dictionary<string, string> dictionary,
}
}

private static readonly char[] s_separator = new char[] { ',', ';', ' ' };

private static IEnumerable<int> ProcessWarningCodes(IEnumerable<string> warningCodes)
{
foreach (string value in warningCodes)
{
string[] values = value.Split(s_separator, StringSplitOptions.RemoveEmptyEntries);
foreach (string id in values)
{
if (!id.StartsWith("IL", StringComparison.Ordinal) || !ushort.TryParse(id.AsSpan(2), out ushort code))
continue;

yield return code;
}
}
}

public ILCompilerOptions Build ()
{
var options = Options;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Linq;
using System.Reflection;
using System.Runtime.InteropServices;
using System.Xml;
using ILCompiler;
using ILCompiler.Dataflow;
using ILLink.Shared.TrimAnalysis;
Expand Down Expand Up @@ -84,15 +85,33 @@ public ILScanResults Trim (ILCompilerOptions options, TrimmingCustomizations? cu
options.SingleWarn,
singleWarnEnabledModules: Enumerable.Empty<string> (),
singleWarnDisabledModules: Enumerable.Empty<string> (),
suppressedCategories: Enumerable.Empty<string> ());
suppressedCategories: Enumerable.Empty<string> (),
treatWarningsAsErrors: options.TreatWarningsAsErrors,
warningsAsErrors: options.WarningsAsErrors);

foreach (var descriptor in options.Descriptors) {
if (!File.Exists (descriptor))
throw new FileNotFoundException ($"'{descriptor}' doesn't exist");
compilationRoots.Add (new ILCompiler.DependencyAnalysis.TrimmingDescriptorNode (descriptor));
}

ilProvider = new FeatureSwitchManager (ilProvider, logger, options.FeatureSwitches, default);

var featureSwitches = options.FeatureSwitches;
BodyAndFieldSubstitutions substitutions = default;
IReadOnlyDictionary<ModuleDesc, IReadOnlySet<string>>? resourceBlocks = default;
foreach (string substitutionFilePath in options.SubstitutionFiles)
{
using FileStream fs = File.OpenRead(substitutionFilePath);
substitutions.AppendFrom(BodySubstitutionsParser.GetSubstitutions(
logger, typeSystemContext, XmlReader.Create(fs), substitutionFilePath, featureSwitches));

fs.Seek(0, SeekOrigin.Begin);

resourceBlocks = ManifestResourceBlockingPolicy.UnionBlockings(resourceBlocks,
ManifestResourceBlockingPolicy.SubstitutionsReader.GetSubstitutions(
logger, typeSystemContext, XmlReader.Create(fs), substitutionFilePath, featureSwitches));
}

ilProvider = new FeatureSwitchManager(ilProvider, logger, featureSwitches, substitutions);

CompilerGeneratedState compilerGeneratedState = new CompilerGeneratedState (ilProvider, logger);

Expand Down
9 changes: 9 additions & 0 deletions src/coreclr/tools/aot/ILCompiler/ILCompilerRootCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,12 @@ internal sealed class ILCompilerRootCommand : CliRootCommand
new("--singlewarnassembly") { DefaultValueFactory = _ => Array.Empty<string>(), Description = "Generate single AOT/trimming warning for given assembly" };
public CliOption<string[]> SingleWarnDisabledAssemblies { get; } =
new("--nosinglewarnassembly") { DefaultValueFactory = _ => Array.Empty<string>(), Description = "Expand AOT/trimming warnings for given assembly" };
public CliOption<bool> TreatWarningsAsErrors { get; } =
new("--warnaserror") { Description = "Treat warnings as errors" };
public CliOption<string[]> WarningsAsErrorsEnable { get; } =
new("--warnaserr") { Description = "Enable treating specific warnings as errors" };
public CliOption<string[]> WarningsAsErrorsDisable { get; } =
new("--nowarnaserr") { Description = "Disable treating specific warnings as errors" };
public CliOption<string[]> DirectPInvokes { get; } =
new("--directpinvoke") { DefaultValueFactory = _ => Array.Empty<string>(), Description = "PInvoke to call directly" };
public CliOption<string[]> DirectPInvokeLists { get; } =
Expand Down Expand Up @@ -225,6 +231,9 @@ public ILCompilerRootCommand(string[] args) : base(".NET Native IL Compiler")
Options.Add(NoAotWarn);
Options.Add(SingleWarnEnabledAssemblies);
Options.Add(SingleWarnDisabledAssemblies);
Options.Add(TreatWarningsAsErrors);
Options.Add(WarningsAsErrorsEnable);
Options.Add(WarningsAsErrorsDisable);
Options.Add(DirectPInvokes);
Options.Add(DirectPInvokeLists);
Options.Add(MaxGenericCycleDepth);
Expand Down
12 changes: 11 additions & 1 deletion src/coreclr/tools/aot/ILCompiler/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,18 @@ public int Run()

ILProvider ilProvider = new NativeAotILProvider();

Dictionary<int, bool> warningsAsErrors = new Dictionary<int, bool>();
foreach (int warning in ProcessWarningCodes(Get(_command.WarningsAsErrorsEnable)))
{
warningsAsErrors[warning] = true;
}
foreach (int warning in ProcessWarningCodes(Get(_command.WarningsAsErrorsDisable)))
{
warningsAsErrors[warning] = false;
}
var logger = new Logger(Console.Out, ilProvider, Get(_command.IsVerbose), ProcessWarningCodes(Get(_command.SuppressedWarnings)),
Get(_command.SingleWarn), Get(_command.SingleWarnEnabledAssemblies), Get(_command.SingleWarnDisabledAssemblies), suppressedWarningCategories);
Get(_command.SingleWarn), Get(_command.SingleWarnEnabledAssemblies), Get(_command.SingleWarnDisabledAssemblies), suppressedWarningCategories,
Get(_command.TreatWarningsAsErrors), warningsAsErrors);

// NativeAOT is full AOT and its pre-compiled methods can not be
// thrown away at runtime if they mismatch in required ISAs or
Expand Down
2 changes: 2 additions & 0 deletions src/tests/Directory.Build.targets
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,8 @@
so the default for the test tree is partial. -->
<TrimMode Condition="'$(EnableAggressiveTrimming)' != 'true'">partial</TrimMode>

<IlcTreatWarningsAsErrors>false</IlcTreatWarningsAsErrors>

<IlcToolsPath>$(CoreCLRILCompilerDir)</IlcToolsPath>
<IlcToolsPath Condition="'$(TargetArchitecture)' != '$(BuildArchitecture)' or '$(TargetOS)' != '$(HostOS)' or '$(EnableNativeSanitizers)' != ''">$(CoreCLRCrossILCompilerDir)</IlcToolsPath>
<IlcBuildTasksPath>$(CoreCLRILCompilerDir)netstandard/ILCompiler.Build.Tasks.dll</IlcBuildTasksPath>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,8 @@ private static void ThrowIfPresent(Type testType, string typeName)
}
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2072:UnrecognizedReflectionPattern",
Justification = "That's the point")]
private static void ThrowIfPresentWithUsableMethodTable(Type testType, string typeName)
{
Type t = GetTypeSecretly(testType, typeName);
Expand Down
2 changes: 2 additions & 0 deletions src/tests/nativeaot/SmokeTests/UnitTests/Devirtualization.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Diagnostics.CodeAnalysis;

class Devirtualization
{
Expand Down Expand Up @@ -103,6 +104,7 @@ static void AssertEqual<T>(T expected, T actual)
[MethodImpl(MethodImplOptions.NoInlining)]
static void TestIntf2(IIntf2 o, int expected) => AssertEqual(expected, o.GetValue());

[UnconditionalSuppressMessage("AOT", "IL3050", Justification = "MakeGenericType - Intentional")]
public static void Run()
{
TestIntf1(new Intf1Impl(), 123);
Expand Down
5 changes: 5 additions & 0 deletions src/tests/nativeaot/SmokeTests/UnitTests/Generics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@
using System.Reflection;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using System.Diagnostics.CodeAnalysis;

[UnconditionalSuppressMessage("Trimming", "IL2055", Justification = "MakeGenericType - Intentional")]
[UnconditionalSuppressMessage("Trimming", "IL2060", Justification = "MakeGenericMethod - Intentional")]
[UnconditionalSuppressMessage("AOT", "IL3050", Justification = "MakeGenericType/MakeGenericMethod - Intentional")]
[UnconditionalSuppressMessage("AOT", "IL3054", Justification = "Generic expansion aborted - Intentional")]
class Generics
{
internal static int Run()
Expand Down
6 changes: 6 additions & 0 deletions src/tests/nativeaot/SmokeTests/UnitTests/Interfaces.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Diagnostics.CodeAnalysis;

public class Interfaces
{
Expand Down Expand Up @@ -1225,6 +1226,7 @@ public static int CallIndirect()

static Type s_fooType = typeof(Foo);

[UnconditionalSuppressMessage("AOT", "IL3050", Justification = "MakeGenericType - Intentional")]
public static void Run()
{
Type t = typeof(FrobCaller<>).MakeGenericType(s_fooType);
Expand Down Expand Up @@ -1289,6 +1291,7 @@ class Atom { }

static Type s_atomType = typeof(Atom);

[UnconditionalSuppressMessage("AOT", "IL3050", Justification = "MakeGenericType - Intentional")]
public static void Run()
{
Type t = typeof(Wrapper<>).MakeGenericType(s_atomType);
Expand Down Expand Up @@ -1354,6 +1357,7 @@ class Atom : AtomBase { }
static Type s_atomType = typeof(Atom);
static Type s_atomBaseType = typeof(AtomBase);

[UnconditionalSuppressMessage("AOT", "IL3050", Justification = "MakeGenericType - Intentional")]
public static void Run()
{
Type t = typeof(FrobCaller<,>).MakeGenericType(typeof(AbjectFail<AtomBase>), s_atomType);
Expand Down Expand Up @@ -1576,6 +1580,7 @@ class Gen<T> where T : IFoo
public static string GrabCookie() => T.ImHungryGiveMeCookie();
}

[UnconditionalSuppressMessage("AOT", "IL3050", Justification = "MakeGenericType - Intentional")]
public static void Run()
{
var r = (string)typeof(Gen<>).MakeGenericType(typeof(Baz)).GetMethod("GrabCookie").Invoke(null, Array.Empty<object>());
Expand Down Expand Up @@ -1612,6 +1617,7 @@ class Gen<T> where T : IFoo
public static string GrabCookie() => T.ImHungryGiveMeCookie();
}

[UnconditionalSuppressMessage("AOT", "IL3050", Justification = "MakeGenericType - Intentional")]
public static void Run()
{
Activator.CreateInstance(typeof(Baz<>).MakeGenericType(typeof(Atom1)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,12 @@ public Task CanWarnAsError ()
return RunTest (allowMissingWarnings: true);
}

[Fact]
public Task CanWarnAsErrorGlobal ()
{
return RunTest (allowMissingWarnings: true);
}

[Fact]
public Task InvalidWarningVersion ()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
namespace Mono.Linker.Tests.Cases.Warnings
{
[ExpectNonZeroExitCode (1)]
[IgnoreTestCase ("Ignore in NativeAOT, see https://github.com/dotnet/runtime/issues/82447", IgnoredBy = Tool.NativeAot)]
[SkipKeptItemsValidation]
[SetupLinkerSubstitutionFile ("CanWarnAsErrorSubstitutions.xml")]
[SetupLinkerArgument ("--verbose")]
Expand Down
Loading

0 comments on commit e7d04e5

Please sign in to comment.