Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Renaming an interface method that has bridges renames one of the bridges #212

Open
770grappenmaker opened this issue Jul 27, 2024 · 6 comments
Labels
backend affects the enigma backend bug Something isn't working

Comments

@770grappenmaker
Copy link
Contributor

For example, when mapping QM, I stumbled upon the class net/minecraft/unmapped/C_bhbelvxt, which appears to be some sort of collection type that extends the capabilities of a Deque with some of the methods from List. Its first method is m_dnwfaduj, but of course since I haven't mapped it yet enigma draws it as gray (the proposed name is reversed, which seems correct). If I then give it a name (either by marking as unmapped or renaming) it has two unexpected behaviours:

  • The overload present in the class net/minecraft/unmapped/C_scihbqqa, an implementation of C_bhbelvxt, does not get remapped (which is a consequence of the next behaviour, probably)
  • The .mapping file unexpectedly gets an entry of METHOD reversed reversed ()Ljava/util/Deque;, which obviously is wrong since I mapped the method m_dnwfaduj()Lnet/minecraft/unmapped/C_bhbelvxt;, not reversed()Ljava/util/Deque;

This might have something to do with the fact a name is proposed.

I tried overwriting the entry in the mapping file as METHOD m_dnwfaduj reversed ()Lnet/minecraft/unmapped/C_bhbelvxt;, and it appears that enigma straight up ignores it. Saving the mappings or requesting to drop invalid mappings does not seem to have an effect.

@770grappenmaker
Copy link
Contributor Author

770grappenmaker commented Jul 27, 2024

Update: it does not have something to do with the fact a name is proposed. Manually building enigma to strip the call to registerSpecializedMethodNamingService indeed removes the bridge method name proposal, but then if I rename it to reversed, or any other name apparently, it does now update the name to appear as "mapped", but the following lines are added to its .mapping file:

	METHOD reversed reversed ()Ljava/util/Deque;
	METHOD reversed reversed ()Ljava/util/SequencedCollection;

Additionally, by removing the proposal, the decompilation of the implementation, net/minecraft/unmapped/C_scihbqqa, successfully remaps, which is extra weird since the return types do not match.

@770grappenmaker
Copy link
Contributor Author

Update: I decided to poke around in the code, and interestingly, in EntryRemapper#doPutMapping, if I print the "resolved entries" (I guess this is for resolving overloads etc?), it reads

Resolved [net/minecraft/unmapped/C_bhbelvxt.reversed()Ljava/util/Deque;, net/minecraft/unmapped/C_bhbelvxt.reversed()Ljava/util/SequencedCollection;] while putting net/minecraft/unmapped/C_bhbelvxt.m_dnwfaduj()Lnet/minecraft/unmapped/C_bhbelvxt;

@770grappenmaker
Copy link
Contributor Author

If I straight up disable bridge indexing, the issue is fixed. (commenting this.getIndex(BridgeMethodIndex.class).findBridgeMethods();)

@IotaBread
Copy link
Member

Update: I decided to poke around in the code, and interestingly, in EntryRemapper#doPutMapping, if I print the "resolved entries" (I guess this is for resolving overloads etc?), it reads

Of course it's an entry resolving issue

@ix0rai
Copy link
Member

ix0rai commented Jul 28, 2024

pretty much the most classic component to break

@770grappenmaker
Copy link
Contributor Author

770grappenmaker commented Jul 28, 2024

I can see why this problem hasn't occurred yet before: according to a quick script I wrote, the following classes (mapped if name is known for the latest build of QM) have potential bridge methods:

net/minecraft/unmapped/C_bhbelvxt
net/minecraft/village/VillagerDataContainer
net/minecraft/client/option/Option$IntRangeBaseValueSet

(the strategy for determining these is basically checking whether for some interface there exists a method such that it is not ACC_PRIVATE, ACC_STATIC or ACC_FINAL, and calls only a single other method, apparently proguard strips ACC_BRIDGE?)
The only one that has a bridge method related to a java/ type is our perpetrator C_bhbelvxt... can't believe this bug occurs for a single class...

For reference, I used this snippet of code

val MethodNode.isPotentialBridge get() =
    access and ACC_SYNTHETIC != 0
            && access and ACC_PRIVATE == 0
            && access and ACC_STATIC == 0
            && access and ACC_FINAL == 0
            && instructions.countIsInstance<MethodInsnNode>() == 1

fun main() {
    JarInputStream(Path("""/path/to/1.21-hashed.jar""").inputStream()).use { input ->
        val itfWithBridge = generateSequence { input.nextJarEntry }
            .filter { it.name.endsWith(".class") }
            .map { ClassReader(input.readBytes()) }
            .filter { it.access and ACC_INTERFACE != 0 }
            .filter { reader ->
                ClassNode().also { reader.accept(it, ClassReader.SKIP_DEBUG) }.methods.any { it.isPotentialBridge }
            }.map { it.className }

        val mappingsPath = """/path/to/quilt-mappings-1.21+build.9-mergedv2/mappings/mappings.tiny"""
        val mappings = Path(mappingsPath).useLines { TinyMappingsV2Format.parse(it) }
        val remapper = SimpleRemapper(mappings.asASMMapping(
            from = "hashed",
            to = "named",
            includeMethods = false,
            includeFields = false
        ))

        itfWithBridge.forEach { println(remapper.map(it)) }
    }
}

@ix0rai ix0rai added bug Something isn't working backend affects the enigma backend labels Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend affects the enigma backend bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants