From b5f37d18b2ca62a4ea6ef86d89d7300914f2baa5 Mon Sep 17 00:00:00 2001 From: Dmitry Petrov Date: Mon, 20 Aug 2018 11:47:30 +0300 Subject: [PATCH] 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. --- .../kotlin/codegen/inline/InlineCache.kt | 3 +- .../kotlin/codegen/inline/InlineCodegen.kt | 8 +- .../codegen/state/KotlinTypeMapper.java | 5 + .../library/1.kt | 3 + .../library/2.kt | 5 + .../output.txt | 11 ++ .../source.kt | 8 + .../CompileKotlinAgainstCustomBinariesTest.kt | 142 +++++++++++------- 8 files changed, 125 insertions(+), 60 deletions(-) create mode 100644 compiler/testData/compileKotlinAgainstCustomBinaries/inlineFunctionsWithMatchingJvmSignatures/library/1.kt create mode 100644 compiler/testData/compileKotlinAgainstCustomBinaries/inlineFunctionsWithMatchingJvmSignatures/library/2.kt create mode 100644 compiler/testData/compileKotlinAgainstCustomBinaries/inlineFunctionsWithMatchingJvmSignatures/output.txt create mode 100644 compiler/testData/compileKotlinAgainstCustomBinaries/inlineFunctionsWithMatchingJvmSignatures/source.kt diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/InlineCache.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/InlineCache.kt index 48f495800b7..46ffc6430b2 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/InlineCache.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/InlineCache.kt @@ -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 = SLRUMap(30, 20) diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/InlineCodegen.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/InlineCodegen.kt index 32bd705e6f0..7c46a9e00b4 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/InlineCodegen.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/inline/InlineCodegen.kt @@ -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( 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) diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/state/KotlinTypeMapper.java b/compiler/backend/src/org/jetbrains/kotlin/codegen/state/KotlinTypeMapper.java index 8033752d299..1dbecfe92c1 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/state/KotlinTypeMapper.java +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/state/KotlinTypeMapper.java @@ -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()); } diff --git a/compiler/testData/compileKotlinAgainstCustomBinaries/inlineFunctionsWithMatchingJvmSignatures/library/1.kt b/compiler/testData/compileKotlinAgainstCustomBinaries/inlineFunctionsWithMatchingJvmSignatures/library/1.kt new file mode 100644 index 00000000000..e4a480d72fd --- /dev/null +++ b/compiler/testData/compileKotlinAgainstCustomBinaries/inlineFunctionsWithMatchingJvmSignatures/library/1.kt @@ -0,0 +1,3 @@ +package lib + +inline fun Int.toString(ignored: Int) = toString() \ No newline at end of file diff --git a/compiler/testData/compileKotlinAgainstCustomBinaries/inlineFunctionsWithMatchingJvmSignatures/library/2.kt b/compiler/testData/compileKotlinAgainstCustomBinaries/inlineFunctionsWithMatchingJvmSignatures/library/2.kt new file mode 100644 index 00000000000..b9f84280a5d --- /dev/null +++ b/compiler/testData/compileKotlinAgainstCustomBinaries/inlineFunctionsWithMatchingJvmSignatures/library/2.kt @@ -0,0 +1,5 @@ +package lib + +inline class J(val value: Int) + +inline fun J.toString(ignored: Int) = "J$value" \ No newline at end of file diff --git a/compiler/testData/compileKotlinAgainstCustomBinaries/inlineFunctionsWithMatchingJvmSignatures/output.txt b/compiler/testData/compileKotlinAgainstCustomBinaries/inlineFunctionsWithMatchingJvmSignatures/output.txt new file mode 100644 index 00000000000..135b906b2a4 --- /dev/null +++ b/compiler/testData/compileKotlinAgainstCustomBinaries/inlineFunctionsWithMatchingJvmSignatures/output.txt @@ -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 diff --git a/compiler/testData/compileKotlinAgainstCustomBinaries/inlineFunctionsWithMatchingJvmSignatures/source.kt b/compiler/testData/compileKotlinAgainstCustomBinaries/inlineFunctionsWithMatchingJvmSignatures/source.kt new file mode 100644 index 00000000000..5d34707e219 --- /dev/null +++ b/compiler/testData/compileKotlinAgainstCustomBinaries/inlineFunctionsWithMatchingJvmSignatures/source.kt @@ -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) +} \ No newline at end of file diff --git a/compiler/tests/org/jetbrains/kotlin/jvm/compiler/CompileKotlinAgainstCustomBinariesTest.kt b/compiler/tests/org/jetbrains/kotlin/jvm/compiler/CompileKotlinAgainstCustomBinariesTest.kt index e79d5fada9c..366e48594f5 100644 --- a/compiler/tests/org/jetbrains/kotlin/jvm/compiler/CompileKotlinAgainstCustomBinariesTest.kt +++ b/compiler/tests/org/jetbrains/kotlin/jvm/compiler/CompileKotlinAgainstCustomBinariesTest.kt @@ -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 = - 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 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 + .loadClass("SourceKt") + .getDeclaredMethod("run") + .invoke(null) as Array 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?): MethodVisitor { + override fun visitMethod( + access: Int, + name: String?, + desc: String?, + signature: String?, + exceptions: Array? + ): 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 = emptySet() + jarPath: File, + transformEntry: (String, ByteArray) -> ByteArray, + entriesToDelete: Set = emptySet() ): File { val outputFile = File(jarPath.parentFile, "${jarPath.nameWithoutExtension}-after.jar")