SpongePowered / Mixin

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

INVOKE_ASSIGN gets stopped by CHECKCASTs #653

Closed quat1024 closed 3 weeks ago

quat1024 commented 7 months ago

Here's an assignment from net.minecraft.world.entity.ai.sensing.TemptingSensor#doTick (1.20.1 forge 47.1.3):

List<Player> list = p_148331_.players().stream().filter(EntitySelector.NO_SPECTATORS).filter((p_148342_) -> {
   return TEMPT_TARGETING.test(p_148332_, p_148342_);
}).filter((p_148335_) -> {
   return p_148332_.closerThan(p_148335_, 10.0D);
}).filter(this::playerHoldingTemptation).filter((p_264964_) -> {
   return !p_148332_.hasPassenger(p_264964_);
}).sorted(Comparator.comparingDouble(p_148332_::distanceToSqr)).collect(Collectors.toList());

The instruction stream for this assignment ends like this:

INVOKESTATIC java/util/Comparator.comparingDouble (Ljava/util/function/ToDoubleFunction;)Ljava/util/Comparator; (itf)
INVOKEINTERFACE java/util/stream/Stream.sorted (Ljava/util/Comparator;)Ljava/util/stream/Stream; (itf)
INVOKESTATIC java/util/stream/Collectors.toList ()Ljava/util/stream/Collector;
INVOKEINTERFACE java/util/stream/Stream.collect (Ljava/util/stream/Collector;)Ljava/lang/Object; (itf)
CHECKCAST java/util/List
ASTORE 4

I wanted to inject into doTick after the .collect call and pick up the List<Player>, so I used an INVOKE_ASSIGN injection with local capture. (This felt more appropriate than a modifyvariable, since I also needed to pick up a few method params.)

@Inject(
  method = "doTick(Lnet/minecraft/server/level/ServerLevel;Lnet/minecraft/world/entity/PathfinderMob;)V",
  at = @At(value = "INVOKE_ASSIGN", target = "Ljava/util/stream/Stream;collect(Ljava/util/stream/Collector;)Ljava/lang/Object;", shift = At.Shift.BY, by = 2),
  locals = LocalCapture.PRINT
)

However, the local variable table didn't include list. The injector actually ended up targeting the CHECKCAST instruction before the local was assigned.

/*       Target Class : net.minecraft.world.entity.ai.sensing.TemptingSensor                                     */
/*      Target Method : protected void doTick(ServerLevel p_148331_, PathfinderMob p_148332_)                    */
/*  Target Max LOCALS : 6                                                                                        */
/* Initial Frame Size : 3                                                                                        */
/*      Callback Name : quark$findTroughs                                                                        */
/*        Instruction : InjectionNode CHECKCAST                                                                  */

It looks like the AfterInvoke injector wants to advance over the INVOKE & over any _STORE instructions immediately following it, but the CHECKCAST got in the way. https://github.com/SpongePowered/Mixin/blob/155314e6e91465dad727e621a569906a410cd6f4/src/main/java/org/spongepowered/asm/mixin/injection/points/AfterInvoke.java#L84-L87

In this case it's workaroundable with shift = At.Shift.BY, by = 2 or targeting a later part of the method.

LlamaLad7 commented 7 months ago

This is already tracked I believe, but regardless LocalCapture + INVOKE_ASSIGN would be a very brittle choice here. Consider instead @ModifyVariable and @At("STORE"), or even better https://github.com/LlamaLad7/MixinExtras/wiki/ModifyExpressionValue if you have MixinExtras available.

quat1024 commented 7 months ago

Ahhh i forgot modifyvariable can also include the method parameters x) was confusing it with ModifyArg