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

Fixing and synchronizing dumper #118

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixing and synchronizing dumper #118

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
57 changes: 48 additions & 9 deletions Harmony/Internal/Util/CecilEmitter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using HarmonyLib.Tools;
using Mono.Cecil;
using Mono.Cecil.Cil;
using Mono.Collections.Generic;
using MonoMod.Cil;
using MonoMod.Utils;
using System;
Expand All @@ -19,7 +20,7 @@ namespace HarmonyLib.Internal.Util;
/// <summary>
/// Basic safe DLL emitter for dynamically generated <see cref="MethodDefinition"/>s.
/// </summary>
/// <remarks>Based on https://github.com/MonoMod/MonoMod.Common/blob/master/Utils/DMDGenerators/DMDCecilGenerator.cs</remarks>
/// <remarks>Based on https://github.com/MonoMod/MonoMod/blob/reorganize/src/MonoMod.Utils/DMDGenerators/DMDCecilGenerator.cs</remarks>
internal static class CecilEmitter
{
private static readonly ConstructorInfo UnverifiableCodeAttributeConstructor =
Expand Down Expand Up @@ -49,26 +50,43 @@ public static void Dump(MethodDefinition md, IEnumerable<string> dumpPaths, Meth

MethodDefinition clone = null;

/* see below
var isVolatile = new TypeReference("System.Runtime.CompilerServices", "IsVolatile", module,
module.TypeSystem.CoreLibrary);
*/
Comment on lines +53 to +56
Copy link

Choose a reason for hiding this comment

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

If this is no longer important, it should be removed.


Relinker relinker = (mtp, _) => mtp == md ? clone : module.ImportReference(mtp);
Relinker relinker = (mtp, _) =>
{
if (mtp == md)
return clone!;
if (mtp is MethodReference mr)
{
if (mr.FullName == md.FullName
&& mr.DeclaringType.FullName == md.DeclaringType.FullName
&& mr.DeclaringType.Scope.Name == md.DeclaringType.Scope.Name)
return clone!;
}
return module.ImportReference(mtp);
};
Comment on lines +58 to +70
Copy link

Choose a reason for hiding this comment

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

Justification?

Copy link
Contributor

@Meivyn Meivyn Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Updated from reorg: https://github.com/MonoMod/MonoMod/blob/26a0a4d22fcd233dd22db4fd034cb3ac41420fd4/src/MonoMod.Utils/DMDGenerators/DMDCecilGenerator.cs#L72-L83

Which is also the same for the double Public, and everything else but the label stuff. Label stuff specifically addressed the issue I mentioned above.


clone =
new MethodDefinition(original?.Name ?? "_" + md.Name.Replace(".", "_"), md.Attributes, module.TypeSystem.Void)
{
MethodReturnType = md.MethodReturnType,
Attributes = MethodAttributes.Public | MethodAttributes.HideBySig | MethodAttributes.Static,
Attributes = MethodAttributes.Public | MethodAttributes.HideBySig | MethodAttributes.Public | MethodAttributes.Static,
Copy link

Choose a reason for hiding this comment

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

I don't see any reason to have Public included twice.

ImplAttributes = MethodImplAttributes.IL | MethodImplAttributes.Managed,
DeclaringType = td,
HasThis = false
HasThis = false,
NoInlining = true
Copy link

Choose a reason for hiding this comment

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

Justification?

Copy link
Contributor

@Meivyn Meivyn Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Dumps are broken without this when you try to dump methods that are inlined, or when dumping a transpiler that is postfixed, or situations like that. Same thing for relinker below. At least this is what this PR is supposed to fix, and it did when I tested it.

For context, this is what happened when I tried to dump a postfix that was inlined:

image

Original method:

public static NoteLineLayer GetNoteLineLayer(int lineLayer)
{
  switch (lineLayer)
  {
    case 0:
      return NoteLineLayer.Base;
    case 1:
      return NoteLineLayer.Upper;
    case 2:
      return NoteLineLayer.Top;
    default:
      return NoteLineLayer.Base;
  }
}

Postfix:

[HarmonyPatch(typeof(BeatmapDataLoaderVersion3.BeatmapDataLoader.ObstacleConverter), nameof(BeatmapDataLoaderVersion3.BeatmapDataLoader.ObstacleConverter.GetNoteLineLayer))]
internal class BeatmapDataLoaderObstacleConverterGetNoteLineLayerPatch
{
    private static void Postfix(ref NoteLineLayer __result, int lineLayer)
    {
        if (lineLayer > 2)
        {
            __result = (NoteLineLayer)lineLayer;
        }
    }
}

};

Copy link

Choose a reason for hiding this comment

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

Avoid formatting changes wherever possible. It makes pull requests more annoying to review.

td.Methods.Add(clone);

foreach (var param in md.Parameters)
clone.Parameters.Add(param.Clone().Relink(relinker, clone));

clone.ReturnType = md.ReturnType.Relink(relinker, clone);

var body = clone.Body = md.Body.Clone(clone);

foreach (var variable in clone.Body.Variables)
Expand All @@ -83,15 +101,25 @@ public static void Dump(MethodDefinition md, IEnumerable<string> dumpPaths, Meth
operand = operand switch
{
ParameterDefinition param => clone.Parameters[param.Index],
ILLabel label => label.Target,
ILLabel label => LabelFix(label, body.Instructions, md.Body.Instructions),
IMetadataTokenProvider mtp => mtp.Relink(relinker, clone),
_ => operand
};

if (instr.Previous?.OpCode == OpCodes.Volatile &&
operand is FieldReference fref &&
(fref.FieldType as RequiredModifierType)?.ModifierType != isVolatile)
fref.FieldType = new RequiredModifierType(isVolatile, fref.FieldType);
// System.Reflection doesn't contain any volatility info.
// System.Reflection.Emit presumably does something similar to this.
// Mono.Cecil thus isn't aware of the volatility as part of the imported field reference.
// The modifier is still necessary though.
// This is done here instead of the copier as Harmony and other users can't track modreqs

// This isn't actually a valid transformation though. A ldfld or stfld can have the volatile
// prefix, without having modreq(IsVolatile) on the field. Adding the modreq() causes the runtime
// to not be able to find the field.
/*if (instr.Previous?.OpCode == OpCodes.Volatile &&
operand is FieldReference fref &&
(fref.FieldType as RequiredModifierType)?.ModifierType != tr_IsVolatile) {
fref.FieldType = new RequiredModifierType(tr_IsVolatile, fref.FieldType);
}*/
Comment on lines +118 to +122
Copy link

Choose a reason for hiding this comment

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

As above, if something isn't important, delete it.


instr.Operand = operand;
}
Expand Down Expand Up @@ -129,4 +157,15 @@ private static string SanitizeTypeName(string typeName)
.Replace("<", "{")
.Replace(">", "}");
}

private static Instruction LabelFix(ILLabel label, Collection<Instruction> clone, Collection<Instruction> original)
{
var target = label.Target;
if (!clone.Contains(target))
{
var idx = original.IndexOf(label.Target);
if (idx != -1) return clone[idx];
}
return target;
}
Comment on lines +161 to +170
Copy link

Choose a reason for hiding this comment

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

Are clone and original guaranteed to have the same count? If so, add a Debug.Assert call. If not, clone[idx] is a bug.

Are there actually situations where clone.Contains(target) returns true?

I'm uncomfortable with ever returning target. I feel like it should throw if the instruction isn't found.

}