octarine-noise / BetterFoliage

Minecraft mod that alters the appearance of leaves & grass
MIT License
98 stars 42 forks source link

LittleTiles crash #128

Closed CreativeMD closed 7 years ago

CreativeMD commented 7 years ago

Hey there,

I'm the author of LittleTiles. Recently somebody posted this issue (https://github.com/CreativeMD/LittleTiles/issues/42). So i spent hours and hours to fix it, but unfortunately I couldn't. Crash-log: https://pastebin.com/f5bbkyiP

As far as i can tell this issue is caused by a label mismatch. I'm adding a local variable, which seems to "end" to early, so the reference leads to nowhere (my code).

BetterFoliage v2.1.3 works just fine, so @SortingIndex(value = 1400) seems to cause the issue. I have no idea, why. Maybe you have?

In Regards CreativeMD

octarine-noise commented 7 years ago

Yes. With the SortingIndex unspecified, the default value is 0. Since 0 == 0 and "B" < "L", my transformer was running first, and since my changes were compatible with yours, everything was running fine.

Now that 1400 > 0 the order is reversed, and because I'm referencing method-local variables by their index (empirically determined, which are now apparently shifted by your extra one), the resulting bytecode is invalid and things break down. You can see the relevant part of my transformer here.

The easiest fix would be for you to set an even higher sorting index, to get behind me again. However, note that there is an FML deobfuscation transformer at index 1000, so you'll see all code in the SRG namespace, instead of vanilla obfuscated. I actually consider that a good thing, but it's still something you need to watch out for.

CreativeMD commented 7 years ago

But why does your transformer crush my code? I have no issues with other mods (for example Optifine), which does transform the same method. I mean my code should still work although you do things afterwards. The SRG names are a problem, I could change it, but it would consume some more time. I just couldn't find out why your code, sets the end of my local variable too early. Theoretically if everything has been done properly (from my end), there should be no issue at all.

I tried to add BetterFoliage to my workbench already (to do some more testing), but failed because i couldn't add 'kotlin nature' to my project (for no reason) and I'm quite inexperienced with those kind of stuff.

Maybe you could change ClassWriter(0) to ClassWriter(3) or ClassWriter(ClassWriter.COMPUTE_MAXS + ClassWriter.COMPUTE_FRAMES), which will set it to compute maxs and frames. Theoretically it should assign new label boundaries to my local variable and solve this issue.

octarine-noise commented 7 years ago

I've had problems with COMPUTE_MAXS and COMPUTE_FRAMES before. But I think it has nothing to do with labels.

The problem is that my transformer expects certain local variables at certain indexes. There's nothing wrong with your transformer per se, and it outputs perfectly fine bytecode. But because of the added local variable, the indexes have shifted, and my transformer generates nonsense, i.e. calling a method with completely different parameters, that are the wrong type. The classloader sees this, and refuses to load the corrupt bytecode. That's what "java.lang.VerifyError: Bad local variable type" means.

So basically, it is my transformer that breaks things, because you broke some of my assumptions.

The reason I think you should move to a higher SortingIndex, is because the alternative is for me to implement my transformer in a way that it identifies the correct variables instead of using hardcoded indexes. That's highly nontrivial if there are multiple variables of the same type in the method body - and in the case of Optifine-transformed code, there are.

edit: I'm not sure what's needed to get my code to compile under Eclipse. It's pretty much point'n'click with IDEA (Kotlin is made by JetBrains after all). You do have the Kotlin plugin installed, right?

CreativeMD commented 7 years ago

Actually i'm quite sure this issue is caused by my code. That's what i'm doing:

The first one seems to be alright no issues there, but the second seems to cause the issue. My local variable (tiles) is not visible in the crash log. If i remove the second part, the third part crashes. So I assumed my variable ends somewhere between the first injection and the second one.

There should be no shift of indexes. I set the index of my local var to 70 (for testing purposes to make sure this is not the problem).

I have installed the Kotlin plugin, but unfortunately i can't add it to my project. Unfortunately there is a lack of tutorials and just don't know how to fix it :( . I might need to setup up a clean workspace.

octarine-noise commented 7 years ago

I'm not sure you can specify the index of your local variable. They seem to be indexed in order of appearance, so ASM probably reorders them when writing out the bytecode (I think). If you look at the locals: part of the crashlog, your variable shows up at index 5, but with the "top" type (whatever that means).

I'm somewhat sure I'm not corrupting it, since I only insert some LOAD/STORE/INVOKE instructions much later in the method, and don't touch the frames or local variables at all. I'll do some testing though, and modify my transformer to write out the class as it receives it to check what my input is.

Just to make sure I understood you correctly: if you only include the local variable declaration and assignment (your first modification), and leave out the rest, the transformation goes through and Minecraft starts?

octarine-noise commented 7 years ago

Yep, as I thought. With the LittleTiles transformer active, this is what shows up on my input. There among the local variables of the method: <localVar:index=5 , name=tiles , desc=Ljava/util/List;, sig=Ljava/util/List<Lcom/creativemd/littletiles/common/tileentity/TileEntityLittleTiles;>;, start=L20, end=L21>

And this: <localVar:index=21 , name=blockrenderlayer1 , desc=Lnet/minecraft/util/BlockRenderLayer;, sig=null, start=L7, end=L5> is expected as variable 20 by my transformer. Changing the hardcoded index from 20 to 21 makes it work properly (and crash if BF is present without LittleTiles of course).

If you set a high enough SortingIndex, the problem will be solved. I think the SRG names will pose no problems for you at all, since you only use MCP names in your transformer. Just switch to using the srg-mcp.srg mapping file in TransformerNames. How do you load those, by the way? I can't find it in any jar file.

CreativeMD commented 7 years ago

Yes, tested it as well. Didn't know that it will assign indexes automatically. The thing is that more mods rely on that feature. So i would have to change that for all of them, or change the system to support both namespaces. The file can be found inside CreativeCore/com/creativemd/creativecore/transformer/notch-mcp.srg. I will try to come up with a solution. Thanks for you help anyway.

octarine-noise commented 7 years ago

No problem.

Although... I'm not 100% sure you are allowed to redistribute the MCP mappings as part of your mod. You'd better check with someone before you get into trouble.

Take care.