Java 24/25 support: recompute stale StackMapTables + JEP 486 launch #193
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| package io.github.crucible.asm; | ||
|
|
||
| import java.io.File; | ||
| import java.io.FileOutputStream; | ||
| import java.lang.reflect.Field; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| import io.github.crucible.CrucibleConfigs; | ||
| import net.minecraft.launchwrapper.IClassTransformer; | ||
| import net.minecraft.launchwrapper.Launch; | ||
|
|
||
| import org.objectweb.asm.ClassReader; | ||
| import org.objectweb.asm.ClassWriter; | ||
|
|
||
| /** | ||
| * Recomputes StackMapTables ({@code COMPUTE_FRAMES}) as the <b>last</b> transform-chain step so classes | ||
| * left with stale frames by upstream {@code COMPUTE_MAXS} transformers pass the split bytecode | ||
| * verifier. Uses the deobf-aware, never-throwing {@link SafeAsmClassWriter}. Runs last by self-reordering | ||
| * to the tail of the LaunchClassLoader transformer list (whole-list copy-and-swap). Never breaks a class: | ||
| * any failure returns the input unchanged. | ||
| * | ||
| * <p>This complements RFB's {@code rfb-asm-safety} plugin, which only makes {@code getCommonSuperClass} | ||
| * safe <em>when a transformer already recomputes frames</em>; it does not force a recompute on a class | ||
| * (like Cauldron's patched {@code ChunkProviderServer.func_73153_a}) whose stale StackMapTable was left by | ||
| * a {@code COMPUTE_MAXS}-only coremod/Mixin pass. Reflectively loading that class (e.g. Dynmap's field | ||
| * scan) throws {@code VerifyError: Expecting a stackmap frame at branch target N}. Measured on the | ||
| * lwjgl3ify server path: the full pack fails this way on every modern JDK tested (21-25); Java 8 is | ||
| * unaffected because its verifier fails over to the old type-inference verifier for these class files. | ||
| * This transformer forces the repair. | ||
| * | ||
| * <p>Set {@code -Dcrucible.frameSafety.debug=true} to log every recompute outcome and dump the pre/post | ||
| * bytes of classes named in {@code -Dcrucible.frameSafety.debugClass=...} (default: ChunkProviderServer) | ||
| * to /tmp. Disable the whole transformer with {@code -Dcrucible.frameSafety=false}. | ||
| */ | ||
| public final class AsmFrameSafetyTransformer implements IClassTransformer { | ||
|
|
||
| private static final Field TRANSFORMERS_FIELD = resolveTransformersField(); | ||
| private static final boolean DEBUG = Boolean.getBoolean("crucible.frameSafety.debug"); | ||
| private static final String DEBUG_CLASS = | ||
| System.getProperty("crucible.frameSafety.debugClass", "ChunkProviderServer"); | ||
| /** | ||
| * Only recompute classes whose (deobf) name starts with one of these comma-separated prefixes — | ||
| * the stale frames the verifier rejects come from COMPUTE_MAXS-only Mixin/coremod passes, | ||
| * which overwhelmingly target {@code net.minecraft.*}. Recomputing every class would needlessly read+ | ||
| * rewrite thousands of mod classes at load time. Configured via {@code crucible.asm.frameSafetyScope} | ||
| * in Crucible.yml ({@code all} or empty = everything). | ||
| */ | ||
| private static final String[] SCOPE = parseScope(CrucibleConfigs.configs.crucible_asm_frameSafetyScope); | ||
|
|
||
| private static String[] parseScope(String raw) { | ||
| if (raw == null) return new String[0]; | ||
| raw = raw.trim(); | ||
| if (raw.isEmpty() || raw.equalsIgnoreCase("all")) return new String[0]; // empty = everything | ||
| String[] parts = raw.split(","); | ||
| for (int i = 0; i < parts.length; i++) parts[i] = parts[i].trim(); | ||
| return parts; | ||
| } | ||
|
|
||
| private static boolean inScope(String transformedName) { | ||
| if (SCOPE.length == 0) return true; // "all" | ||
| if (transformedName == null) return false; | ||
| for (String p : SCOPE) if (!p.isEmpty() && transformedName.startsWith(p)) return true; | ||
| return false; | ||
| } | ||
|
|
||
| private static Field resolveTransformersField() { | ||
| try { | ||
| Field f = Launch.classLoader.getClass().getDeclaredField("transformers"); | ||
| f.setAccessible(true); | ||
| return f; | ||
| } catch (Throwable t) { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public byte[] transform(String name, String transformedName, byte[] basicClass) { | ||
| ensureRunsLast(); | ||
| if (basicClass == null) return null; | ||
| // never recompute our own writer/transformer, nor the external lwjgl3ify classes (which carry | ||
| // transformerExclusions and must reach the verifier untouched). | ||
| if (name != null && (name.startsWith("io.github.crucible.asm") | ||
| || name.startsWith("me.eigenraven.lwjgl3ify"))) return basicClass; | ||
| if (!inScope(transformedName)) return basicClass; | ||
|
|
||
| final boolean dbg = DEBUG && transformedName != null && transformedName.contains(DEBUG_CLASS); | ||
| if (dbg) dump(transformedName + ".pre", basicClass); | ||
| try { | ||
| ClassReader reader = new ClassReader(basicClass); | ||
| SafeAsmClassWriter writer = new SafeAsmClassWriter(reader, ClassWriter.COMPUTE_FRAMES); | ||
| reader.accept(writer, 0); | ||
| byte[] out = writer.toByteArray(); | ||
| if (dbg) { | ||
| dump(transformedName + ".post", out); | ||
| System.out.println("[frameSafety] recomputed OK: " + transformedName); | ||
| } | ||
| return out; | ||
| } catch (Throwable t) { | ||
| if (dbg) { | ||
| System.out.println("[frameSafety] recompute THREW for " + transformedName + ": " + t); | ||
| t.printStackTrace(System.out); | ||
| } | ||
| return basicClass; | ||
| } | ||
| } | ||
|
|
||
| private static void dump(String tag, byte[] bytes) { | ||
| try { | ||
| File f = new File("/tmp/framedbg-" + tag.replace('/', '.') + ".class"); | ||
| FileOutputStream fos = new FileOutputStream(f); | ||
| try { | ||
| fos.write(bytes); | ||
| } finally { | ||
| fos.close(); | ||
| } | ||
| System.out.println("[frameSafety] dumped " + f + " (" + bytes.length + " bytes)"); | ||
| } catch (Throwable ignored) { | ||
| // no-op | ||
| } | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| private void ensureRunsLast() { | ||
| final Field f = TRANSFORMERS_FIELD; | ||
|
Contributor
There was a problem hiding this comment. This is quite risky code with reflection. Even though we guarantee that the launchwrapper implementation is RFB and we can change this code if it changes, it's still a dirty hack.
Contributor
Author
There was a problem hiding this comment. You're right, and I agree the reflective reorder is fragile, it pokes LaunchClassLoader.transformers directly and only works as long as that field stays a List we're allowed to mutate. The reason it's there at all: this transformer has to run after every other IClassTransformer, because anything that transforms a class after we recompute its frames can re-introduce the stale StackMapTable we just fixed. Registration order alone doesn't guarantee that, so ensureRunsLast() moves us to the tail of the list on each transform() call. The obvious clean replacement seems to be an RFB RfbClassTransformer in the LCL_WITH_TRANSFORMS phase, that phase already appears to run after the legacy IClassTransformer chain (in LaunchClassLoader I see runTransformers(...) invoked first and runRfbTransformers(..., LCL_WITH_TRANSFORMS, ...) after it), so the ordering would be handled by RFB and the reflection would go away. I tried fairly hard to make that work and kept hitting frame-computation problems that I couldn't fully explain, so I'd appreciate your read on it. Here's what I tried, all on a full GTNH-style pack on GraalVM 25:
The part I can't reconcile: the same in-repo SafeAsmClassWriter recompute boots clean (server reaches Done, no VerifyError) when it runs as a legacy IClassTransformer reordered to last, but the same writer on the LCL_WITH_TRANSFORMS phase produces a bad frame for some net.minecraft classes. So it seems less like the writer itself and more like the bytes/type-resolution context differing between "legacy chain, last position" and "the RFB phase" — but I genuinely don't know the RFB contract well enough to say which difference matters. So for now I've kept the legacy IClassTransformer with the reflective tail-reorder, since that path boots reliably which is itself very much in the spirit of 1.7.10: code shaped by the 2014 toolchain, twelve years on, still somehow load-bearing. Doing it the clean way feels like it could mean untangling how the launch chain resolves types on that phase, and at that point that is rewriting half the server to win back one reflective field access. |
||
| if (f == null) return; | ||
| try { | ||
| final List<IClassTransformer> current = (List<IClassTransformer>) f.get(Launch.classLoader); | ||
| if (current == null || current.isEmpty()) return; | ||
| if (current.get(current.size() - 1) == this) return; | ||
| final int idx = current.indexOf(this); | ||
| if (idx < 0) return; | ||
| final List<IClassTransformer> next = new ArrayList<IClassTransformer>(current.size()); | ||
| for (int i = 0; i < current.size(); i++) { | ||
| if (i != idx) next.add(current.get(i)); | ||
| } | ||
| next.add(this); | ||
| f.set(Launch.classLoader, next); | ||
| } catch (Throwable ignored) { | ||
| // best-effort | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use com.gtnewhorizons.retrofuturabootstrap.asm.SafeAsmClassWriter. Why do we need to re-implement it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, and I tried that first. RFB's SafeAsmClassWriter has the flags-only constructor I need (new SafeAsmClassWriter(ClassWriter.COMPUTE_FRAMES) with no reader, so it can't fall into the "copies unchanged methods byte-for-byte and skips the recompute" trap), so on paper it's a drop-in and one less copy of this to maintain. But swapping it in regresses the full pack on GraalVM 25:
java.lang.NoClassDefFoundError: ahb
ahb is the obfuscated name of net.minecraft.world.World, so the recompute produced a class the JVM then can't define under its obf alias, i.e. the recomputed frames are wrong for that class. With the in-repo writer the same boot reaches Done cleanly.
As far as I can tell the difference is deobfuscation. The whole job of getCommonSuperClass during COMPUTE_FRAMES is to find the common supertype of two types without loading them, and on 1.7.10 those types are obfuscated and not yet loadable. The in-repo writer's getCommonSuperClass resolves the hierarchy through FMLDeobfuscatingRemapper (obf↔srg) and reads class headers off the LaunchClassLoader, so it gets the right supertype for a transformed net.minecraft class; RFB's appears to resolve names directly, which on these classes lands on the wrong supertype and emits a frame that doesn't verify. It's the same deobf-awareness gap I hit trying to move the whole pass onto the RFB phase...
This is also the exact failure mode the PR exists to fix in the first place, an imprecise getCommonSuperClass widening a type and producing a bad frame, so using a writer that isn't deobf-aware would reintroduce the bug it's meant to repair.
So I've kept the in-repo writer, but I'd rather not if RFB's can cover it. Is RFB's SafeAsmClassWriter.getCommonSuperClass meant to be deobf-aware when used on FML-remapped net.minecraft classes and if so, is there something I should be passing it (a loader, a remapper) to make it resolve the obf hierarchy correctly..?