Fix inlining of functions with matching JVM signatures in same package

When two functions with matching JVM signatures in the same package were inlined sequentially, inliner could take a wrong method body from the cache due to MethodId clash. This manifested with inline classes, but also was possible with some legal Kotlin overloads with matching erasure. Use internal name of the corresponding implementation owner class instead of FQN of the containing declaration. Such MethodIds can't match accidentally, because corresponding JVM method signatures would clash.
2018-08-20 11:47:30 +03:00
parent f9bba3ca86
commit b5f37d18b2
8 changed files with 125 additions and 60 deletions
@@ -18,10 +18,9 @@ package org.jetbrains.kotlin.codegen.inline

 import com.intellij.util.containers.SLRUMap
 import org.jetbrains.kotlin.name.ClassId
-import org.jetbrains.kotlin.name.FqName
 import org.jetbrains.org.objectweb.asm.commons.Method

-data class MethodId(val containingFqName: FqName, val method: Method)
+data class MethodId(val ownerInternalName: String, val method: Method)

 class InlineCache {
    val classBytes: SLRUMap<ClassId, ByteArray> = SLRUMap(30, 20)
@@ -13,7 +13,10 @@ import org.jetbrains.kotlin.codegen.*
 import org.jetbrains.kotlin.codegen.AsmUtil.getMethodAsmFlags
 import org.jetbrains.kotlin.codegen.AsmUtil.isPrimitive
 import org.jetbrains.kotlin.codegen.context.ClosureContext
-import org.jetbrains.kotlin.codegen.coroutines.*
+import org.jetbrains.kotlin.codegen.coroutines.createMethodNodeForCoroutineContext
+import org.jetbrains.kotlin.codegen.coroutines.createMethodNodeForIntercepted
+import org.jetbrains.kotlin.codegen.coroutines.createMethodNodeForSuspendCoroutineUninterceptedOrReturn
+import org.jetbrains.kotlin.codegen.coroutines.isBuiltInSuspendCoroutineUninterceptedOrReturnInJvm
 import org.jetbrains.kotlin.codegen.intrinsics.bytecode
 import org.jetbrains.kotlin.codegen.intrinsics.classId
 import org.jetbrains.kotlin.codegen.state.GenerationState
@@ -500,7 +503,8 @@ abstract class InlineCodegen<out T : BaseExpressionCodegen>(
            else
                mangleSuspendInlineFunctionAsmMethodIfNeeded(functionDescriptor, jvmSignature.asmMethod)

-            val methodId = MethodId(DescriptorUtils.getFqNameSafe(functionDescriptor.containingDeclaration), asmMethod)
+            val owner = state.typeMapper.mapImplementationOwner(functionDescriptor)
+            val methodId = MethodId(owner.internalName, asmMethod)
            val directMember = getDirectMemberAndCallableFromObject(functionDescriptor)
            if (!isBuiltInArrayIntrinsic(functionDescriptor) && directMember !is DeserializedCallableMemberDescriptor) {
                return sourceCompilerForInline.doCreateMethodNodeFromSource(functionDescriptor, jvmSignature, callDefault, asmMethod)
@@ -20,6 +20,7 @@ import org.jetbrains.kotlin.codegen.*;
 import org.jetbrains.kotlin.codegen.binding.CodegenBinding;
 import org.jetbrains.kotlin.codegen.binding.MutableClosure;
 import org.jetbrains.kotlin.codegen.coroutines.CoroutineCodegenUtilKt;
+import org.jetbrains.kotlin.codegen.inline.FictitiousArrayConstructor;
 import org.jetbrains.kotlin.codegen.signature.AsmTypeFactory;
 import org.jetbrains.kotlin.codegen.signature.BothSignatureWriter;
 import org.jetbrains.kotlin.codegen.signature.JvmSignatureWriter;
@@ -252,6 +253,10 @@ public class KotlinTypeMapper {
            return descriptor.getContainingDeclaration().getName().asString();
        }

+        if (directMember instanceof FictitiousArrayConstructor) {
+            return "kotlin.Array";
+        }
+
        throw new RuntimeException("Could not find package member for " + descriptor +
                                   " in package fragment " + descriptor.getContainingDeclaration());
    }
@@ -0,0 +1,3 @@
+package lib
+
+inline fun Int.toString(ignored: Int) = toString()
@@ -0,0 +1,5 @@
+package lib
+
+inline class J(val value: Int)
+
+inline fun J.toString(ignored: Int) = "J$value"
@@ -0,0 +1,11 @@
+warning: ATTENTION!
+This build uses unsafe internal compiler arguments:
+
+-XXLanguage:+InlineClasses
+
+This mode is not recommended for production use,
+as no stability/compatibility guarantees are given on
+compiler or generated code. Use it at your own risk!
+
+warning: following manually enabled features will force generation of pre-release binaries: InlineClasses
+OK
@@ -0,0 +1,8 @@
+import lib.*
+
+fun run() {
+    val test1 = 42.toString(10)
+    val test2 = J(42).toString(10)
+    if (test1 != "42") throw AssertionError(test1)
+    if (test2 != "J42") throw AssertionError(test2)
+}
@@ -51,9 +51,9 @@ class CompileKotlinAgainstCustomBinariesTest : AbstractKotlinCompilerIntegration

    private fun doTestWithTxt(vararg extraClassPath: File) {
        validateAndCompareDescriptorWithFile(
-                analyzeFileToPackageView(*extraClassPath),
-                AbstractLoadJavaTest.COMPARATOR_CONFIGURATION,
-                getTestDataFileWithExtension("txt")
+            analyzeFileToPackageView(*extraClassPath),
+            AbstractLoadJavaTest.COMPARATOR_CONFIGURATION,
+            getTestDataFileWithExtension("txt")
        )
    }

@@ -74,7 +74,7 @@ class CompileKotlinAgainstCustomBinariesTest : AbstractKotlinCompilerIntegration
    }

    private fun analyzeAndGetAllDescriptors(vararg extraClassPath: File): Collection<DeclarationDescriptor> =
-            DescriptorUtils.getAllDescriptors(analyzeFileToPackageView(*extraClassPath).memberScope)
+        DescriptorUtils.getAllDescriptors(analyzeFileToPackageView(*extraClassPath).memberScope)

    private fun doTestBrokenLibrary(libraryName: String, vararg pathsToDelete: String) {
        // This function compiles a library, then deletes one class file and attempts to compile a Kotlin source against
@@ -105,10 +105,10 @@ class CompileKotlinAgainstCustomBinariesTest : AbstractKotlinCompilerIntegration
    }

    private fun doTestPreReleaseKotlinLibrary(
-            compiler: CLICompiler<*>,
-            libraryName: String,
-            usageDestination: File,
-            vararg additionalOptions: String
+        compiler: CLICompiler<*>,
+        libraryName: String,
+        usageDestination: File,
+        vararg additionalOptions: String
    ) {
        // Compiles the library with the "pre-release" flag, then compiles a usage of this library in the release mode

@@ -126,13 +126,12 @@ class CompileKotlinAgainstCustomBinariesTest : AbstractKotlinCompilerIntegration
    }

    private fun <T> withPreRelease(value: Boolean, block: () -> T): T =
-            try {
-                System.setProperty(TEST_IS_PRE_RELEASE_SYSTEM_PROPERTY, value.toString())
-                block()
-            }
-            finally {
-                System.clearProperty(TEST_IS_PRE_RELEASE_SYSTEM_PROPERTY)
-            }
+        try {
+            System.setProperty(TEST_IS_PRE_RELEASE_SYSTEM_PROPERTY, value.toString())
+            block()
+        } finally {
+            System.clearProperty(TEST_IS_PRE_RELEASE_SYSTEM_PROPERTY)
+        }

    // ------------------------------------------------------------------------------

@@ -175,8 +174,8 @@ class CompileKotlinAgainstCustomBinariesTest : AbstractKotlinCompilerIntegration
        result.throwIfError()

        AnalyzerWithCompilerReport.reportDiagnostics(
-                result.bindingContext.diagnostics,
-                PrintingMessageCollector(System.err, MessageRenderer.PLAIN_FULL_PATHS, false)
+            result.bindingContext.diagnostics,
+            PrintingMessageCollector(System.err, MessageRenderer.PLAIN_FULL_PATHS, false)
        )

        assertEquals("There should be no diagnostics", 0, result.bindingContext.diagnostics.count())
@@ -207,10 +206,14 @@ class CompileKotlinAgainstCustomBinariesTest : AbstractKotlinCompilerIntegration
    }

    fun testMissingDependencyConflictingLibraries() {
-        val library1 = copyJarFileWithoutEntry(compileLibrary("library1"),
-                                               "a/A.class", "a/A\$Inner.class", "a/AA.class", "a/AA\$Inner.class")
-        val library2 = copyJarFileWithoutEntry(compileLibrary("library2"),
-                                               "a/A.class", "a/A\$Inner.class", "a/AA.class", "a/AA\$Inner.class")
+        val library1 = copyJarFileWithoutEntry(
+            compileLibrary("library1"),
+            "a/A.class", "a/A\$Inner.class", "a/AA.class", "a/AA\$Inner.class"
+        )
+        val library2 = copyJarFileWithoutEntry(
+            compileLibrary("library2"),
+            "a/A.class", "a/A\$Inner.class", "a/AA.class", "a/AA\$Inner.class"
+        )
        compileKotlin("source.kt", tmpdir, listOf(library1, library2))
    }

