From 3cef50d6cf33f10fad53d90f1ab2dae839fd799e Mon Sep 17 00:00:00 2001 From: Alexey Nikitin Date: Thu, 13 Aug 2026 02:10:39 -0500 Subject: [PATCH] Skip methods Log all patch cannot instrument Log all patch attaches a call logger to every method the assembly declares. Harmony patches by emitting a wrapper around the original's body, and an extern method has none, so the wrapper is malformed and the runtime rejects it with InvalidProgramException. The assembly declares two, both P/Invokes used to hide the headless arbiter's window. Reaching the first threw out of the loop, so every method not yet reached was left uninstrumented while the action still emitted a large, plausible-looking trace with no sign that coverage was partial. Which methods survived depended on the order DefinedTypes happened to return, so the boundary moved between builds. Exclude externs and methods still carrying open generic parameters, and guard the Patch call. The filter cannot anticipate every reason Harmony may refuse a method, and this action is wanted precisely when something is already wrong, so it should lose one entry rather than the whole trace. The count is reported once at the end, and the first refusal in full. The decision moves to Common so it can be covered by a test; the test project cannot reference the client, which needs the game's assemblies. --- Source/Client/Debug/DebugActions.cs | 35 ++++++--- Source/Common/InstrumentationTargets.cs | 59 +++++++++++++++ Source/Tests/InstrumentationTargetsTest.cs | 83 ++++++++++++++++++++++ 3 files changed, 167 insertions(+), 10 deletions(-) create mode 100644 Source/Common/InstrumentationTargets.cs create mode 100644 Source/Tests/InstrumentationTargetsTest.cs 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() + { + } + } +}