Ensure member arrays are accessed synchronized during write.
Now that we share classes between two files, we have to make sure
that we do not access the member arrays while updating them. The
referenced bug was caused (I believe) by us using the array to
collect items in one thread while another thread was sorting them.
This CL changes synchronization to be per member array instead of
per dex class. This is mostly to make it more visible what we are
synchronizing on.
Bug: 67258314
Change-Id: I28a9bb68335673377032e0470ca5aef4c96e87ca
diff --git a/src/main/java/com/android/tools/r8/graph/DexClass.java b/src/main/java/com/android/tools/r8/graph/DexClass.java
index 8f3a923..5050711 100644
--- a/src/main/java/com/android/tools/r8/graph/DexClass.java
+++ b/src/main/java/com/android/tools/r8/graph/DexClass.java
@@ -23,9 +23,22 @@
public DexType superType;
public DexTypeList interfaces;
public DexString sourceFile;
+
+ /**
+ * Access has to be synchronized during concurrent collection/writing phase.
+ */
protected DexEncodedField[] staticFields;
+ /**
+ * Access has to be synchronized during concurrent collection/writing phase.
+ */
protected DexEncodedField[] instanceFields;
+ /**
+ * Access has to be synchronized during concurrent collection/writing phase.
+ */
protected DexEncodedMethod[] directMethods;
+ /**
+ * Access has to be synchronized during concurrent collection/writing phase.
+ */
protected DexEncodedMethod[] virtualMethods;
public DexAnnotationSet annotations;
diff --git a/src/main/java/com/android/tools/r8/graph/DexProgramClass.java b/src/main/java/com/android/tools/r8/graph/DexProgramClass.java
index 3c9c796..fcb4684 100644
--- a/src/main/java/com/android/tools/r8/graph/DexProgramClass.java
+++ b/src/main/java/com/android/tools/r8/graph/DexProgramClass.java
@@ -97,10 +97,17 @@
if (interfaces != null) {
interfaces.collectIndexedItems(indexedItems);
}
- collectAll(indexedItems, staticFields);
- collectAll(indexedItems, instanceFields);
- collectAll(indexedItems, directMethods);
- collectAll(indexedItems, virtualMethods);
+ synchronizedCollectAll(indexedItems, staticFields);
+ synchronizedCollectAll(indexedItems, instanceFields);
+ synchronizedCollectAll(indexedItems, directMethods);
+ synchronizedCollectAll(indexedItems, virtualMethods);
+ }
+ }
+
+ private static <T extends DexItem> void synchronizedCollectAll(IndexedItemCollection collection,
+ T[] items) {
+ synchronized (items) {
+ collectAll(collection, items);
}
}
@@ -120,10 +127,10 @@
// We only have a class data item if there are methods or fields.
if (hasMethodsOrFields()) {
collector.add(this);
- collectAll(collector, directMethods);
- collectAll(collector, virtualMethods);
- collectAll(collector, staticFields);
- collectAll(collector, instanceFields);
+ synchronizedCollectAll(collector, directMethods);
+ synchronizedCollectAll(collector, virtualMethods);
+ synchronizedCollectAll(collector, staticFields);
+ synchronizedCollectAll(collector, instanceFields);
}
if (annotations != null) {
annotations.collectMixedSectionItems(collector);
@@ -134,6 +141,13 @@
annotations.collectMixedSectionItems(collector);
}
+ private static <T extends DexItem> void synchronizedCollectAll(MixedSectionCollection collection,
+ T[] items) {
+ synchronized (items) {
+ collectAll(collection, items);
+ }
+ }
+
@Override
public String toString() {
return type.toString();
@@ -176,11 +190,15 @@
}
private boolean hasAnnotations(DexEncodedField[] fields) {
- return fields != null && Arrays.stream(fields).anyMatch(DexEncodedField::hasAnnotation);
+ synchronized (fields) {
+ return Arrays.stream(fields).anyMatch(DexEncodedField::hasAnnotation);
+ }
}
private boolean hasAnnotations(DexEncodedMethod[] methods) {
- return methods != null && Arrays.stream(methods).anyMatch(DexEncodedMethod::hasAnnotation);
+ synchronized (methods) {
+ return Arrays.stream(methods).anyMatch(DexEncodedMethod::hasAnnotation);
+ }
}
private static Collection<DexProgramClass> accumulateSynthesizedFrom(
@@ -208,30 +226,22 @@
assert !virtualMethod.accessFlags.isStatic();
assert !virtualMethod.accessFlags.isPrivate();
assert !virtualMethod.accessFlags.isConstructor();
- virtualMethods = Arrays.copyOf(virtualMethods, virtualMethods.length + 1);
- virtualMethods[virtualMethods.length - 1] = virtualMethod;
+ synchronized (virtualMethods) {
+ virtualMethods = Arrays.copyOf(virtualMethods, virtualMethods.length + 1);
+ virtualMethods[virtualMethods.length - 1] = virtualMethod;
+ }
}
public void addStaticMethod(DexEncodedMethod staticMethod) {
assert staticMethod.accessFlags.isStatic();
assert !staticMethod.accessFlags.isPrivate();
- directMethods = Arrays.copyOf(directMethods, directMethods.length + 1);
- directMethods[directMethods.length - 1] = staticMethod;
- }
-
- public void removeStaticMethod(DexEncodedMethod staticMethod) {
- assert staticMethod.accessFlags.isStatic();
- DexEncodedMethod[] newDirectMethods = new DexEncodedMethod[directMethods.length - 1];
- int toIndex = 0;
- for (int fromIndex = 0; fromIndex < directMethods.length; fromIndex++) {
- if (directMethods[fromIndex] != staticMethod) {
- newDirectMethods[toIndex++] = directMethods[fromIndex];
- }
+ synchronized (directMethods) {
+ directMethods = Arrays.copyOf(directMethods, directMethods.length + 1);
+ directMethods[directMethods.length - 1] = staticMethod;
}
- directMethods = newDirectMethods;
}
- public synchronized void sortMembers() {
+ public void sortMembers() {
sortEncodedFields(staticFields);
sortEncodedFields(instanceFields);
sortEncodedMethods(directMethods);
@@ -239,11 +249,15 @@
}
private void sortEncodedFields(DexEncodedField[] fields) {
- Arrays.sort(fields, Comparator.comparing(a -> a.field));
+ synchronized (fields) {
+ Arrays.sort(fields, Comparator.comparing(a -> a.field));
+ }
}
private void sortEncodedMethods(DexEncodedMethod[] methods) {
- Arrays.sort(methods, Comparator.comparing(a -> a.method));
+ synchronized (methods) {
+ Arrays.sort(methods, Comparator.comparing(a -> a.method));
+ }
}
@Override
