Open Barteks2x opened 3 years ago
I don't see much wrong with modifying the stack directly, perhaps even multiple stack elements at once, this is useful for adding data to builders:
public static Foo createFoo();
Code:
invokestatic ... // Foo.createBuilder
getstatic .. // TargetClass.VALUE_X
invokestatic .. // FooBuilder.valueX
getstatic .. // TargetClass.VALUE_Y
invokestatic .. // FooBuilder.valueY
getstatic .. // TargetClass.VALUE_Z
invokestatic .. // FooBuilder.valueZ
<modify the FooBuilder here>
invokestatic .. // FooBuilder.build
areturn
Forgive my ignorance if I am missing out on something, but isn't this achievable with @Inject(at = @At("RETURN"), cancelable = true)
and CallbackInfoReturnable
?
Forgive my ignorance if I am missing out on something, but isn't this achievable with
@Inject(at = @At("RETURN"), cancelable = true) and CallbackInfoReturnable
?
It appears so, CallbackInfoReturnable.getReturnValue
contains the old return value, although this does create bloated CallbackInfoReturnable
infrastructure that we never use. It also does not quite handle my use case shown above.
Thinking about it a bit, both of these use cases just seems like more general use of ModifyArg
/ModifyArgs
, applied to injection points other than invokes (not currently possible). I'm still in favor of a new @Decorator
(ModifyStack
?) just for documentation purposes.
Another thought: I'm not quite certain how (or even if) we can type check if we allow multiple stack elements, since the JVM doesn't seem to expose this, but for most opcodes finding the type of the item at the top of the stack right before and/or after an opcode is trivial (example: if we inject before dreturn
, then the top of the stack should always be a double
, if we inject after newarray
, then the top of the stack should always be an array of some known type).
We could simply disallow ambiguous injection points. And if we know the item of the top of the stack is T
, we can invokestatic
a function static T changeStack(T item);
, and then the type at the top of the stack will remain unchanged, all while injecting just a single opcode. I think that handles the majority of use cases in a very simple way.
For more natural injection using a member method T changeStack(T item);
, we could also:
<any opcode> # oldItem
aload_0 # oldItem this
swap # this oldItem
invokevirtual # newItem
For long
/double
, we would still have have to store
/load
into a temporary local variable, since there is no swap
for those.
Or, if the opcode does not pop any elements (example: LOAD_CONSTANT
/LOAD
/INVOKE
with 0 arguments):
aload_0 # this
<opcode*> # this oldItem
invokevirtual # newItem
Either way, this is only a couple of opcodes, and no local variable usage, much better than @ModifyArg
and definitely better than CallbackInfoReturnable
.
Also, it seems theoretically feasible to use static callgraph analysis to resolve ambiguous stack types so we can resolve ambiguous stack elements and even allow modification of multiple arbitrary stack elements at once just like @ModifyArgs
, but the best use case for that I can think of is to modify arguments, and we already have an injector for that.
EDIT: Also, there is StackMapTable
, but that is usually incomplete.
Usually, when you want to change the stack, it is right before a pop or right after a push. I see no reason to restrict the usage of that to only apply to return values.
Another example I ran across where something like @ModifyStackTop
is necessary, in a 400 opcode function that can't be easily redirected:
// I want to transform this:
double a = b + x * CONSTANT;
// into this
double a = b + modify(x) * CONSTANT;
// but currently, I have to do this:
double a = b + x * CONSTANT;
a = modify(a);
// and then find+subtract b and divide by CONSTANT again, hoping I don't lose precision.
Update: I decided to take a spin at implementing this feature like the theoretical implementation outlined above. It didn't turn out to be very difficult, as expected.
I also had to implement three new injection points for the feature to be useful:
BeforeOpcode (OPCODE)
AfterOpcode (OPCODE_RESULT)
AfterInvokeResult (INVOKE_RESULT)
I would like to know if there is interest to add this to the library so I can create a pull request, otherwise I will just turn it into a library.
Some refactoring of injection points might also be in order, since most injection points just look for instructions matching a predicate, which is a lot of duplicated code.
Example usage:
@Mixin(TitleScreen.class)
public class TitleScreenMixin {
@Inject(method="init()V", at=@At("HEAD"))
private void onInject(CallbackInfo ci) {
System.out.println("Normal injection");
}
// This is the equivalent of the CallbackInfoReturnable method mentioned above, but much cleaner
@ModifyStackTop(method="areRealmsNotificationsEnabled()Z", at=@At(value="TAIL"), require=1, allow=1)
private static boolean changeReturnValue(boolean b) {
return false;
}
// This seems to be the actual feature requested in the issue
// Changes `if (this.client.isDemo()) { ... }` to
// `if (changeIsDemoResult(this.client.isDemo())) { ... }`
@ModifyStackTop(method="init()V",
at=@At(value="INVOKE_RESULT", target="Lnet/minecraft/client/MinecraftClient;isDemo()Z", ordinal=0),
require=1, allow=1)
private static boolean changeIsDemoResult(boolean isDemo) {
System.out.println("Is this a demo version? " + isDemo);
return true; // We are now!
}
// Change the result of a non-function call.
@ModifyStackTop(method="initWidgetsDemo(II)V",
at=@At(value="OPCODE_RESULT", opcode=Opcodes.IDIV, ordinal=0),
require=1, allow=1)
private static int onDivision(int halfWidth) {
return halfWidth / 2;
}
// or, equivalently:
/*
@ModifyStackTop(method="initWidgetsDemo(II)V",
at=@At(value="OPCODE", opcode=Opcodes.IDIV, ordinal=0),
require=1, allow=1)
private static int onDivision(int divisor) {
return divisor * 2;
}
*/
}
// This is the equivalent of the CallbackInfoReturnable method mentioned above, but much cleaner
@ModifyStackTop(method="areRealmsNotificationsEnabled()Z", at=@At(value="TAIL"), require=1, allow=1)
private static boolean changeReturnValue(boolean b) {
return false;
}
But how would this behave if there are multiple return points within the same method?
@Aizistral
The callback will be automatically injected into every single injection point, just like any other injector, so if you specify @At("RETURN")
, your callback will get called for every single return point.
The ModifyStackTop
doesn't decide where to inject the callback, if that's what you're concerned about.
To be honest, I don't see how this is better than CallbackInfoReturnable
for the particular purpose of modifying returned value, knowing CallbackInfoReturnable
was designed specifically to handle this. Your ModifyStackTop
thing apparently has uses other than this, but since this issue was initially written as request for @ModifyReturnValue
feature, I wish you could explain more clearly what exactly isn't good with way of achieving such functionality we already have at our disposal.
The CallbackInfoReturnable
method only works for my use case, it doesn't actually do what's requested in the issue (which is not possible at all right now). Being able to avoid 20/30 opcodes including memory allocations caused by CallbackInfoReturnable
is just a nice side effect I liked to show off.
The showcase for the actual feature request is the changeIsDemoResult
function, which changes the result of the this.client.isDemo()
function inside of TitleScreen#init()
before it is processed by the if
statement.
Ok, makes sense now. Somehow I've just been missing out on the key point.
I just ran into this missing feature myself, and would also greatly appreciate the addition of an annotation that allows me to modify the return value of a function call without completely redirecting it.
You could consider using this in the meantime https://github.com/LlamaLad7/MixinExtras/wiki/ModifyExpressionValue
You could consider using this in the meantime https://github.com/LlamaLad7/MixinExtras/wiki/ModifyExpressionValue
That looks like exactly what I need. Thank you! :D
Does it support capturing locals?
Does it support capturing locals?
Not currently, as with all stock injectors except Inject
itself. Depending on your specific situation there's often ways around it, and in many cases relying on local capture isn't a great idea anyway, though discussions of specific situations like that would be best placed in the appropriate channels in the forge, fabric, or sponge discord.
Explanation
A
@ModifyReturnValue
would target method calls and take the return value of the method as an argument, and return the modified return value. Additional parameters as with other injectors could also be allowed.In the case of only one mixin modifying a given method, it's already possible to do the same thing with
@Redirect
that just calls the original method, and then applies it's changes to the return value. And this is a common use case for redirects that I can see. The main issue with this approach is that multiple mixins can't modify return value of the same method. Sometimes similar cases can be solved with very specific use ofModifyArg
andModifyVariable
but they are generally more fragile, less direct about the intent and not always possible.@ModifyReturnValue
could allow multiple mixins to target the same method call, as the original method call remains there, and the order in which return value changes are applied would depend on mixin application order (mixin priority).Example
Consider the following code
If we consider 2 mods, one of which wants to change the code to effectively do this:
And another that wants to change it to
both mods using
ModifyReturnValue
with appropriate priorities would result in transformed code like thisThis result is, to my knowledge, currently impossible without close cooperation of both mods.
As explained on discord, II also thought about extending it all the way to "modify top of the stack" as it seemed like quite a natural way to extend it, with what is targeted being specified just by the injection point, but it did seem like something way too powerful, difficult to verify correctness of, and prone to misuse. But I can also see this being extended for field access, and maybe other specific cases.
This could still open the possibility of modders using it incorrectly, and just completely replacing the return value, as if it was just a redirect, simply because of being told that it's better to use this instead of a redirect, and blindly applying it.