BytecodePath NOP padding creates invalid class

[LexManos]

Swap the order of the nop padding, so that replaced code comes BEFORE the nop padding.

        ci.write(repl, matcher.getStart());
        for (int i = 0; i < skip; i++) {
            ci.writeByte(Opcode.NOP, matcher.getStart() + repl.length + i);
        }

It may just be the fact that my replaced code returns from the function, but having a ton of NOPs before real code causes the Method to become invalid. {Various runtime errors when MC trys to use it.}

Possible future fix is to go in a dynamically remove all NOPs from the final {after all mods have had a pass} and condense the code. Also, need to figure out a way to verify all paths are reachable. {May not be an issue}

Also, Exception handlers need to be verified after the code has been replaced, if a exception handler spills over to NOPs/After return, java won't load the class.

Also, I may be bugging you with pointing out things at this stage. But I'm more then willing to help track things down and fix up the code.

[prupe]

I appreciate you finding these. I didn't have to touch exception handlers for the texture and font fixes, so it doesn't surprise me that something fell through the cracks.

I'm still not sure I understand the problem though. Can you post a sample before and after bytecode?

[LexManos]

Here is the exception:
Exception in thread "Minecraft main thread" java.lang.ClassFormatError: (class: ep, method: signature: ()V) Illegal exception table range
at net.minecraft.client.Minecraft.a(SourceFile:349)
at net.minecraft.client.Minecraft.run(SourceFile:638)
at java.lang.Thread.run(Unknown Source)
Here is the new bytecode:
0 aload_0
1 invokespecial #12 <cf/()V>
4 aload_0
5 fconst_0
6 putfield #16 <ep/i F>
9 aload_0
10 invokestatic #22 <org/jbls/LexManos/PackManager/GetSplash()Ljava/lang/String;>
13 putfield #26 <ep/j Ljava/lang/String;>
16 return
17 nop
{Nop repeats, trimmed for length
114 nop

Exception table:
Idx: 0: Start: 15 End: 110 Handler: 113 <Ljava/lang/Exception;>

Last but not least, my code:
public GUIMainScreenMod()
{
this.classSignatures.add(new ConstSignature("menu.singleplayer"));
this.fieldMappers.add(new ClassMod.FieldMapper("splashText", "Ljava/lang/String;"));

    patches.add(new BytecodePatch()
    {
        @Override
        public String getDescription()
        {
            return "splashText set -> PackManager.GetSplash()";
        }
        @Override
        public boolean filterMethod(MethodInfo info)
        {
            if (!info.isConstructor())
                return false;

            ExceptionTable exs = info.getCodeAttribute().getExceptionTable();
            //while(exs.size() > 0)
            {
                Logger.log(1, "Removing exception handler");
                //exs.remove(0);
            }
            return true;
        }
        @Override
        public String getMatchExpression(MethodInfo info)
        {
            return buildExpression(
                    Opcode.ALOAD_0,
                    Opcode.LDC,
                    BinaryRegex.any(),
                    reference(info, Opcode.PUTFIELD,      new FieldRef(info.getConstPool().getClassName(), "j", "Ljava/lang/String;")),
                    "(?: (?!b1)\\p{XDigit}{2})+",
                    Opcode.RETURN
                    );
        }
        @Override
        public byte[] getReplacementBytes(MethodInfo info) throws IOException
        {
            return buildCode(
                    Opcode.ALOAD_0,
                    reference(info, Opcode.INVOKESTATIC,  new MethodRef("org/jbls/LexManos/PackManager", "GetSplash", "()Ljava/lang/String;")),
                    reference(info, Opcode.PUTFIELD,      new FieldRef(info.getConstPool().getClassName(), "j", "Ljava/lang/String;")),
                    Opcode.RETURN
            );
        }
    });
}

Also, FieldMapper doesn't seem to work, I still have to use the obfusicated name.
And if this becomes a reliable system, I'll write a scripting system to make writing the patches easier for the end user.

[prupe]

I've had a chance to play with this now and I see the problem, but I'm not sure the best way to fix it.

Looking at your example, the exception handler covers 15 to 110. Your match expression starts at 10 and runs to 113. So here's what you get now:

[bunch of NOPs]
107: aload_0
108: invokestatic #22; //Method org/jbls/LexManos/PackManager.GetSplash:()Ljava/lang/String;
111: putfield #26; //Field j:Ljava/lang/String;

Offset 110 is in the middle of an instruction which I assume is why it is complaining.

But if I try your suggestion and put the replacement code first, this is what happens:

10: invokestatic #22; //Method org/jbls/LexManos/PackManager.GetSplash:()Ljava/lang/String;
13: putfield #26; //Field j:Ljava/lang/String;
16: return
[bunch of NOPs]

So it still fails because 15 is in the middle of the putfield instruction.

Ugh, what a mess. Really the "right" way to fix this is to not reuse at space at all, but rather overwrite the old code with NOPs and insert the new code before or after it.

[EDIT] Forgot to mention, the FieldMapper should work, but you have to do this

reference(info, PUTFIELD, map(new FieldRef("GUIMainScreen", "splashText", "Ljava/lang/String;"))),

It would be easier and more consistent if it just called map() for you. I may go ahead and change that. I can't remember why I didn't do it that way from the beginning.