From 3a197e8d7e8e88de4f89dc17a290777c4c20abfc Mon Sep 17 00:00:00 2001 From: Denis Zharkov Date: Mon, 24 Oct 2016 16:13:46 +0300 Subject: [PATCH] Fix NPE caused by nullability checks optimizations Mostly this commit replaces instances of original BasicValue from ASM with ones of StrictBasicValue having strict equals implementation Optimization issue was related to non-symmetric 'equals': - NotNullBasicValue("java/lang/Object").equals(BasicValue("java/lang/Object")) == false - BasicValue("java/lang/Object").equals(NotNullBasicValue("java/lang/Object")) == true #KT-14242 Fixed --- .../optimization/boxing/BoxedBasicValue.kt | 7 +-- .../optimization/boxing/BoxingInterpreter.kt | 7 ++- .../boxing/NullabilityInterpreter.kt | 8 +-- .../boxing/ProgressionIteratorBasicValue.java | 4 +- .../RedundantBoxingMethodTransformer.java | 3 +- .../common/OptimizationBasicInterpreter.java | 33 +++++----- .../optimization/common/StrictBasicValue.kt | 63 +++++++++++++++++++ .../codegen/optimization/common/Util.kt | 9 --- .../codegen/box/regressions/hashCodeNPE.kt | 13 ++++ .../ir/IrBlackBoxCodegenTestGenerated.java | 6 ++ .../codegen/BlackBoxCodegenTestGenerated.java | 6 ++ 11 files changed, 122 insertions(+), 37 deletions(-) create mode 100644 compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/StrictBasicValue.kt create mode 100644 compiler/testData/codegen/box/regressions/hashCodeNPE.kt diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/BoxedBasicValue.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/BoxedBasicValue.kt index e47aa06c5c8..e3126c94ee8 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/BoxedBasicValue.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/BoxedBasicValue.kt @@ -18,19 +18,18 @@ package org.jetbrains.kotlin.codegen.optimization.boxing import com.intellij.openapi.util.Pair import org.jetbrains.kotlin.codegen.AsmUtil +import org.jetbrains.kotlin.codegen.optimization.common.StrictBasicValue import org.jetbrains.kotlin.resolve.jvm.AsmTypes import org.jetbrains.org.objectweb.asm.Type import org.jetbrains.org.objectweb.asm.tree.AbstractInsnNode import org.jetbrains.org.objectweb.asm.tree.analysis.BasicValue - -import java.util.ArrayList -import java.util.HashSet +import java.util.* class BoxedBasicValue( boxedType: Type, val boxingInsn: AbstractInsnNode, val progressionIterator: ProgressionIteratorBasicValue? -) : BasicValue(boxedType) { +) : StrictBasicValue(boxedType) { private val associatedInsns = HashSet() private val unboxingWithCastInsns = HashSet>() private val associatedVariables = HashSet() diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/BoxingInterpreter.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/BoxingInterpreter.kt index 6a399df8665..34724925786 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/BoxingInterpreter.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/BoxingInterpreter.kt @@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableSet import org.jetbrains.kotlin.codegen.AsmUtil import org.jetbrains.kotlin.codegen.RangeCodegenUtil import org.jetbrains.kotlin.codegen.optimization.common.OptimizationBasicInterpreter +import org.jetbrains.kotlin.codegen.optimization.common.StrictBasicValue import org.jetbrains.kotlin.name.FqName import org.jetbrains.kotlin.resolve.jvm.AsmTypes import org.jetbrains.kotlin.resolve.jvm.JvmPrimitiveType @@ -93,8 +94,8 @@ open class BoxingInterpreter(private val insnList: InsnList) : OptimizationBasic override fun merge(v: BasicValue, w: BasicValue) = when { - v == BasicValue.UNINITIALIZED_VALUE || w == BasicValue.UNINITIALIZED_VALUE -> { - BasicValue.UNINITIALIZED_VALUE + v == StrictBasicValue.UNINITIALIZED_VALUE || w == StrictBasicValue.UNINITIALIZED_VALUE -> { + StrictBasicValue.UNINITIALIZED_VALUE } v is BoxedBasicValue && v.typeEquals(w) -> { onMergeSuccess(v, w as BoxedBasicValue) @@ -199,4 +200,4 @@ private fun getValuesTypeOfProgressionClass(progressionClassInternalName: String RangeCodegenUtil.getPrimitiveRangeOrProgressionElementType(buildFqNameByInternal(progressionClassInternalName))?.let { type -> type.typeName.asString() - } ?: error("type should be not null") \ No newline at end of file + } ?: error("type should be not null") diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/NullabilityInterpreter.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/NullabilityInterpreter.kt index 194facb2efd..e596d7e1909 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/NullabilityInterpreter.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/NullabilityInterpreter.kt @@ -16,7 +16,7 @@ package org.jetbrains.kotlin.codegen.optimization.boxing -import org.jetbrains.kotlin.codegen.optimization.common.BasicValueWrapper +import org.jetbrains.kotlin.codegen.optimization.common.StrictBasicValue import org.jetbrains.org.objectweb.asm.Opcodes import org.jetbrains.org.objectweb.asm.Type import org.jetbrains.org.objectweb.asm.tree.AbstractInsnNode @@ -31,21 +31,21 @@ class NullabilityInterpreter(insns: InsnList) : BoxingInterpreter(insns) { override fun isExactValue(value: BasicValue) = super.isExactValue(value) || value is NotNullBasicValue override fun createNewBoxing(insn: AbstractInsnNode, type: Type, progressionIterator: ProgressionIteratorBasicValue?) = - NotNullBasicValue(BasicValue(type)) + NotNullBasicValue(type) } private fun makeNotNullIfNeeded(insn: AbstractInsnNode, value: BasicValue?): BasicValue? = when (insn.opcode) { Opcodes.ANEWARRAY, Opcodes.NEWARRAY, Opcodes.LDC, Opcodes.NEW -> if (value?.type?.sort == Type.OBJECT || value?.type?.sort == Type.ARRAY) - NotNullBasicValue(value) + NotNullBasicValue(value.type) else value else -> value } -class NotNullBasicValue(wrappedValue: BasicValue?) : BasicValueWrapper(wrappedValue) { +class NotNullBasicValue(type: Type?) : StrictBasicValue(type) { override fun equals(other: Any?): Boolean = other is NotNullBasicValue // We do not differ not-nullable values, so we should always return the same hashCode // Actually it doesn't really matter because analyzer is not supposed to store values in hashtables diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/ProgressionIteratorBasicValue.java b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/ProgressionIteratorBasicValue.java index 3f6dc1b5c48..68d233d96ae 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/ProgressionIteratorBasicValue.java +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/ProgressionIteratorBasicValue.java @@ -21,12 +21,12 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.kotlin.builtins.PrimitiveType; import org.jetbrains.kotlin.codegen.RangeCodegenUtil; import org.jetbrains.kotlin.codegen.intrinsics.IteratorNext; +import org.jetbrains.kotlin.codegen.optimization.common.StrictBasicValue; import org.jetbrains.kotlin.name.Name; import org.jetbrains.kotlin.resolve.jvm.JvmPrimitiveType; import org.jetbrains.org.objectweb.asm.Type; -import org.jetbrains.org.objectweb.asm.tree.analysis.BasicValue; -public class ProgressionIteratorBasicValue extends BasicValue { +public class ProgressionIteratorBasicValue extends StrictBasicValue { private final static ImmutableMap VALUES_TYPENAME_TO_TYPE; static { diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/RedundantBoxingMethodTransformer.java b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/RedundantBoxingMethodTransformer.java index b4083e61589..6eb2023ea4b 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/RedundantBoxingMethodTransformer.java +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/RedundantBoxingMethodTransformer.java @@ -22,6 +22,7 @@ import com.intellij.openapi.util.Pair; import kotlin.collections.CollectionsKt; import kotlin.jvm.functions.Function1; import org.jetbrains.annotations.NotNull; +import org.jetbrains.kotlin.codegen.optimization.common.StrictBasicValue; import org.jetbrains.kotlin.codegen.optimization.transformer.MethodTransformer; import org.jetbrains.org.objectweb.asm.Opcodes; import org.jetbrains.org.objectweb.asm.Type; @@ -114,7 +115,7 @@ public class RedundantBoxingMethodTransformer extends MethodTransformer { if (CollectionsKt.any(usedValues, new Function1() { @Override public Boolean invoke(BasicValue input) { - if (input == BasicValue.UNINITIALIZED_VALUE) return false; + if (input == StrictBasicValue.UNINITIALIZED_VALUE) return false; return input == null || !(input instanceof BoxedBasicValue) || !((BoxedBasicValue) input).isSafeToRemove() || diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/OptimizationBasicInterpreter.java b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/OptimizationBasicInterpreter.java index 875a7dfd6f4..0b6f77703d6 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/OptimizationBasicInterpreter.java +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/OptimizationBasicInterpreter.java @@ -25,22 +25,27 @@ import org.jetbrains.org.objectweb.asm.tree.analysis.AnalyzerException; import org.jetbrains.org.objectweb.asm.tree.analysis.BasicInterpreter; import org.jetbrains.org.objectweb.asm.tree.analysis.BasicValue; -public class OptimizationBasicInterpreter extends BasicInterpreter { - private static final BasicValue BOOLEAN_VALUE = new BasicValue(Type.BOOLEAN_TYPE); - private static final BasicValue CHAR_VALUE = new BasicValue(Type.CHAR_TYPE); - private static final BasicValue BYTE_VALUE = new BasicValue(Type.BYTE_TYPE); - private static final BasicValue SHORT_VALUE = new BasicValue(Type.SHORT_TYPE); +import static org.jetbrains.kotlin.codegen.optimization.common.StrictBasicValue.*; +public class OptimizationBasicInterpreter extends BasicInterpreter { @Override @Nullable - public BasicValue newValue(@Nullable Type type) { + public StrictBasicValue newValue(@Nullable Type type) { if (type == null) { - return super.newValue(null); + return UNINITIALIZED_VALUE; } switch (type.getSort()) { case Type.VOID: return null; + case Type.INT: + return INT_VALUE; + case Type.FLOAT: + return FLOAT_VALUE; + case Type.LONG: + return LONG_VALUE; + case Type.DOUBLE: + return DOUBLE_VALUE; case Type.BOOLEAN: return BOOLEAN_VALUE; case Type.CHAR: @@ -51,9 +56,9 @@ public class OptimizationBasicInterpreter extends BasicInterpreter { return SHORT_VALUE; case Type.OBJECT: case Type.ARRAY: - return new BasicValue(type); + return new StrictBasicValue(type); default: - return super.newValue(type); + throw new IllegalArgumentException("Unknown type sort " + type.getSort()); } } @@ -88,23 +93,23 @@ public class OptimizationBasicInterpreter extends BasicInterpreter { ) { if (v.equals(w)) return v; - if (v == BasicValue.UNINITIALIZED_VALUE || w == BasicValue.UNINITIALIZED_VALUE) { - return BasicValue.UNINITIALIZED_VALUE; + if (v == StrictBasicValue.UNINITIALIZED_VALUE || w == StrictBasicValue.UNINITIALIZED_VALUE) { + return StrictBasicValue.UNINITIALIZED_VALUE; } // if merge of two references then `lub` is java/lang/Object // arrays also are BasicValues with reference type's if (isReference(v) && isReference(w)) { - return BasicValue.REFERENCE_VALUE; + return StrictBasicValue.REFERENCE_VALUE; } // if merge of something can be stored in int var (int, char, boolean, byte, character) if (v.getType().getOpcode(Opcodes.ISTORE) == Opcodes.ISTORE && w.getType().getOpcode(Opcodes.ISTORE) == Opcodes.ISTORE) { - return BasicValue.INT_VALUE; + return StrictBasicValue.INT_VALUE; } - return BasicValue.UNINITIALIZED_VALUE; + return StrictBasicValue.UNINITIALIZED_VALUE; } private static boolean isReference(@NotNull BasicValue v) { diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/StrictBasicValue.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/StrictBasicValue.kt new file mode 100644 index 00000000000..0038fb65ad8 --- /dev/null +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/StrictBasicValue.kt @@ -0,0 +1,63 @@ +/* + * Copyright 2010-2016 JetBrains s.r.o. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.jetbrains.kotlin.codegen.optimization.common + +import org.jetbrains.org.objectweb.asm.Type +import org.jetbrains.org.objectweb.asm.tree.analysis.BasicValue + +/** + * This class has strict `equals` implementation, that does not accept its subclasses, + * unlike BasicValue that requires `other` instance only to be a subtype of BasicValue and must have the same `type` + */ +open class StrictBasicValue(type: Type?) : BasicValue(type) { + companion object { + @JvmField + val UNINITIALIZED_VALUE = StrictBasicValue(null) + @JvmField + val INT_VALUE = StrictBasicValue(Type.INT_TYPE) + @JvmField + val FLOAT_VALUE = StrictBasicValue(Type.FLOAT_TYPE) + @JvmField + val LONG_VALUE = StrictBasicValue(Type.LONG_TYPE) + @JvmField + val DOUBLE_VALUE = StrictBasicValue(Type.DOUBLE_TYPE) + @JvmField + val BOOLEAN_VALUE = StrictBasicValue(Type.BOOLEAN_TYPE) + @JvmField + val CHAR_VALUE = StrictBasicValue(Type.CHAR_TYPE) + @JvmField + val BYTE_VALUE = StrictBasicValue(Type.BYTE_TYPE) + @JvmField + val SHORT_VALUE = StrictBasicValue(Type.SHORT_TYPE) + @JvmField + val REFERENCE_VALUE = StrictBasicValue(Type.getObjectType("java/lang/Object")) + } + + override fun equals(other: Any?): Boolean { + if (this === other) return true + if (other?.javaClass != javaClass) return false + if (!super.equals(other)) return false + + other as StrictBasicValue + + if (type != other.type) return false + + return true + } + + override fun hashCode() = (type?.hashCode() ?: 0) +} diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/Util.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/Util.kt index 50e0765796c..c8c57ed2d0d 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/Util.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/common/Util.kt @@ -19,7 +19,6 @@ package org.jetbrains.kotlin.codegen.optimization.common import org.jetbrains.org.objectweb.asm.Opcodes import org.jetbrains.org.objectweb.asm.Opcodes.* import org.jetbrains.org.objectweb.asm.tree.* -import org.jetbrains.org.objectweb.asm.tree.analysis.BasicValue val AbstractInsnNode.isMeaningful: Boolean get() = when (this.type) { @@ -77,14 +76,6 @@ fun MethodNode.removeEmptyCatchBlocks() { } } -abstract class BasicValueWrapper(val wrappedValue: BasicValue?) : BasicValue(wrappedValue?.type) { - val basicValue: BasicValue? get() = (wrappedValue as? BasicValueWrapper)?.basicValue ?: wrappedValue - - override fun equals(other: Any?): Boolean { - return super.equals(other) && this.javaClass == other?.javaClass - } -} - inline fun AbstractInsnNode.findNextOrNull(predicate: (AbstractInsnNode) -> Boolean): AbstractInsnNode? { var finger = this.next while (finger != null && !predicate(finger)) { diff --git a/compiler/testData/codegen/box/regressions/hashCodeNPE.kt b/compiler/testData/codegen/box/regressions/hashCodeNPE.kt new file mode 100644 index 00000000000..d362db21e77 --- /dev/null +++ b/compiler/testData/codegen/box/regressions/hashCodeNPE.kt @@ -0,0 +1,13 @@ +// See KT-14242 +var x = 1 +fun box(): String { + val any: Any? = when (1) { + x -> null + else -> Any() + } + + // Must not be NPE here + val hashCode = any?.hashCode() + + return hashCode?.toString() ?: "OK" +} diff --git a/compiler/tests-ir-jvm/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java b/compiler/tests-ir-jvm/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java index a682babdfda..f90a01c9ca3 100644 --- a/compiler/tests-ir-jvm/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java +++ b/compiler/tests-ir-jvm/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java @@ -13846,6 +13846,12 @@ public class IrBlackBoxCodegenTestGenerated extends AbstractIrBlackBoxCodegenTes doTest(fileName); } + @TestMetadata("hashCodeNPE.kt") + public void testHashCodeNPE() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/regressions/hashCodeNPE.kt"); + doTest(fileName); + } + @TestMetadata("internalTopLevelOtherPackage.kt") public void testInternalTopLevelOtherPackage() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/regressions/internalTopLevelOtherPackage.kt"); diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java index eea9876b504..e7a70fc5956 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java @@ -13846,6 +13846,12 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest { doTest(fileName); } + @TestMetadata("hashCodeNPE.kt") + public void testHashCodeNPE() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/regressions/hashCodeNPE.kt"); + doTest(fileName); + } + @TestMetadata("internalTopLevelOtherPackage.kt") public void testInternalTopLevelOtherPackage() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/regressions/internalTopLevelOtherPackage.kt");