Handle assertions where the assertions enabled guard is checked more
than once
The following code pattern is in the Kotlin stdlib
if (_Assertions.ENABLED)
assert(...) { ... }
}
Bug: b/226370094
Change-Id: I681acad967f8bc0ae05abbd55ea4d92989377699diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
index 3dd01d8..fa7a539 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
@@ -1175,7 +1175,7 @@
return timing;
}
- assertionsRewriter.run(method, code, timing);
+ assertionsRewriter.run(method, code, deadCodeRemover, timing);
if (serviceLoaderRewriter != null) {
assert appView.appInfo().hasLiveness();
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/AssertionsRewriter.java b/src/main/java/com/android/tools/r8/ir/optimize/AssertionsRewriter.java
index db7343c..1961b30 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/AssertionsRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/AssertionsRewriter.java
@@ -33,10 +33,13 @@
import com.android.tools.r8.utils.ThrowingCharIterator;
import com.android.tools.r8.utils.Timing;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Sets;
+import it.unimi.dsi.fastutil.objects.Reference2BooleanOpenHashMap;
import java.io.UTFDataFormatException;
import java.util.IdentityHashMap;
import java.util.List;
import java.util.Map;
+import java.util.Set;
import java.util.stream.Collectors;
public class AssertionsRewriter {
@@ -273,6 +276,7 @@
* }
* }
* </pre>
+ *
* For Kotlin the Class instance method desiredAssertionStatus() is only called for the class
* kotlin._Assertions, where kotlin._Assertions.class.desiredAssertionStatus() is read into the
* static field kotlin._Assertions.ENABLED.
@@ -310,7 +314,8 @@
* }
* </pre>
*
- * With the rewriting below and AssertionTransformation.DISABLE (and other rewritings) the resulting code is:
+ * With the rewriting below and AssertionTransformation.DISABLE (and other rewritings) the
+ * resulting code is:
*
* <pre>
* class A {
@@ -331,23 +336,26 @@
* }
* }
* </pre>
-
+ *
* NOTE: that in Kotlin the assertion condition is always calculated. So it is still present in
* the code and even for AssertionTransformation.DISABLE.
*/
- public void run(DexEncodedMethod method, IRCode code, Timing timing) {
+ public void run(
+ DexEncodedMethod method, IRCode code, DeadCodeRemover deadCodeRemover, Timing timing) {
if (enabled) {
timing.begin("Rewrite assertions");
- runInternal(method, code);
+ if (runInternal(method, code)) {
+ deadCodeRemover.run(code, timing);
+ }
assert code.isConsistentSSA(appView);
timing.end();
}
}
- private void runInternal(DexEncodedMethod method, IRCode code) {
+ private boolean runInternal(DexEncodedMethod method, IRCode code) {
ConfigurationEntryWithDexString configuration = getTransformationForMethod(method);
if (configuration.isPassthrough()) {
- return;
+ return false;
}
DexEncodedMethod clinit;
// If the <clinit> of this class did not have have code to turn on assertions don't try to
@@ -357,20 +365,25 @@
} else {
DexClass clazz = appView.definitionFor(method.getHolderType());
if (clazz == null) {
- return;
+ return false;
}
clinit = clazz.getClassInitializer();
}
// For the transformation to rewrite the throw with a callback collect information on the
// blocks covered by the if (!$assertionsDisabled or ENABLED) condition together with weather
// the assertion handling is on the true or false branch.
- Map<If, Boolean> assertionEntryIfs = new IdentityHashMap<>();
+ Map<If, Boolean> assertionEntryIfs = new Reference2BooleanOpenHashMap<>();
Map<Throw, BasicBlock> throwSuccessorAfterHandler = new IdentityHashMap<>();
+ Set<BasicBlock> assertionBlocks = Sets.newIdentityHashSet();
+ Map<If, Boolean> additionalAssertionsEnabledIfs = new Reference2BooleanOpenHashMap<>();
if (configuration.isAssertionHandler()) {
LazyBox<DominatorTree> dominatorTree = new LazyBox<>(() -> new DominatorTree(code));
code.getBlocks()
.forEach(
basicBlock -> {
+ if (assertionBlocks.contains(basicBlock)) {
+ return;
+ }
If theIf = isCheckAssertionsEnabledBlock(basicBlock);
if (theIf != null) {
// All blocks dominated by the if is the assertion code. For Java it is on the
@@ -381,13 +394,30 @@
theIf.lhs().getDefinition().asStaticGet());
BasicBlock assertionBlockEntry =
theIf.targetFromBoolean(conditionForAssertionBlock);
+ List<BasicBlock> dominatedBlocks =
+ dominatorTree.computeIfAbsent().dominatedBlocks(assertionBlockEntry);
Throw throwInstruction =
- dominatedBlocksHasSingleThrow(
- assertionBlockEntry, dominatorTree.computeIfAbsent());
+ dominatedBlocksHasSingleThrow(assertionBlockEntry, dominatedBlocks);
if (throwInstruction != null) {
assertionEntryIfs.put(theIf, conditionForAssertionBlock);
throwSuccessorAfterHandler.put(
throwInstruction, theIf.targetFromBoolean(!conditionForAssertionBlock));
+ // Collect any additional assertions enabled checks dominated by the current
+ // assertions entry check.
+ dominatedBlocks.forEach(
+ block -> {
+ If additionalAssertionsEnabledIf = isCheckAssertionsEnabledBlock(block);
+ if (additionalAssertionsEnabledIf != null) {
+ additionalAssertionsEnabledIfs.put(
+ additionalAssertionsEnabledIf,
+ !isUsingJavaAssertionsDisabledField(
+ additionalAssertionsEnabledIf
+ .lhs()
+ .getDefinition()
+ .asStaticGet()));
+ }
+ });
+ assertionBlocks.addAll(dominatedBlocks);
}
}
});
@@ -399,6 +429,7 @@
clinit != null && clinit.getOptimizationInfo().isInitializerEnablingJavaVmAssertions();
// This code will process the assertion code in all methods including <clinit>.
InstructionListIterator iterator = code.instructionListIterator();
+ boolean needsDeadCodeRemoval = false;
while (iterator.hasNext()) {
Instruction current = iterator.next();
if (current.isInvokeMethod()) {
@@ -444,11 +475,12 @@
if (current.isIf()) {
If ifInstruction = current.asIf();
if (assertionEntryIfs.containsKey(ifInstruction)) {
- ifInstruction
- .targetFromBoolean(!assertionEntryIfs.get(ifInstruction))
- .unlinkSinglePredecessorSiblingsAllowed();
- ifInstruction.lhs().removeUser(ifInstruction);
- iterator.replaceCurrentInstruction(new Goto());
+ forceAssertionsEnabled(ifInstruction, assertionEntryIfs, iterator);
+ needsDeadCodeRemoval = true;
+ }
+ if (additionalAssertionsEnabledIfs.containsKey(ifInstruction)) {
+ forceAssertionsEnabled(ifInstruction, additionalAssertionsEnabledIfs, iterator);
+ needsDeadCodeRemoval = true;
}
} else if (current.isThrow()) {
Throw throwInstruction = current.asThrow();
@@ -467,6 +499,7 @@
}
}
}
+ return needsDeadCodeRemoval;
}
private void rewriteKotlinAssertionEnable(
@@ -539,10 +572,9 @@
: null;
}
- private Throw dominatedBlocksHasSingleThrow(BasicBlock block, DominatorTree dominatorTree) {
+ private Throw dominatedBlocksHasSingleThrow(BasicBlock block, List<BasicBlock> dominatedBlocks) {
Throw theThrow = null;
- List<BasicBlock> blocks = dominatorTree.dominatedBlocks(block);
- for (BasicBlock current : blocks) {
+ for (BasicBlock current : dominatedBlocks) {
if (current.exit().isReturn()) {
return null;
}
@@ -555,4 +587,13 @@
}
return theThrow;
}
+
+ private void forceAssertionsEnabled(
+ If ifInstruction, Map<If, Boolean> targetMap, InstructionListIterator iterator) {
+ ifInstruction
+ .targetFromBoolean(!targetMap.get(ifInstruction))
+ .unlinkSinglePredecessorSiblingsAllowed();
+ ifInstruction.lhs().removeUser(ifInstruction);
+ iterator.replaceCurrentInstruction(new Goto());
+ }
}
diff --git a/src/test/java/com/android/tools/r8/rewrite/assertions/AssertionConfigurationAssertionHandlerKotlinTestBase.java b/src/test/java/com/android/tools/r8/rewrite/assertions/AssertionConfigurationAssertionHandlerKotlinTestBase.java
index 84bd1ef..b70b311 100644
--- a/src/test/java/com/android/tools/r8/rewrite/assertions/AssertionConfigurationAssertionHandlerKotlinTestBase.java
+++ b/src/test/java/com/android/tools/r8/rewrite/assertions/AssertionConfigurationAssertionHandlerKotlinTestBase.java
@@ -4,12 +4,13 @@
package com.android.tools.r8.rewrite.assertions;
-import static org.hamcrest.CoreMatchers.equalTo;
import static org.junit.Assume.assumeTrue;
import com.android.tools.r8.KotlinTestBase;
import com.android.tools.r8.KotlinTestParameters;
import com.android.tools.r8.R8FullTestBuilder;
+import com.android.tools.r8.R8TestCompileResult;
+import com.android.tools.r8.TestBuilder;
import com.android.tools.r8.TestCompilerBuilder;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.references.MethodReference;
@@ -70,10 +71,21 @@
protected abstract List<Path> getKotlinFiles() throws IOException;
+ protected boolean transformKotlinClasses() {
+ return false;
+ }
+
+ protected byte[] transformedKotlinClasses(Path kotlinClasses) throws IOException {
+ assert false;
+ return null;
+ }
+
protected abstract String getTestClassName();
protected void configureR8(R8FullTestBuilder builder) {}
+ protected void configureResultR8(R8TestCompileResult builder) {}
+
private Path kotlinStdlibLibraryForRuntime() throws Exception {
Path kotlinStdlibCf = kotlinc.getKotlinStdlibJar();
if (parameters.getRuntime().isCf()) {
@@ -107,6 +119,16 @@
}
}
+ private void addKotlinClasses(TestBuilder<?, ?> builder) {
+ builder.applyIf(
+ transformKotlinClasses(),
+ b ->
+ b.addProgramClassFileData(
+ transformedKotlinClasses(
+ compiledForAssertions.getForConfiguration(kotlinc, targetVersion))),
+ b -> b.addProgramFiles(compiledForAssertions.getForConfiguration(kotlinc, targetVersion)));
+ }
+
@Test
public void testD8() throws Exception {
assumeTrue(parameters.isDexRuntime());
@@ -114,7 +136,7 @@
.apply(this::configureKotlinStdlib)
.setMinApi(parameters.getApiLevel())
.addProgramClasses(AssertionHandlers.class)
- .addProgramFiles(compiledForAssertions.getForConfiguration(kotlinc, targetVersion))
+ .apply(this::addKotlinClasses)
.addAssertionsConfiguration(
builder ->
builder
@@ -131,7 +153,7 @@
.apply(this::configureKotlinStdlib)
.setMinApi(parameters.getApiLevel())
.addProgramClasses(AssertionHandlers.class)
- .addProgramFiles(compiledForAssertions.getForConfiguration(kotlinc, targetVersion))
+ .apply(this::addKotlinClasses)
.addAssertionsConfiguration(
builder ->
builder
@@ -140,13 +162,8 @@
.build())
.addKeepMainRule(getTestClassName())
.apply(this::configureR8)
- .allowDiagnosticWarningMessages(!kotlinStdlibAsLibrary)
.compile()
- .applyIf(
- !kotlinStdlibAsLibrary,
- result ->
- result.assertAllWarningMessagesMatch(
- equalTo("Resource 'META-INF/MANIFEST.MF' already exists.")))
+ .apply(this::configureResultR8)
.run(parameters.getRuntime(), getTestClassName())
.assertSuccessWithOutput(getExpectedOutput());
}
diff --git a/src/test/java/com/android/tools/r8/rewrite/assertions/kotlinassertionhandlerdoublecheck/AssertionConfigurationAssertionHandlerKotlinDoubleCheckTest.java b/src/test/java/com/android/tools/r8/rewrite/assertions/kotlinassertionhandlerdoublecheck/AssertionConfigurationAssertionHandlerKotlinDoubleCheckTest.java
new file mode 100644
index 0000000..d0177bb
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/rewrite/assertions/kotlinassertionhandlerdoublecheck/AssertionConfigurationAssertionHandlerKotlinDoubleCheckTest.java
@@ -0,0 +1,97 @@
+// Copyright (c) 2022, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+package com.android.tools.r8.rewrite.assertions.kotlinassertionhandlerdoublecheck;
+
+import com.android.tools.r8.KotlinCompilerTool.KotlinCompilerVersion;
+import com.android.tools.r8.KotlinTestParameters;
+import com.android.tools.r8.R8FullTestBuilder;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.references.MethodReference;
+import com.android.tools.r8.references.Reference;
+import com.android.tools.r8.rewrite.assertions.AssertionConfigurationAssertionHandlerKotlinTestBase;
+import com.android.tools.r8.rewrite.assertions.assertionhandler.AssertionHandlers;
+import com.android.tools.r8.utils.DescriptorUtils;
+import com.android.tools.r8.utils.FileUtils;
+import com.android.tools.r8.utils.StringUtils;
+import com.android.tools.r8.utils.ZipUtils;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.List;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class AssertionConfigurationAssertionHandlerKotlinDoubleCheckTest
+ extends AssertionConfigurationAssertionHandlerKotlinTestBase {
+
+ public AssertionConfigurationAssertionHandlerKotlinDoubleCheckTest(
+ TestParameters parameters,
+ KotlinTestParameters kotlinParameters,
+ boolean kotlinStdlibAsClasspath,
+ boolean useJvmAssertions)
+ throws IOException {
+ super(parameters, kotlinParameters, kotlinStdlibAsClasspath, useJvmAssertions);
+ }
+
+ @Override
+ protected String getExpectedOutput() {
+ return StringUtils.lines("assertionHandler: doubleCheckAssertion");
+ }
+
+ @Override
+ protected MethodReference getAssertionHandler() throws Exception {
+ return Reference.methodFromMethod(
+ AssertionHandlers.class.getMethod("assertionHandler", Throwable.class));
+ }
+
+ @Override
+ protected List<Path> getKotlinFiles() throws IOException {
+ return getKotlinFilesInTestPackage(getClass().getPackage());
+ }
+
+ @Override
+ protected boolean transformKotlinClasses() {
+ return true;
+ }
+
+ @Override
+ protected byte[] transformedKotlinClasses(Path kotlinClasses) throws IOException {
+ Path compiledKotlinClasses = temp.newFolder().toPath();
+ ZipUtils.unzip(
+ compiledForAssertions.getForConfiguration(kotlinc, targetVersion), compiledKotlinClasses);
+ String testClassPath =
+ DescriptorUtils.getPackageBinaryNameFromJavaType(getTestClassName())
+ + FileUtils.CLASS_EXTENSION;
+ String assertionsMockBinaryName =
+ DescriptorUtils.getPackageBinaryNameFromJavaType(
+ getClass().getPackage().getName() + ".AssertionsMock");
+ // Rewrite the static get on AssertionsMock.Enabled to static get on kotlin._Assertions.ENABLED
+ return transformer(
+ compiledKotlinClasses.resolve(testClassPath),
+ Reference.classFromTypeName(getTestClassName()))
+ .transformFieldInsnInMethod(
+ "doubleCheckAssertionsEnabled",
+ (opcode, owner, name, descriptor, continuation) -> {
+ continuation.visitFieldInsn(
+ opcode,
+ owner.equals(assertionsMockBinaryName) ? "kotlin/_Assertions" : owner,
+ name,
+ descriptor);
+ })
+ .transform();
+ }
+
+ @Override
+ protected String getTestClassName() {
+ return getClass().getPackage().getName() + ".AssertionDoubleCheckKt";
+ }
+
+ @Override
+ protected void configureR8(R8FullTestBuilder builder) {
+ boolean referencesNotNull =
+ !kotlinParameters.is(KotlinCompilerVersion.KOTLINC_1_3_72) && !kotlinStdlibAsLibrary;
+ builder.applyIf(referencesNotNull, b -> b.addDontWarn("org.jetbrains.annotations.NotNull"));
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/rewrite/assertions/kotlinassertionhandlerdoublecheck/AssertionDoubleCheck.kt b/src/test/java/com/android/tools/r8/rewrite/assertions/kotlinassertionhandlerdoublecheck/AssertionDoubleCheck.kt
new file mode 100644
index 0000000..3351bcd
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/rewrite/assertions/kotlinassertionhandlerdoublecheck/AssertionDoubleCheck.kt
@@ -0,0 +1,28 @@
+// Copyright (c) 2022, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+package com.android.tools.r8.rewrite.assertions.kotlinassertionhandlerdoublecheck
+
+fun doubleCheckAssertionsEnabled() {
+ // AssertionsMock.ENABLED will be rewritten to kotlin._Assertions.ENABLED to
+ // test
+ // * two checks on kotlin._Assertions.ENABLED and
+ // * check on both kotlin._Assertions.ENABLED and $assertionsDisabled
+ // before entering the assertion code.
+ //
+ // This is testing code like this found in kotlin-stdlib:
+ //
+ // if (_Assertions.ENABLED)
+ // assert(...) { ... }
+ // }
+ //
+ // E.g. in kotlin/io/files/FileTreeWalk.kt.
+ if (AssertionsMock.ENABLED) {
+ assert(false) { "doubleCheckAssertion" }
+ }
+}
+
+fun main() {
+ doubleCheckAssertionsEnabled();
+}
\ No newline at end of file
diff --git a/src/test/java/com/android/tools/r8/rewrite/assertions/kotlinassertionhandlerdoublecheck/AssertionsMock.kt b/src/test/java/com/android/tools/r8/rewrite/assertions/kotlinassertionhandlerdoublecheck/AssertionsMock.kt
new file mode 100644
index 0000000..bd05154
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/rewrite/assertions/kotlinassertionhandlerdoublecheck/AssertionsMock.kt
@@ -0,0 +1,10 @@
+// Copyright (c) 2022, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+package com.android.tools.r8.rewrite.assertions.kotlinassertionhandlerdoublecheck
+
+object AssertionsMock {
+ @JvmField
+ val ENABLED: Boolean = false
+}
diff --git a/src/test/java/com/android/tools/r8/rewrite/assertions/kotlinassertionhandlersimple/AssertionConfigurationAssertionHandlerKotlinSimpleTest.java b/src/test/java/com/android/tools/r8/rewrite/assertions/kotlinassertionhandlersimple/AssertionConfigurationAssertionHandlerKotlinSimpleTest.java
index e460253..aad753e 100644
--- a/src/test/java/com/android/tools/r8/rewrite/assertions/kotlinassertionhandlersimple/AssertionConfigurationAssertionHandlerKotlinSimpleTest.java
+++ b/src/test/java/com/android/tools/r8/rewrite/assertions/kotlinassertionhandlersimple/AssertionConfigurationAssertionHandlerKotlinSimpleTest.java
@@ -4,9 +4,12 @@
package com.android.tools.r8.rewrite.assertions.kotlinassertionhandlersimple;
+import static org.hamcrest.CoreMatchers.equalTo;
+
import com.android.tools.r8.KotlinCompilerTool.KotlinCompilerVersion;
import com.android.tools.r8.KotlinTestParameters;
import com.android.tools.r8.R8FullTestBuilder;
+import com.android.tools.r8.R8TestCompileResult;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.references.MethodReference;
import com.android.tools.r8.references.Reference;
@@ -62,6 +65,17 @@
!kotlinParameters.is(KotlinCompilerVersion.KOTLINC_1_3_72)
&& !kotlinStdlibAsLibrary
&& !useJvmAssertions;
- builder.applyIf(referencesNotNull, b -> b.addDontWarn("org.jetbrains.annotations.NotNull"));
+ builder
+ .applyIf(referencesNotNull, b -> b.addDontWarn("org.jetbrains.annotations.NotNull"))
+ .allowDiagnosticWarningMessages(!kotlinStdlibAsLibrary);
+ }
+
+ @Override
+ protected void configureResultR8(R8TestCompileResult builder) {
+ builder.applyIf(
+ !kotlinStdlibAsLibrary,
+ result ->
+ result.assertAllWarningMessagesMatch(
+ equalTo("Resource 'META-INF/MANIFEST.MF' already exists.")));
}
}
diff --git a/src/test/java/com/android/tools/r8/rewrite/assertions/kotlinassertionhandlerwithexceptions/AssertionConfigurationAssertionHandlerKotlinRethrowingTest.java b/src/test/java/com/android/tools/r8/rewrite/assertions/kotlinassertionhandlerwithexceptions/AssertionConfigurationAssertionHandlerKotlinRethrowingTest.java
index 93b3b57..c2848c3 100644
--- a/src/test/java/com/android/tools/r8/rewrite/assertions/kotlinassertionhandlerwithexceptions/AssertionConfigurationAssertionHandlerKotlinRethrowingTest.java
+++ b/src/test/java/com/android/tools/r8/rewrite/assertions/kotlinassertionhandlerwithexceptions/AssertionConfigurationAssertionHandlerKotlinRethrowingTest.java
@@ -4,8 +4,11 @@
package com.android.tools.r8.rewrite.assertions.kotlinassertionhandlerwithexceptions;
+import static org.hamcrest.CoreMatchers.equalTo;
+
import com.android.tools.r8.KotlinTestParameters;
import com.android.tools.r8.R8FullTestBuilder;
+import com.android.tools.r8.R8TestCompileResult;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.references.MethodReference;
import com.android.tools.r8.references.Reference;
@@ -66,6 +69,16 @@
builder
.addKeepRules("-keep class **.*Kt { assertionsWith*(); simpleAssertion(); }")
.addDontWarn("org.jetbrains.annotations.NotNull")
- .applyIf(!kotlinStdlibAsLibrary, b -> b.addDontWarn("org.jetbrains.annotations.Nullable"));
+ .applyIf(!kotlinStdlibAsLibrary, b -> b.addDontWarn("org.jetbrains.annotations.Nullable"))
+ .allowDiagnosticWarningMessages(!kotlinStdlibAsLibrary);
+ }
+
+ @Override
+ protected void configureResultR8(R8TestCompileResult builder) {
+ builder.applyIf(
+ !kotlinStdlibAsLibrary,
+ result ->
+ result.assertAllWarningMessagesMatch(
+ equalTo("Resource 'META-INF/MANIFEST.MF' already exists.")));
}
}
