diff --git a/Source/Client/Debug/DebugActions.cs b/Source/Client/Debug/DebugActions.cs index 46d05147b..e1117d5f6 100644 --- a/Source/Client/Debug/DebugActions.cs +++ b/Source/Client/Debug/DebugActions.cs @@ -11,6 +11,7 @@ using HarmonyLib; using LudeonTK; using Multiplayer.Client.Desyncs; +using Multiplayer.Common; using Multiplayer.Client.Util; using Multiplayer.Client.Windows; using RimWorld; @@ -335,17 +336,31 @@ static void DumpIRenameableTypes() [DebugAction(MultiplayerCategory, allowedGameStates = AllowedGameStates.Playing)] static void LogAllPatch() { + var logger = new HarmonyMethod(typeof(MpDebugActions), nameof(MultiplayerMethodCallLogger)); + var refused = 0; + foreach (var method in Assembly.GetExecutingAssembly().DefinedTypes.SelectMany(t => t.DeclaredMethods)) - if (method.Name != "MultiplayerMethodCallLogger" && - !method.Name.StartsWith("get_") && - !method.IsGenericMethod && - method.DeclaringType?.IsGenericType is false && - method.DeclaringType?.BaseType != typeof(MulticastDelegate) && - !method.IsAbstract) - Multiplayer.harmony.Patch( - method, - prefix: new HarmonyMethod(typeof(MpDebugActions), nameof(MultiplayerMethodCallLogger)) - ); + { + if (!InstrumentationTargets.ShouldInstrument(method)) + continue; + + try + { + Multiplayer.harmony.Patch(method, prefix: logger); + } + catch (Exception e) + { + // The filter cannot anticipate every reason Harmony may refuse a method, and a trace + // missing one entry is worth far more than no trace at all. Counted rather than logged + // per method, so a systematic refusal does not bury the trace this action exists for. + refused++; + if (refused == 1) + Log.Warning($"MP: could not instrument {method.FullDescription()}: {e.GetBaseException().Message}"); + } + } + + if (refused > 0) + Log.Warning($"MP: {refused} method(s) could not be instrumented; the trace below is incomplete"); } [DebugAction(MultiplayerCategory, allowedGameStates = AllowedGameStates.Entry)] diff --git a/Source/Common/InstrumentationTargets.cs b/Source/Common/InstrumentationTargets.cs new file mode 100644 index 000000000..ef2792c00 --- /dev/null +++ b/Source/Common/InstrumentationTargets.cs @@ -0,0 +1,59 @@ +using System.Reflection; + +namespace Multiplayer.Common +{ + /// + /// Decides which methods the "Log all patch" debug action may attach its call logger to. + /// + /// Kept here rather than beside the debug action so it can be tested. The test project cannot + /// reference the client, which needs the game's own assemblies to load. + /// + public static class InstrumentationTargets + { + /// The logger's own prefix, which must never instrument itself. + public const string LoggerMethodName = "MultiplayerMethodCallLogger"; + + /// + /// Whether a call logger can be attached to . + /// + /// Contract: exclude anything Harmony cannot build a wrapper around. Harmony patches by emitting + /// a replacement that wraps the original's body, so a method with no body to wrap produces + /// malformed IL and the runtime rejects it. + /// + public static bool ShouldInstrument(MethodBase method) + { + if (method == null) + return false; + + if (method.Name == LoggerMethodName) + return false; + + // Extern, so the body lives in a native library and there is nothing to wrap. This assembly + // declares two, in ArbiterWindowFix. Tested via MethodAttributes rather than a dedicated + // property, because MethodBase exposes no IsPInvokeImpl on every target framework here. + if ((method.Attributes & MethodAttributes.PinvokeImpl) != 0) + return false; + + // Property getters are noise: they run constantly and say nothing about control flow. + if (method.Name.StartsWith("get_")) + return false; + + if (method.IsAbstract) + return false; + + // Open generic parameters, whether the method's own or inherited from its declaring type, + // leave nothing concrete to emit against. + if (method.IsGenericMethod || method.ContainsGenericParameters) + return false; + + var declaring = method.DeclaringType; + if (declaring == null || declaring.IsGenericType) + return false; + + if (declaring.BaseType == typeof(System.MulticastDelegate)) + return false; + + return true; + } + } +} diff --git a/Source/Tests/InstrumentationTargetsTest.cs b/Source/Tests/InstrumentationTargetsTest.cs new file mode 100644 index 000000000..f15a8194e --- /dev/null +++ b/Source/Tests/InstrumentationTargetsTest.cs @@ -0,0 +1,83 @@ +using System.Reflection; +using System.Runtime.InteropServices; +using Multiplayer.Common; + +namespace Tests; + +public class InstrumentationTargetsTest +{ + private static class Candidates + { + public static void Ordinary() + { + } + + /// + /// Stands in for ArbiterWindowFix.SetParent, the declaration that aborts the real action. + /// + /// Declared but never called, so it needs no library at run time and behaves the same on every + /// platform. What matters is only that reflection reports it as extern. + /// + [DllImport("User32")] + public static extern int SetParent(int hwnd, int nCmdShow); + + public static void Generic() + { + } + + public static int Value => 0; + } + + private static MethodBase MethodOf(string name) + => typeof(Candidates).GetMethod(name, BindingFlags.Public | BindingFlags.Static); + + [Test] + public void ShouldInstrument_AnOrdinaryMethod() + { + Assert.That(InstrumentationTargets.ShouldInstrument(MethodOf(nameof(Candidates.Ordinary))), Is.True); + } + + [Test] + public void ShouldNotInstrument_AnExternMethod() + { + var method = MethodOf(nameof(Candidates.SetParent)); + + Assert.That(method.Attributes.HasFlag(MethodAttributes.PinvokeImpl), Is.True, + "the stand-in must actually be extern for this test to mean anything"); + Assert.That(InstrumentationTargets.ShouldInstrument(method), Is.False, + "an extern method has no IL body, so Harmony emits a malformed wrapper and the runtime rejects it"); + } + + [Test] + public void ShouldNotInstrument_AGenericMethod() + { + Assert.That(InstrumentationTargets.ShouldInstrument(MethodOf(nameof(Candidates.Generic))), Is.False); + } + + [Test] + public void ShouldNotInstrument_APropertyGetter() + { + var getter = typeof(Candidates).GetProperty(nameof(Candidates.Value), BindingFlags.Public | BindingFlags.Static)!.GetGetMethod(); + + Assert.That(InstrumentationTargets.ShouldInstrument(getter), Is.False); + } + + [Test] + public void ShouldNotInstrument_TheLoggerItself() + { + var logger = typeof(SelfReference).GetMethod( + InstrumentationTargets.LoggerMethodName, + BindingFlags.Public | BindingFlags.Static); + + Assert.That(InstrumentationTargets.ShouldInstrument(logger), Is.False, + "instrumenting the logger would make every logged call log itself"); + } + + private static class SelfReference + { + // Named to match the real prefix, so the guard is exercised by name as it is in production. + public static void MultiplayerMethodCallLogger() + { + } + } +}