@@ -248,16 +251,20 @@ class CompileKotlinAgainstCustomBinariesTest : AbstractKotlinCompilerIntegration
        withPreRelease(true) {
            val library = compileLibrary("library")
            val someStableReleasedVersion = LanguageVersion.values().first().also { assert(it.isStable) }
-            compileKotlin("source.kt", tmpdir, listOf(library), K2JVMCompiler(),
-                          listOf("-language-version", someStableReleasedVersion.versionString))
+            compileKotlin(
+                "source.kt", tmpdir, listOf(library), K2JVMCompiler(),
+                listOf("-language-version", someStableReleasedVersion.versionString)
+            )
        }
    }

    fun testPreReleaseCompilerAgainstPreReleaseLibraryLatestStable() {
        withPreRelease(true) {
            val library = compileLibrary("library")
-            compileKotlin("source.kt", tmpdir, listOf(library), K2JVMCompiler(),
-                          listOf("-language-version", LanguageVersion.LATEST_STABLE.versionString))
+            compileKotlin(
+                "source.kt", tmpdir, listOf(library), K2JVMCompiler(),
+                listOf("-language-version", LanguageVersion.LATEST_STABLE.versionString)
+            )
        }
    }

@@ -277,8 +284,7 @@ class CompileKotlinAgainstCustomBinariesTest : AbstractKotlinCompilerIntegration
                strings.map { string ->
                    String(string.toByteArray().map { x -> x xor 42 }.toTypedArray().toByteArray())
                }.toTypedArray()
