Retry: Make ConfiguredJarList immutable

By making the Append and RemoveList methods return a new list instead
of modifying the existing list it makes the ConfiguredJarList usages
easier to reason about and safer to use, especially considering that
they are primarily used in global configuration.

Added some tests for Append/RemoveList to ensure that they work and
do not modify the original or result in newly created lists sharing
storage with the original which would lead to corruption.

Bug: 171756871
Bug: 171479578
Test: m nothing
      EMMA_INSTRUMENT=true EMMA_INSTRUMENT_FRAMEWORK=true m nothing
Change-Id: I541c4686ecdd45c6a0c8b1c93fedf0fcd5952e2b

android/config.go

@@ -1361,14 +1361,31 @@ func (l *ConfiguredJarList) IndexOfJar(jar string) int {
 	return IndexList(jar, l.jars)
 }
 
+func copyAndAppend(list []string, item string) []string {
+	// Create the result list to be 1 longer than the input.
+	result := make([]string, len(list)+1)
+
+	// Copy the whole input list into the result.
+	count := copy(result, list)
+
+	// Insert the extra item at the end.
+	result[count] = item
+
+	return result
+}
+
 // Append an (apex, jar) pair to the list.
-func (l *ConfiguredJarList) Append(apex string, jar string) {
-	l.apexes = append(l.apexes, apex)
-	l.jars = append(l.jars, jar)
+func (l *ConfiguredJarList) Append(apex string, jar string) ConfiguredJarList {
+	// Create a copy of the backing arrays before appending to avoid sharing backing
+	// arrays that are mutated across instances.
+	apexes := copyAndAppend(l.apexes, apex)
+	jars := copyAndAppend(l.jars, jar)
+
+	return ConfiguredJarList{apexes, jars}
 }
 
 // Filter out sublist.
-func (l *ConfiguredJarList) RemoveList(list ConfiguredJarList) {
+func (l *ConfiguredJarList) RemoveList(list ConfiguredJarList) ConfiguredJarList {
 	apexes := make([]string, 0, l.Len())
 	jars := make([]string, 0, l.Len())
 
@@ -1380,13 +1397,7 @@ func (l *ConfiguredJarList) RemoveList(list ConfiguredJarList) {
 		}
 	}
 
-	l.apexes = apexes
-	l.jars = jars
-}
-
-// A copy of itself.
-func (l *ConfiguredJarList) CopyOf() ConfiguredJarList {
-	return ConfiguredJarList{CopyOf(l.apexes), CopyOf(l.jars)}
+	return ConfiguredJarList{apexes, jars}
 }
 
 // A copy of the list of strings containing jar components.
@@ -1469,6 +1480,14 @@ func (l *ConfiguredJarList) DevicePaths(cfg Config, ostype OsType) []string {
 	return paths
 }
 
+func (l *ConfiguredJarList) String() string {
+	var pairs []string
+	for i := 0; i < l.Len(); i++ {
+		pairs = append(pairs, l.apexes[i]+":"+l.jars[i])
+	}
+	return strings.Join(pairs, ",")
+}
+
 func splitListOfPairsIntoPairOfLists(list []string) ([]string, []string, error) {
 	// Now we need to populate this list by splitting each item in the slice of
 	// pairs and appending them to the appropriate list of apexes or jars.

android/config_test.go

@@ -91,3 +91,49 @@ func TestMissingVendorConfig(t *testing.T) {
 		t.Errorf("Expected false")
 	}
 }
+
+func assertStringEquals(t *testing.T, expected, actual string) {
+	if actual != expected {
+		t.Errorf("expected %q found %q", expected, actual)
+	}
+}
+
+func TestConfiguredJarList(t *testing.T) {
+	list1 := CreateTestConfiguredJarList([]string{"apex1:jarA"})
+
+	t.Run("create", func(t *testing.T) {
+		assertStringEquals(t, "apex1:jarA", list1.String())
+	})
+
+	list2 := list1.Append("apex2", "jarB")
+	t.Run("append", func(t *testing.T) {
+		assertStringEquals(t, "apex1:jarA,apex2:jarB", list2.String())
+	})
+
+	t.Run("append does not modify", func(t *testing.T) {
+		assertStringEquals(t, "apex1:jarA", list1.String())
+	})
+
+	// Make sure that two lists created by appending to the same list do not share storage.
+	list3 := list1.Append("apex3", "jarC")
+	t.Run("append does not share", func(t *testing.T) {
+		assertStringEquals(t, "apex1:jarA,apex2:jarB", list2.String())
+		assertStringEquals(t, "apex1:jarA,apex3:jarC", list3.String())
+	})
+
+	list4 := list3.RemoveList(list1)
+	t.Run("remove", func(t *testing.T) {
+		assertStringEquals(t, "apex3:jarC", list4.String())
+	})
+
+	t.Run("remove does not modify", func(t *testing.T) {
+		assertStringEquals(t, "apex1:jarA,apex3:jarC", list3.String())
+	})
+
+	// Make sure that two lists created by removing from the same list do not share storage.
+	list5 := list3.RemoveList(CreateTestConfiguredJarList([]string{"apex3:jarC"}))
+	t.Run("remove", func(t *testing.T) {
+		assertStringEquals(t, "apex3:jarC", list4.String())
+		assertStringEquals(t, "apex1:jarA", list5.String())
+	})
+}

java/dexpreopt_config.go

@@ -81,13 +81,12 @@ func genBootImageConfigs(ctx android.PathContext) map[string]*bootImageConfig {
 		targets := dexpreoptTargets(ctx)
 		deviceDir := android.PathForOutput(ctx, ctx.Config().DeviceName())
 
-		artModules := global.ArtApexJars.CopyOf()
+		artModules := global.ArtApexJars
 		// With EMMA_INSTRUMENT_FRAMEWORK=true the Core libraries depend on jacoco.
 		if ctx.Config().IsEnvTrue("EMMA_INSTRUMENT_FRAMEWORK") {
-			artModules.Append("com.android.art", "jacocoagent")
+			artModules = artModules.Append("com.android.art", "jacocoagent")
 		}
-		frameworkModules := global.BootJars.CopyOf()
-		frameworkModules.RemoveList(artModules)
+		frameworkModules := global.BootJars.RemoveList(artModules)
 
 		artSubdir := "apex/art_boot_images/javalib"
 		frameworkSubdir := "system/framework"