Mumfrey
commented
3 years ago The pass I made was specifically for making injectors more consistent given the incredibly inconsistent ways that frames can turn up in the bytecode. All this state-machine nonsense is only necessary because the frames (especially when generated by ASM) don't tell the whole story, and every iteration on Locals is attempting to get closer to what the user expects. Locals are by nature a brittle thing and every change is attempting to reduce that brittleness.
Can I just say that while I appreciate the issue, your timing sucks as I've kept these changes in snapshots trying to get people to find regressions for ages because I knew the changes would result in some different results (that's the intention) but there shouldn't have been any broken ones. I literally promoted this to release on friday and your issue was opened the next day! Anyway.
I note that the examples you gave are both fabric/quilt mods. The changes made were actually made specifically to address some situations in fabric where locals that should still have been in scope from the user's point of view were being trimmed too early, and where ASM's frame calculations aren't reliable enough. The expected result is that some things previously cut from the frame but intuitively in-scope should now be included, at the expense of some things that were "leaking" previously potentially being removed.
Unfortunately testing with fabric is a massive pain at the moment, as the hacky way to get it to let me use my development version of mixin with a fabric mod causes it to not load a bunch of their mixins and therefore the game doesn't run. I've introduced a new method of obtaining the transformer in 0.8.3 so I'm hoping this will get simpler soon. In the mean time if you have any MCP versions of the same regressions that would help in troubleshooting this.
This example target method features a float in the descriptor, two float variables before the mixin at-location and a F_NEW frame between the variables and the at-location:
FSTORE FSTORE F_NEW <at-loc>. The F_NEW frame moves those variables out of (JVM) scope by resetting the locals to only the method parameters. Older Mixin versions use the variables defined in the frame, where the only float is the method parameter, latest Mixin appears to include the two floats that went out of scope.
Either I'm reading this wrong or there's some part of this puzzle that I'm missing. The target method looks like this:
protected void applyDamage(DamageSource source, float amount) {
if (!this.isInvulnerableTo(source)) {
amount = this.applyArmorToDamage(source, amount);
amount = this.applyEnchantmentsToDamage(source, amount);
float f = amount;
amount = Math.max(amount - this.getAbsorptionAmount(), 0.0F);
this.setAbsorptionAmount(this.getAbsorptionAmount() - (f - amount));
float g = f - amount;
if (g > 0.0F && g < 3.4028235E37F) {
this.increaseStat(Stats.DAMAGE_ABSORBED, Math.round(g * 10.0F));
}
if (amount != 0.0F) {
this.addExhaustion(source.getExhaustion());
float h = this.getHealth();
this.setHealth(this.getHealth() - amount);
this.getDamageTracker().onDamage(source, h, amount);
if (amount < 3.4028235E37F) {
this.increaseStat(Stats.DAMAGE_TAKEN, Math.round(amount * 10.0F));
}
}
}
}and I think it's intuitive to say that the floats f and g are in scope before the injection point (at getHealth) and as far as I can tell there's no F_NEW in the bytecode for the method. So this is at least one case where I'd say the locals capture has been improved, as it's capturing variables which are intuitively in scope from the user's point of view.
The choice of implicit discriminator in a method which definitely contains other floats could have been prevented by the mixin author specifying argsOnly (which is more efficient anyway) or not using an implicit disciminator at a location other than HEAD (which is the primary use-case for implicit discriminators anyway).
The example case breaks implicit LV selection (1 float -> 3 floats = ambiguous = InvalidImplicitDiscriminatorException), explicit specification with ordinal could easily start referencing the wrong LV slot in other cases. From my reading of the Mixin code it would treat F_CHOP differently from F_NEW even if both resulted in the same frame state.
Probably, though the stretchiness of the frame is embedded in the heuristic knownFrameSize variable which is a confidence-based expansion of the frame based on locals we have computed but have been "chopped" by F_NEW or F_CHOP but are confident are still realistically in scope. This helps in situations where the variables are about to go out of scope, but aren't actually out of scope but have been declared out of scope by the frame because a join point is the next instruction.
Now since this is almost too easy, another case at https://github.com/Juuxel/Adorn/blob/1.16.5/src/main/java/juuxel/adorn/mixin/BubbleColumnBlockMixin.java#L25 does apparently quite the opposite. The target method features two returns, one with the requests LV in scope, the other without (F_NEW clears all locals before 2nd IRETURN). Old Mixin doesn't care, new Mixin can't find the out of scope local.
Again, I think we're looking at different bytecode because the one I see (in my available Fabric 1.16.5 workspace) is F_SAME1 but the point still stands. This is a better example of something which is probably a regression (I'd argue that the previous example is actually an improvement since it allows capturing of variables which are intuitively in scope).
This kind of situation where something is lopped out of the frame by the stack map itself but still intuitively in-scope is what the update was meant to improve, and neatly demonstrates what knownFrameSize is trying to achieve. The fact that it misses this case is interesting and I'll look into it.
Since Mixin uses COMPUTE_FRAMES, it can and does resurrect out of scope LVs (at least some? uninit likely breaks) in some cases.
Mostly this is only an issue if the variable hasn't been initialised, as you say COMPUTE_FRAMES does handle the rest quite gracefully. The intention, as mentioned, is that "resurrected" LVs are the ones which look like they're in scope but the VM has cropped them from the frame because they're not used any more, rather than them being actually out of scope.
For a mod loader the problem is quite difficult to tackle, the mods can't be updated reasonably, even less so with retro packs that are otherwise perfectly compatible with the latest loader version. The status quo looks to remain inconsistent as the above examples exhibit opposite behaviors, indicating another incompatible change is in order.
I don't really know what to tell you, as I said I left this in SNAPSHOT for absolutely ages on purpose so that people could test for regressions. Perhaps encourage fabric/quilt to have a snapshot/bleeding branch which pulls my snapshots more frequently so that people who want to test things out can report them upstream.
On the one hand there's always pressure to move more quickly, and on the other I want this to be a stable library people can rely on and with something as brittle as locals are it's just impossible to find a balance that will suit everyone. Locals have improved a lot in recent versions and I think people would rather I make moves toward greater consistency overall than just throw up my hands and say "it's broken and as good as it's gonna get" and leave it at that. This gets more relevant in 0.10 because that will support local capture in all injectors.
Are LV references supposed to work off the frames, the entirety of initialized LV slots (considering all branches, IMO the best option) or all LV slots including uninitialized (probably invalid)?
I think I've answered this above but the goals for locals are:
- Capture things which are intuitively in scope as far as possible.
- Provide the same result even when the frames in the code change (eg. platform changes, people making mods for both Forge and Quilt/Fabric) - the 0.8.3 does this in a big way, I could copy-paste injectors between workspaces and just change the targets
The contract of locals is always going to be "best effort" because without going full "analytical decompiler" it's just impossible to come up with something which is both reasonably efficient and reasonably consistent. Note "reasonably" because I don't have any delusions that it's ever gonna be perfect.
If fabric/quilt want to ignore my upstream changes or at least commute them to major version bumps (when people expect to have to update anyway) then they can do that. The external contract of Locals hasn't changed so they can roll back if they want to preserve the old (imo worse) behaviour for consistency sake. They already change an unknown number of things from upstream so this one more probably won't hurt too much. Then the 1.17 release can have the tweaked method, brining the aforementioned better parity between injectors on different platforms.
I don't have all the answers and I've never claimed to, I just try to make this stuff work the best it can. The fabric peeps all know where I am on discord if they want to discuss these things, the sponge channel is platform-agnostic and just for discussing mixin in general.
How should one deal with these incompatibilities (I don't have a good answer there)?
I don't have a good answer either. About the only thing I can think is adding some kind of versioning to the locals detection to allow mixin authors to choose (or at least pin support for) a specific version of locals capture (or maybe just a general purpose selection of internal APIs) so that it's possible to have improvements as they come for those that choose it, and pinned stability for the others. Though the general ambivalence towards the versioning mechanisms that I do provide makes me think nobody would use that anyway!
Local variables were a mistake, we should never have moved out of the caves, life was simpler when being eaten by a bear was your no1 worry.
sfPlayer1
I'm running into some issues where (as far as I can tell) https://github.com/SpongePowered/Mixin/commit/6f5dce173b44b7be55252cd5dfcca6f064ad2c99#diff-4bd581bd3412aebda2607dc2b9421fe52b2788cc301ca7049a3bcfeb1b1d035cL203 breaks some LVT sensitive uses such as https://github.com/MoriyaShiine/bewitchment/blob/1.16/src/main/java/moriyashiine/bewitchment/mixin/transformation/PlayerEntityMixin.java#L261 .
This example target method features a float in the descriptor, two float variables before the mixin at-location and a F_NEW frame between the variables and the at-location:
FSTORE FSTORE F_NEW <at-loc>. The F_NEW frame moves those variables out of (JVM) scope by resetting the locals to only the method parameters. Older Mixin versions use the variables defined in the frame, where the only float is the method parameter, latest Mixin appears to include the two floats that went out of scope.The example case breaks implicit LV selection (1 float -> 3 floats = ambiguous = InvalidImplicitDiscriminatorException), explicit specification with ordinal could easily start referencing the wrong LV slot in other cases. From my reading of the Mixin code it would treat F_CHOP differently from F_NEW even if both resulted in the same frame state.
Now since this is almost too easy, another case at https://github.com/Juuxel/Adorn/blob/1.16.5/src/main/java/juuxel/adorn/mixin/BubbleColumnBlockMixin.java#L25 does apparently quite the opposite. The target method features two returns, one with the requests LV in scope, the other without (F_NEW clears all locals before 2nd IRETURN). Old Mixin doesn't care, new Mixin can't find the out of scope local.
Since Mixin uses COMPUTE_FRAMES, it can and does resurrect out of scope LVs (at least some? uninit likely breaks) in some cases.
For a mod loader the problem is quite difficult to tackle, the mods can't be updated reasonably, even less so with retro packs that are otherwise perfectly compatible with the latest loader version. The status quo looks to remain inconsistent as the above examples exhibit opposite behaviors, indicating another incompatible change is in order.
Are LV references supposed to work off the frames, the entirety of initialized LV slots (considering all branches, IMO the best option) or all LV slots including uninitialized (probably invalid)? How should one deal with these incompatibilities (I don't have a good answer there)?