-            }
-            else null
+            } else null
        })
    }

@@ -363,8 +369,7 @@ class CompileKotlinAgainstCustomBinariesTest : AbstractKotlinCompilerIntegration

        if (GENERATE_SMAP) {
            assertEquals(expected, debugInfo)
-        }
-        else {
+        } else {
            assertEquals(null, debugInfo)
        }
    }
@@ -421,28 +426,44 @@ class CompileKotlinAgainstCustomBinariesTest : AbstractKotlinCompilerIntegration
        val version = intArrayOf(1, 0, 1) // legacy coroutines metadata
        val options = listOf("-Xcoroutines=enable")
        val library = transformJar(
-                compileLibrary("library", additionalOptions = options),
-                { _, bytes ->
-                    val (resultBytes, removedCounter) = stripSuspensionMarksToImitateLegacyCompiler(
-                        WrongBytecodeVersionTest.transformMetadataInClassFile(bytes) { name, _ ->
-                            if (name == JvmAnnotationNames.BYTECODE_VERSION_FIELD_NAME) version else null
-                        })
-                    // we expect 4 instructions to be removed in this test library
-                    assertEquals(4, removedCounter)
-                    resultBytes
-                })
-        compileKotlin("source.kt", tmpdir, listOf(library), K2JVMCompiler(),
-                      additionalOptions = options)
-        val classLoader = URLClassLoader(arrayOf(library.toURI().toURL(), tmpdir.toURI().toURL()),
-                                         ForTestCompileRuntime.runtimeJarClassLoader())
+            compileLibrary("library", additionalOptions = options),
+            { _, bytes ->
+                val (resultBytes, removedCounter) = stripSuspensionMarksToImitateLegacyCompiler(
+                    WrongBytecodeVersionTest.transformMetadataInClassFile(bytes) { name, _ ->
+                        if (name == JvmAnnotationNames.BYTECODE_VERSION_FIELD_NAME) version else null
+                    })
+                // we expect 4 instructions to be removed in this test library
+                assertEquals(4, removedCounter)
+                resultBytes
+            })
+        compileKotlin(
+            "source.kt", tmpdir, listOf(library), K2JVMCompiler(),
+            additionalOptions = options
+        )
+        val classLoader = URLClassLoader(
+            arrayOf(library.toURI().toURL(), tmpdir.toURI().toURL()),
+            ForTestCompileRuntime.runtimeJarClassLoader()
+        )
        @Suppress("UNCHECKED_CAST")
        val result = classLoader
