SpongePowered / Mixin

Mixin is a trait/mixin and bytecode weaving framework for Java using ASM
MIT License
1.43k stars 193 forks source link

Access target MethodNode from custom InjectionPoint (to create ASM Analyzer) #310

Open Barteks2x opened 5 years ago

Barteks2x commented 5 years ago

What I'm trying to do:

I'm trying to make a custom injection point that can filter method by constant arguments to them. For example I want it to be able to accept someMethod(0, something) but not someMethod(something, somethingElse). ASM offers a way to do it with a custom Interpreter class an an Analyzer. Unfortunately, Analyzer requires an instance of MethodNode, which an InjectionPoint has no access to. Since it makes a lot of sense to be able to use Analyzer in custom InjectionPoint, I'm asking for a way to access the target MethodNode or at least some way to create an Analyzer instance.

The way to do it that I'm currently in the middle of doing (and will probably take a lot of time, and may have enough undebuggable-to-me bugs, that I may never finish it) is to duplicate the Analyzer, Subroutine and Frame code in a way that doesn't rely on any of the MethodNode data (which is hard because there is no access to local count and stack size).

Another possible solution without Analyzer: work backwards from the found methods to somehow find where a given method argument appears on the stack. I don't expect it to be anywhere close to easy, and it still leaves other situations in which Analyzer could be useful.

Injecting into both and checking constant value won't work because the constant 0 is a valid argument in the first case too, and the replacement wouldn't be valid. Using a @Slice would work fine, but there really isn't anything specific about this place in the target method, just that this one method call has constant 0 as one of the arguments.

My actual problem and what I was trying to do and why:

I recently found another (core)mod that I wanted working with my mod. Eventually (after fixing a bunch of other issues in it) I discovered that one change it's doing breaks one of my mixins, even with the actual intended target point not being changed at all.

The real issue was that the other coremod overwrote the first usage of the same method (that I didn't want to change) with it's own hook. The real distinguishing factor of the second call was that the second argument was the constant 0 (and constants 0 are in a lot of places, so ModifyConstant would be even more brittle, and would fail mostly silently). I've seen the same pattern in a few different places in my code, where I remember using ordinal or slice instead, so I thought it would be a good idea to write a custom InjectionPoint. After writing my custom interpreter that keeps track of constants I hit the wall - mixin doesn't give me MethodNode.

Mumfrey commented 5 years ago

The main reason injection points currently only receive the instruction list is for defensive purposes, the instruction list the injection points receive as part of their operation is read-only (and may be only a partial instruction list since it can be returned from a slice. The main reason for not including the target MethodNode is that there wasn't any need for the existing injection points to use it, and making a readonly wrapper would have been unnecessary overhead.

However given that you've articulated a use-case for having the MethodNode in some circumstances I feel like it would be reasonable to be able to obtain a read-only methodnode for the purposes of feeding it to an analyser or accessing things like the LVT. This might need to be on an on-demand basis so that Injection Points which do not require the MethodNode (eg. most of them) wouldn't cause a performance penalty. I'm not sure the creation of a ReadOnlyMethodNode in a similar way to ReadOnlyInsnList would be reasonable for each injection pass, since it could not be a thin wrapper thanks to the copious public variables in MethodNode which preclude defensive wrapping. This means that for the ReadOnlyMethodNode you'd need to initialise all of the public List<> members as readonly lists, the instructions as ReadOnlyInsnList etc. ASM's surface area is set up to prioritise performance over encapsulation so a lot of structures are public and not accessed via getters.

Some delegate via which the ReadOnlyMethodNode could be constructed on-demand seems like the most reasonable way to deal with this.


I should point out that there's a work-around you can use right now which leverages companion plugins. Note that before a Mixin is applied to a target class the preApply method is called and receives the ClassNode of the target and the name of the mixin being applied. It would be possible to write companion code which could match a particular mixin/target combo and performed the necessary analysis on the target method(s) then stored the results for later use by the injection point. Other transformers should have already run on the target class code at this point so the changes in question should be present.