Debug actions: Skip methods Log all patch cannot instrument

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)]

Source/Common/InstrumentationTargets.cs

@@ -0,0 +1,59 @@
+using System.Reflection;
+
+namespace Multiplayer.Common
+{
+    /// <summary>
+    /// 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.
+    /// </summary>
+    public static class InstrumentationTargets
+    {
+        /// <summary>The logger's own prefix, which must never instrument itself.</summary>
+        public const string LoggerMethodName = "MultiplayerMethodCallLogger";
+
+        /// <summary>
+        /// Whether a call logger can be attached to <paramref name="method"/>.
+        ///
+        /// 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.
+        /// </summary>
+        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;
+        }
+    }
+}

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()
+        {
+        }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        [DllImport("User32")]
+        public static extern int SetParent(int hwnd, int nCmdShow);
+
+        public static void Generic<T>()
+        {
+        }
+
+        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()
+        {
+        }
+    }
+}