-                .loadClass("SourceKt")
-                .getDeclaredMethod("run")
-                .invoke(null) as Array<String>
+            .loadClass("SourceKt")
+            .getDeclaredMethod("run")
+            .invoke(null) as Array<String>
        assertEquals(result[0], result[1])
    }

+    fun testInlineFunctionsWithMatchingJvmSignatures() {
+        val library = compileLibrary(
+            "library",
+            additionalOptions = listOf("-XXLanguage:+InlineClasses"),
+            checkKotlinOutput = { _ -> }
+        )
+        compileKotlin("source.kt", tmpdir, listOf(library), additionalOptions = listOf("-XXLanguage:+InlineClasses"))
+
+        URLClassLoader(arrayOf(library.toURI().toURL(), tmpdir.toURI().toURL()), ForTestCompileRuntime.runtimeJarClassLoader())
+            .loadClass("SourceKt").getDeclaredMethod("run").invoke(null)
+    }
+
    fun testClassFromJdkInLibrary() {
        val library = compileLibrary("library")
        compileKotlin("source.kt", tmpdir, listOf(library))
@@ -503,9 +524,11 @@ class CompileKotlinAgainstCustomBinariesTest : AbstractKotlinCompilerIntegration

    fun testInternalFromFriendModuleCommon() {
        val library = compileCommonLibrary("library")
-        compileKotlin("source.kt", tmpdir, listOf(library), K2MetadataCompiler(), listOf(
-            // TODO: "-Xfriend-paths=${library.path}"
-        ))
+        compileKotlin(
+            "source.kt", tmpdir, listOf(library), K2MetadataCompiler(), listOf(
+                // TODO: "-Xfriend-paths=${library.path}"
+            )
+        )
    }

    companion object {
@@ -514,12 +537,18 @@ class CompileKotlinAgainstCustomBinariesTest : AbstractKotlinCompilerIntegration
            val writer = ClassWriter(0)
            var removedCounter = 0
            ClassReader(bytes).accept(object : ClassVisitor(Opcodes.ASM5, writer) {
-                override fun visitMethod(access: Int, name: String?, desc: String?, signature: String?, exceptions: Array<out String>?): MethodVisitor {
+                override fun visitMethod(
+                    access: Int,
+                    name: String?,
+                    desc: String?,
+                    signature: String?,
+                    exceptions: Array<out String>?
+                ): MethodVisitor {
                    val superMV = super.visitMethod(access, name, desc, signature, exceptions)
                    return object : MethodNode(Opcodes.ASM5, access, name, desc, signature, exceptions) {
                        override fun visitEnd() {
                            val removeList = instructions.asSequence()
-                                    .flatMap { suspendMarkerInsns(it).asSequence() }.toList()
+                                .flatMap { suspendMarkerInsns(it).asSequence() }.toList()
                            remove(removeList)
                            removedCounter += removeList.size
                            accept(superMV)
@@ -537,18 +566,19 @@ class CompileKotlinAgainstCustomBinariesTest : AbstractKotlinCompilerIntegration
                && insn.opcode == Opcodes.INVOKESTATIC
                && insn.owner == "kotlin/jvm/internal/InlineMarker"
                && insn.name == "mark"
-                && insn.previous.intConstant in 0..1) listOf(insn, insn.previous)
+                && insn.previous.intConstant in 0..1
+            ) listOf(insn, insn.previous)
            else emptyList()

        // -----

        private fun copyJarFileWithoutEntry(jarPath: File, vararg entriesToDelete: String): File =
-                transformJar(jarPath, { _, bytes -> bytes }, entriesToDelete.toSet())
+            transformJar(jarPath, { _, bytes -> bytes }, entriesToDelete.toSet())

        private fun transformJar(
-                jarPath: File,
-                transformEntry: (String, ByteArray) -> ByteArray,
-                entriesToDelete: Set<String> = emptySet()
+            jarPath: File,
+            transformEntry: (String, ByteArray) -> ByteArray,
+            entriesToDelete: Set<String> = emptySet()
        ): File {
            val outputFile = File(jarPath.parentFile, "${jarPath.nameWithoutExtension}-after.jar")