Merge "Have Soong cc static linker dep order account for shared deps too"

cc/cc.go

@@ -326,9 +326,12 @@ type Module struct {
 	flags Flags
 
 	// When calling a linker, if module A depends on module B, then A must precede B in its command
-	// line invocation. staticDepsInLinkOrder stores the proper ordering of all of the transitive
+	// line invocation. depsInLinkOrder stores the proper ordering of all of the transitive
 	// deps of this module
-	staticDepsInLinkOrder android.Paths
+	depsInLinkOrder android.Paths
+
+	// only non-nil when this is a shared library that reuses the objects of a static library
+	staticVariant *Module
 }
 
 func (c *Module) Init() android.Module {
@@ -537,7 +540,8 @@ func (c *Module) Name() string {
 // orderDeps reorders dependencies into a list such that if module A depends on B, then
 // A will precede B in the resultant list.
 // This is convenient for passing into a linker.
-func orderDeps(directDeps []android.Path, transitiveDeps map[android.Path][]android.Path) (orderedAllDeps []android.Path, orderedDeclaredDeps []android.Path) {
+// Note that directSharedDeps should be the analogous static library for each shared lib dep
+func orderDeps(directStaticDeps []android.Path, directSharedDeps []android.Path, allTransitiveDeps map[android.Path][]android.Path) (orderedAllDeps []android.Path, orderedDeclaredDeps []android.Path) {
 	// If A depends on B, then
 	//   Every list containing A will also contain B later in the list
 	//   So, after concatenating all lists, the final instance of B will have come from the same
@@ -545,38 +549,46 @@ func orderDeps(directDeps []android.Path, transitiveDeps map[android.Path][]andr
 	//   So, the final instance of B will be later in the concatenation than the final A
 	//   So, keeping only the final instance of A and of B ensures that A is earlier in the output
 	//     list than B
-	for _, dep := range directDeps {
+	for _, dep := range directStaticDeps {
 		orderedAllDeps = append(orderedAllDeps, dep)
-		orderedAllDeps = append(orderedAllDeps, transitiveDeps[dep]...)
+		orderedAllDeps = append(orderedAllDeps, allTransitiveDeps[dep]...)
+	}
+	for _, dep := range directSharedDeps {
+		orderedAllDeps = append(orderedAllDeps, dep)
+		orderedAllDeps = append(orderedAllDeps, allTransitiveDeps[dep]...)
 	}
 
 	orderedAllDeps = android.LastUniquePaths(orderedAllDeps)
 
-	// We don't want to add any new dependencies into directDeps (to allow the caller to
+	// We don't want to add any new dependencies into directStaticDeps (to allow the caller to
 	// intentionally exclude or replace any unwanted transitive dependencies), so we limit the
-	// resultant list to only what the caller has chosen to include in directDeps
+	// resultant list to only what the caller has chosen to include in directStaticDeps
-	_, orderedDeclaredDeps = android.FilterPathList(orderedAllDeps, directDeps)
+	_, orderedDeclaredDeps = android.FilterPathList(orderedAllDeps, directStaticDeps)
 
 	return orderedAllDeps, orderedDeclaredDeps
 }
 
-func orderStaticModuleDeps(module *Module, deps []*Module) (results []android.Path) {
+func orderStaticModuleDeps(module *Module, staticDeps []*Module, sharedDeps []*Module) (results []android.Path) {
-	// make map of transitive dependencies
+	// convert Module to Path
-	transitiveStaticDepNames := make(map[android.Path][]android.Path, len(deps))
+	allTransitiveDeps := make(map[android.Path][]android.Path, len(staticDeps))
-	for _, dep := range deps {
+	staticDepFiles := []android.Path{}
-		transitiveStaticDepNames[dep.outputFile.Path()] = dep.staticDepsInLinkOrder
+	for _, dep := range staticDeps {
+		allTransitiveDeps[dep.outputFile.Path()] = dep.depsInLinkOrder
+		staticDepFiles = append(staticDepFiles, dep.outputFile.Path())
 	}
-	// get the output file for each declared dependency
+	sharedDepFiles := []android.Path{}
-	depFiles := []android.Path{}
+	for _, sharedDep := range sharedDeps {
-	for _, dep := range deps {
+		staticAnalogue := sharedDep.staticVariant
-		depFiles = append(depFiles, dep.outputFile.Path())
+		if staticAnalogue != nil {
+			allTransitiveDeps[staticAnalogue.outputFile.Path()] = staticAnalogue.depsInLinkOrder
+			sharedDepFiles = append(sharedDepFiles, staticAnalogue.outputFile.Path())
+		}
 	}
 
 	// reorder the dependencies based on transitive dependencies
-	module.staticDepsInLinkOrder, results = orderDeps(depFiles, transitiveStaticDepNames)
+	module.depsInLinkOrder, results = orderDeps(staticDepFiles, sharedDepFiles, allTransitiveDeps)
 
 	return results
-
 }
 
 func (c *Module) GenerateAndroidBuildActions(actx android.ModuleContext) {
@@ -1026,6 +1038,7 @@ func (c *Module) depsToPaths(ctx android.ModuleContext) PathDeps {
 	var depPaths PathDeps
 
 	directStaticDeps := []*Module{}
+	directSharedDeps := []*Module{}
 
 	ctx.VisitDirectDeps(func(dep android.Module) {
 		depName := ctx.OtherModuleName(dep)
@@ -1092,6 +1105,7 @@ func (c *Module) depsToPaths(ctx android.ModuleContext) PathDeps {
 		// re-exporting flags
 		if depTag == reuseObjTag {
 			if l, ok := ccDep.compiler.(libraryInterface); ok {
+				c.staticVariant = ccDep
 				objs, flags, deps := l.reuseObjs()
 				depPaths.Objs = depPaths.Objs.Append(objs)
 				depPaths.ReexportedFlags = append(depPaths.ReexportedFlags, flags...)
@@ -1130,6 +1144,7 @@ func (c *Module) depsToPaths(ctx android.ModuleContext) PathDeps {
 			ptr = &depPaths.SharedLibs
 			depPtr = &depPaths.SharedLibsDeps
 			depFile = ccDep.linker.(libraryInterface).toc()
+			directSharedDeps = append(directSharedDeps, ccDep)
 		case lateSharedDepTag, ndkLateStubDepTag:
 			ptr = &depPaths.LateSharedLibs
 			depPtr = &depPaths.LateSharedLibsDeps
@@ -1221,7 +1236,7 @@ func (c *Module) depsToPaths(ctx android.ModuleContext) PathDeps {
 	})
 
 	// use the ordered dependencies as this module's dependencies
-	depPaths.StaticLibs = append(depPaths.StaticLibs, orderStaticModuleDeps(c, directStaticDeps)...)
+	depPaths.StaticLibs = append(depPaths.StaticLibs, orderStaticModuleDeps(c, directStaticDeps, directSharedDeps)...)
 
 	// Dedup exported flags from dependencies
 	depPaths.Flags = android.FirstUniqueStrings(depPaths.Flags)

cc/cc_test.go

@@ -289,7 +289,11 @@ func TestSplitListForSize(t *testing.T) {
 var staticLinkDepOrderTestCases = []struct {
 	// This is a string representation of a map[moduleName][]moduleDependency .
 	// It models the dependencies declared in an Android.bp file.
-	in string
+	inStatic string
+
+	// This is a string representation of a map[moduleName][]moduleDependency .
+	// It models the dependencies declared in an Android.bp file.
+	inShared string
 
 	// allOrdered is a string representation of a map[moduleName][]moduleDependency .
 	// The keys of allOrdered specify which modules we would like to check.
@@ -305,82 +309,99 @@ var staticLinkDepOrderTestCases = []struct {
 }{
 	// Simple tests
 	{
-		in:         "",
+		inStatic:   "",
 		outOrdered: "",
 	},
 	{
-		in:         "a:",
+		inStatic:   "a:",
 		outOrdered: "a:",
 	},
 	{
-		in:         "a:b; b:",
+		inStatic:   "a:b; b:",
 		outOrdered: "a:b; b:",
 	},
 	// Tests of reordering
 	{
 		// diamond example
-		in:         "a:d,b,c; b:d; c:d; d:",
+		inStatic:   "a:d,b,c; b:d; c:d; d:",
 		outOrdered: "a:b,c,d; b:d; c:d; d:",
 	},
 	{
 		// somewhat real example
-		in:         "bsdiff_unittest:b,c,d,e,f,g,h,i; e:b",
+		inStatic:   "bsdiff_unittest:b,c,d,e,f,g,h,i; e:b",
 		outOrdered: "bsdiff_unittest:c,d,e,b,f,g,h,i; e:b",
 	},
 	{
 		// multiple reorderings
-		in:         "a:b,c,d,e; d:b; e:c",
+		inStatic:   "a:b,c,d,e; d:b; e:c",
 		outOrdered: "a:d,b,e,c; d:b; e:c",
 	},
 	{
 		// should reorder without adding new transitive dependencies
-		in:         "bin:lib2,lib1;             lib1:lib2,liboptional",
+		inStatic:   "bin:lib2,lib1;             lib1:lib2,liboptional",
 		allOrdered: "bin:lib1,lib2,liboptional; lib1:lib2,liboptional",
 		outOrdered: "bin:lib1,lib2;             lib1:lib2,liboptional",
 	},
 	{
 		// multiple levels of dependencies
-		in:         "a:b,c,d,e,f,g,h; f:b,c,d; b:c,d; c:d",
+		inStatic:   "a:b,c,d,e,f,g,h; f:b,c,d; b:c,d; c:d",
 		allOrdered: "a:e,f,b,c,d,g,h; f:b,c,d; b:c,d; c:d",
 		outOrdered: "a:e,f,b,c,d,g,h; f:b,c,d; b:c,d; c:d",
 	},
+	// shared dependencies
+	{
+		// Note that this test doesn't recurse, to minimize the amount of logic it tests.
+		// So, we don't actually have to check that a shared dependency of c will change the order
+		// of a library that depends statically on b and on c.  We only need to check that if c has
+		// a shared dependency on b, that that shows up in allOrdered.
+		inShared:   "c:b",
+		allOrdered: "c:b",
+		outOrdered: "c:",
+	},
+	{
+		// This test doesn't actually include any shared dependencies but it's a reminder of what
+		// the second phase of the above test would look like
+		inStatic:   "a:b,c; c:b",
+		allOrdered: "a:c,b; c:b",
+		outOrdered: "a:c,b; c:b",
+	},
 	// tiebreakers for when two modules specifying different orderings and there is no dependency
 	// to dictate an order
 	{
 		// if the tie is between two modules at the end of a's deps, then a's order wins
-		in:         "a1:b,c,d,e; a2:b,c,e,d; b:d,e; c:e,d",
+		inStatic:   "a1:b,c,d,e; a2:b,c,e,d; b:d,e; c:e,d",
 		outOrdered: "a1:b,c,d,e; a2:b,c,e,d; b:d,e; c:e,d",
 	},
 	{
 		// if the tie is between two modules at the start of a's deps, then c's order is used
-		in:         "a1:d,e,b1,c1; b1:d,e; c1:e,d;   a2:d,e,b2,c2; b2:d,e; c2:d,e",
+		inStatic:   "a1:d,e,b1,c1; b1:d,e; c1:e,d;   a2:d,e,b2,c2; b2:d,e; c2:d,e",
 		outOrdered: "a1:b1,c1,e,d; b1:d,e; c1:e,d;   a2:b2,c2,d,e; b2:d,e; c2:d,e",
 	},
 	// Tests involving duplicate dependencies
 	{
 		// simple duplicate
-		in:         "a:b,c,c,b",
+		inStatic:   "a:b,c,c,b",
 		outOrdered: "a:c,b",
 	},
 	{
 		// duplicates with reordering
-		in:         "a:b,c,d,c; c:b",
+		inStatic:   "a:b,c,d,c; c:b",
 		outOrdered: "a:d,c,b",
 	},
 	// Tests to confirm the nonexistence of infinite loops.
 	// These cases should never happen, so as long as the test terminates and the
 	// result is deterministic then that should be fine.
 	{
-		in:         "a:a",
+		inStatic:   "a:a",
 		outOrdered: "a:a",
 	},
 	{
-		in:         "a:b;   b:c;   c:a",
+		inStatic:   "a:b;   b:c;   c:a",
 		allOrdered: "a:b,c; b:c,a; c:a,b",
 		outOrdered: "a:b;   b:c;   c:a",
 	},
 	{
-		in:         "a:b,c;   b:c,a;   c:a,b",
+		inStatic:   "a:b,c;   b:c,a;   c:a,b",
 		allOrdered: "a:c,a,b; b:a,b,c; c:b,c,a",
 		outOrdered: "a:c,b;   b:a,c;   c:b,a",
 	},
@@ -427,43 +448,47 @@ func parseModuleDeps(text string) (modulesInOrder []android.Path, allDeps map[an
 	return modulesInOrder, allDeps
 }
 
-func TestStaticLinkDependencyOrdering(t *testing.T) {
+func TestLinkReordering(t *testing.T) {
 	for _, testCase := range staticLinkDepOrderTestCases {
 		errs := []string{}
 
 		// parse testcase
-		_, givenTransitiveDeps := parseModuleDeps(testCase.in)
+		_, givenTransitiveDeps := parseModuleDeps(testCase.inStatic)
 		expectedModuleNames, expectedTransitiveDeps := parseModuleDeps(testCase.outOrdered)
 		if testCase.allOrdered == "" {
 			// allow the test case to skip specifying allOrdered
 			testCase.allOrdered = testCase.outOrdered
 		}
 		_, expectedAllDeps := parseModuleDeps(testCase.allOrdered)
+		_, givenAllSharedDeps := parseModuleDeps(testCase.inShared)
 
 		// For each module whose post-reordered dependencies were specified, validate that
 		// reordering the inputs produces the expected outputs.
 		for _, moduleName := range expectedModuleNames {
 			moduleDeps := givenTransitiveDeps[moduleName]
-			orderedAllDeps, orderedDeclaredDeps := orderDeps(moduleDeps, givenTransitiveDeps)
+			givenSharedDeps := givenAllSharedDeps[moduleName]
+			orderedAllDeps, orderedDeclaredDeps := orderDeps(moduleDeps, givenSharedDeps, givenTransitiveDeps)
 
 			correctAllOrdered := expectedAllDeps[moduleName]
 			if !reflect.DeepEqual(orderedAllDeps, correctAllOrdered) {
 				errs = append(errs, fmt.Sprintf("orderDeps returned incorrect orderedAllDeps."+
-					"\nInput:    %q"+
+					"\nin static:%q"+
+					"\nin shared:%q"+
 					"\nmodule:   %v"+
 					"\nexpected: %s"+
 					"\nactual:   %s",
-					testCase.in, moduleName, correctAllOrdered, orderedAllDeps))
+					testCase.inStatic, testCase.inShared, moduleName, correctAllOrdered, orderedAllDeps))
 			}
 
 			correctOutputDeps := expectedTransitiveDeps[moduleName]
 			if !reflect.DeepEqual(correctOutputDeps, orderedDeclaredDeps) {
 				errs = append(errs, fmt.Sprintf("orderDeps returned incorrect orderedDeclaredDeps."+
-					"\nInput:    %q"+
+					"\nin static:%q"+
+					"\nin shared:%q"+
 					"\nmodule:   %v"+
 					"\nexpected: %s"+
 					"\nactual:   %s",
-					testCase.in, moduleName, correctOutputDeps, orderedDeclaredDeps))
+					testCase.inStatic, testCase.inShared, moduleName, correctOutputDeps, orderedDeclaredDeps))
 			}
 		}
 
@@ -493,7 +518,7 @@ func getOutputPaths(ctx *android.TestContext, variant string, moduleNames []stri
 	return paths
 }
 
-func TestLibDeps(t *testing.T) {
+func TestStaticLibDepReordering(t *testing.T) {
 	ctx := testCc(t, `
 	cc_library {
 		name: "a",
@@ -514,7 +539,7 @@ func TestLibDeps(t *testing.T) {
 
 	variant := "android_arm64_armv8-a_core_static"
 	moduleA := ctx.ModuleForTests("a", variant).Module().(*Module)
-	actual := moduleA.staticDepsInLinkOrder
+	actual := moduleA.depsInLinkOrder
 	expected := getOutputPaths(ctx, variant, []string{"c", "b", "d"})
 
 	if !reflect.DeepEqual(actual, expected) {
@@ -527,6 +552,37 @@ func TestLibDeps(t *testing.T) {
 	}
 }
 
+func TestStaticLibDepReorderingWithShared(t *testing.T) {
+	ctx := testCc(t, `
+	cc_library {
+		name: "a",
+		static_libs: ["b", "c"],
+	}
+	cc_library {
+		name: "b",
+	}
+	cc_library {
+		name: "c",
+		shared_libs: ["b"],
+	}
+
+	`)
+
+	variant := "android_arm64_armv8-a_core_static"
+	moduleA := ctx.ModuleForTests("a", variant).Module().(*Module)
+	actual := moduleA.depsInLinkOrder
+	expected := getOutputPaths(ctx, variant, []string{"c", "b"})
+
+	if !reflect.DeepEqual(actual, expected) {
+		t.Errorf("staticDeps orderings did not account for shared libs"+
+			"\nactual:   %v"+
+			"\nexpected: %v",
+			actual,
+			expected,
+		)
+	}
+}
+
 var compilerFlagsTestCases = []struct {
 	in  string
 	out bool