From 52aa4e1fd4b291ad9c4021b747924307eadba34e Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Tue, 25 May 2021 15:20:39 +0000 Subject: [PATCH] Revert "Split asm and c flags and srcs in bp2build output" Revert submission 1714835-roboleaf-asm-c Reason for revert: TestCcLibraryStaticProductVariableSelects fails everywhere Reverted Changes: I28cf7437e:Split asm and c flags and srcs in bp2build output I2b47e6b55:Split libraries by language in cc_library_static Change-Id: I85d39a462f0a5b3f5ff3d685906813fab9f01358 --- android/bazel.go | 10 ++- bazel/properties.go | 48 -------------- bp2build/cc_library_conversion_test.go | 10 ++- bp2build/cc_library_static_conversion_test.go | 40 ++++++------ cc/bp2build.go | 65 +++---------------- cc/library.go | 43 +++--------- cc/object.go | 8 +-- 7 files changed, 52 insertions(+), 172 deletions(-) diff --git a/android/bazel.go b/android/bazel.go index c7669ac96..f56c24e04 100644 --- a/android/bazel.go +++ b/android/bazel.go @@ -220,7 +220,15 @@ var ( // Per-module denylist to opt modules out of mixed builds. Such modules will // still be generated via bp2build. mixedBuildsDisabledList = []string{ - "libc_common_shared", // cparsons@ cc_library_static, version script assignment of 'LIBC' to symbol '__cxa_atexit' failed: symbol not defined + "libc_common", // cparsons@ cc_library_static, depends on //bionic/libc:libc_nopthread + "libc_common_static", // cparsons@ cc_library_static, depends on //bionic/libc:libc_common + "libc_common_shared", // cparsons@ cc_library_static, depends on //bionic/libc:libc_common + "libc_netbsd", // lberki@, cc_library_static, version script assignment of 'LIBC_PRIVATE' to symbol 'SHA1Final' failed: symbol not defined + "libc_nopthread", // cparsons@ cc_library_static, version script assignment of 'LIBC' to symbol 'memcmp' failed: symbol not defined + "libc_openbsd", // ruperts@, cc_library_static, OK for bp2build but error: duplicate symbol: strcpy for mixed builds + "libarm-optimized-routines-string", // jingwen@, cc_library_static, OK for bp2build but b/186615213 (asflags not handled in bp2build), version script assignment of 'LIBC' to symbol 'memcmp' failed: symbol not defined (also for memrchr, strnlen) + "fmtlib_ndk", // http://b/187040371, cc_library_static, OK for bp2build but format-inl.h:11:10: fatal error: 'cassert' file not found for mixed builds + "libc_nomalloc", // cc_library_static, OK for bp2build but ld.lld: error: undefined symbol: pthread_mutex_lock (and others) } // Used for quicker lookups diff --git a/bazel/properties.go b/bazel/properties.go index 49c99c27d..640275f6f 100644 --- a/bazel/properties.go +++ b/bazel/properties.go @@ -137,54 +137,6 @@ func SubtractStrings(haystack []string, needle []string) []string { return strings } -// Return all needles in a given haystack, where needleFn is true for needles. -func FilterLabelList(haystack LabelList, needleFn func(string) bool) LabelList { - var includes []Label - - for _, inc := range haystack.Includes { - if needleFn(inc.Label) { - includes = append(includes, inc) - } - } - return LabelList{Includes: includes, Excludes: haystack.Excludes} -} - -// Return all needles in a given haystack, where needleFn is true for needles. -func FilterLabelListAttribute(haystack LabelListAttribute, needleFn func(string) bool) LabelListAttribute { - var result LabelListAttribute - - result.Value = FilterLabelList(haystack.Value, needleFn) - - for arch := range PlatformArchMap { - result.SetValueForArch(arch, FilterLabelList(haystack.GetValueForArch(arch), needleFn)) - } - - for os := range PlatformOsMap { - result.SetValueForOS(os, FilterLabelList(haystack.GetValueForOS(os), needleFn)) - } - - return result -} - -// Subtract needle from haystack -func SubtractBazelLabelListAttribute(haystack LabelListAttribute, needle LabelListAttribute) LabelListAttribute { - var result LabelListAttribute - - for arch := range PlatformArchMap { - result.SetValueForArch(arch, - SubtractBazelLabelList(haystack.GetValueForArch(arch), needle.GetValueForArch(arch))) - } - - for os := range PlatformOsMap { - result.SetValueForOS(os, - SubtractBazelLabelList(haystack.GetValueForOS(os), needle.GetValueForOS(os))) - } - - result.Value = SubtractBazelLabelList(haystack.Value, needle.Value) - - return result -} - // Subtract needle from haystack func SubtractBazelLabels(haystack []Label, needle []Label) []Label { // This is really a set diff --git a/bp2build/cc_library_conversion_test.go b/bp2build/cc_library_conversion_test.go index 454b15320..0a729374e 100644 --- a/bp2build/cc_library_conversion_test.go +++ b/bp2build/cc_library_conversion_test.go @@ -311,7 +311,7 @@ cc_library { "//build/bazel/platforms/arch:arm64": ["-DHAVE_FAST_FMA=1"], "//conditions:default": [], }), - srcs_c = ["math/cosf.c"], + srcs = ["math/cosf.c"], )`}, }) } @@ -618,7 +618,7 @@ cc_library { func TestCcLibraryCppFlagsGoesIntoCopts(t *testing.T) { runCcLibraryTestCase(t, bp2buildTestCase{ - description: "cc_library cppflags usage", + description: "cc_library cppflags goes into copts", moduleTypeUnderTest: "cc_library", moduleTypeUnderTestFactory: cc.LibraryFactory, moduleTypeUnderTestBp2BuildMutator: cc.CcLibraryBp2Build, @@ -654,12 +654,10 @@ func TestCcLibraryCppFlagsGoesIntoCopts(t *testing.T) { name = "a", copts = [ "-Wall", - "-Ifoo/bar", - "-I$(BINDIR)/foo/bar", - ], - cppflags = [ "-fsigned-char", "-pedantic", + "-Ifoo/bar", + "-I$(BINDIR)/foo/bar", ] + select({ "//build/bazel/platforms/arch:arm64": ["-DARM64=1"], "//conditions:default": [], diff --git a/bp2build/cc_library_static_conversion_test.go b/bp2build/cc_library_static_conversion_test.go index 6c2b31731..44c6ad4d1 100644 --- a/bp2build/cc_library_static_conversion_test.go +++ b/bp2build/cc_library_static_conversion_test.go @@ -650,7 +650,7 @@ cc_library_static { "-I$(BINDIR)/.", ], linkstatic = True, - srcs_c = [ + srcs = [ "common.c", "foo-a.c", ], @@ -682,7 +682,7 @@ cc_library_static { "-I$(BINDIR)/.", ], linkstatic = True, - srcs_c = ["common.c"] + select({ + srcs = ["common.c"] + select({ "//build/bazel/platforms/arch:arm": ["foo-arm.c"], "//conditions:default": [], }), @@ -719,7 +719,7 @@ cc_library_static { "-I$(BINDIR)/.", ], linkstatic = True, - srcs_c = ["common.c"] + select({ + srcs = ["common.c"] + select({ "//build/bazel/platforms/arch:arm": ["for-arm.c"], "//conditions:default": ["not-for-arm.c"], }), @@ -758,7 +758,7 @@ cc_library_static { "-I$(BINDIR)/.", ], linkstatic = True, - srcs_c = ["common.c"] + select({ + srcs = ["common.c"] + select({ "//build/bazel/platforms/arch:arm": [ "for-arm.c", "not-for-x86.c", @@ -813,7 +813,7 @@ cc_library_static { "-I$(BINDIR)/.", ], linkstatic = True, - srcs_c = ["common.c"] + select({ + srcs = ["common.c"] + select({ "//build/bazel/platforms/arch:arm": [ "for-arm.c", "not-for-arm64.c", @@ -909,7 +909,7 @@ cc_library_static { "-I$(BINDIR)/.", ], linkstatic = True, - srcs_c = ["common.c"] + select({ + srcs = ["common.c"] + select({ "//build/bazel/platforms/arch:arm": ["for-lib32.c"], "//build/bazel/platforms/arch:x86": ["for-lib32.c"], "//conditions:default": ["not-for-lib32.c"], @@ -948,7 +948,7 @@ cc_library_static { "-I$(BINDIR)/.", ], linkstatic = True, - srcs_c = ["common.c"] + select({ + srcs = ["common.c"] + select({ "//build/bazel/platforms/arch:arm": [ "for-lib32.c", "not-for-lib64.c", @@ -1020,7 +1020,7 @@ cc_library_static { "-I$(BINDIR)/.", ], linkstatic = True, - srcs_c = ["common.c"] + select({ + srcs = ["common.c"] + select({ "//build/bazel/platforms/arch:arm": [ "for-arm.c", "for-lib32.c", @@ -1074,10 +1074,10 @@ func TestCcLibraryStaticArchSrcsExcludeSrcsGeneratedFiles(t *testing.T) { moduleTypeUnderTestBp2BuildMutator: cc.CcLibraryStaticBp2Build, depsMutators: []android.RegisterMutatorFunc{cc.RegisterDepsBp2Build}, filesystem: map[string]string{ - "common.cpp": "", - "for-x86.cpp": "", - "not-for-x86.cpp": "", - "not-for-everything.cpp": "", + "common.c": "", + "for-x86.c": "", + "not-for-x86.c": "", + "not-for-everything.c": "", "dep/Android.bp": ` genrule { name: "generated_src_other_pkg", @@ -1118,14 +1118,14 @@ genrule { cc_library_static { name: "foo_static3", - srcs: ["common.cpp", "not-for-*.cpp"], - exclude_srcs: ["not-for-everything.cpp"], + srcs: ["common.c", "not-for-*.c"], + exclude_srcs: ["not-for-everything.c"], generated_sources: ["generated_src", "generated_src_other_pkg"], generated_headers: ["generated_hdr", "generated_hdr_other_pkg"], arch: { x86: { - srcs: ["for-x86.cpp"], - exclude_srcs: ["not-for-x86.cpp"], + srcs: ["for-x86.c"], + exclude_srcs: ["not-for-x86.c"], generated_sources: ["generated_src_x86"], generated_headers: ["generated_hdr_other_pkg_x86"], }, @@ -1144,14 +1144,14 @@ cc_library_static { "//dep:generated_src_other_pkg", ":generated_hdr", ":generated_src", - "common.cpp", + "common.c", ] + select({ "//build/bazel/platforms/arch:x86": [ "//dep:generated_hdr_other_pkg_x86", ":generated_src_x86", - "for-x86.cpp", + "for-x86.c", ], - "//conditions:default": ["not-for-x86.cpp"], + "//conditions:default": ["not-for-x86.c"], }), )`}, }) @@ -1197,7 +1197,7 @@ cc_library_static { "//conditions:default": [], }), linkstatic = True, - srcs_c = ["common.c"], + srcs = ["common.c"], )`}, }) } diff --git a/cc/bp2build.go b/cc/bp2build.go index e417c6907..0c827c5ed 100644 --- a/cc/bp2build.go +++ b/cc/bp2build.go @@ -183,26 +183,15 @@ func bp2BuildParseStaticProps(ctx android.TopDownMutatorContext, module *Module) // Convenience struct to hold all attributes parsed from compiler properties. type compilerAttributes struct { - // Options for all languages - copts bazel.StringListAttribute - // Assembly options and sources - asFlags bazel.StringListAttribute - asSrcs bazel.LabelListAttribute - // C options and sources - conlyFlags bazel.StringListAttribute - cSrcs bazel.LabelListAttribute - // C++ options and sources - cppFlags bazel.StringListAttribute + copts bazel.StringListAttribute srcs bazel.LabelListAttribute + includes bazel.StringListAttribute } // bp2BuildParseCompilerProps returns copts, srcs and hdrs and other attributes. func bp2BuildParseCompilerProps(ctx android.TopDownMutatorContext, module *Module) compilerAttributes { var srcs bazel.LabelListAttribute var copts bazel.StringListAttribute - var asFlags bazel.StringListAttribute - var conlyFlags bazel.StringListAttribute - var cppFlags bazel.StringListAttribute // Creates the -I flags for a directory, while making the directory relative // to the exec root for Bazel to work. @@ -226,21 +215,15 @@ func bp2BuildParseCompilerProps(ctx android.TopDownMutatorContext, module *Modul return append(includeDirs, baseCompilerProps.Local_include_dirs...) } - parseCommandLineFlags := func(soongFlags []string) []string { - var result []string - for _, flag := range soongFlags { - // Soong's cflags can contain spaces, like `-include header.h`. For - // Bazel's copts, split them up to be compatible with the - // no_copts_tokenization feature. - result = append(result, strings.Split(flag, " ")...) - } - return result - } - // Parse the list of copts. parseCopts := func(baseCompilerProps *BaseCompilerProperties) []string { var copts []string - copts = append(copts, parseCommandLineFlags(baseCompilerProps.Cflags)...) + for _, flag := range append(baseCompilerProps.Cflags, baseCompilerProps.Cppflags...) { + // Soong's cflags can contain spaces, like `-include header.h`. For + // Bazel's copts, split them up to be compatible with the + // no_copts_tokenization feature. + copts = append(copts, strings.Split(flag, " ")...) + } for _, dir := range parseLocalIncludeDirs(baseCompilerProps) { copts = append(copts, includeFlags(dir)...) } @@ -277,9 +260,6 @@ func bp2BuildParseCompilerProps(ctx android.TopDownMutatorContext, module *Modul if baseCompilerProps, ok := props.(*BaseCompilerProperties); ok { srcs.Value = parseSrcs(baseCompilerProps) copts.Value = parseCopts(baseCompilerProps) - asFlags.Value = parseCommandLineFlags(baseCompilerProps.Asflags) - conlyFlags.Value = parseCommandLineFlags(baseCompilerProps.Conlyflags) - cppFlags.Value = parseCommandLineFlags(baseCompilerProps.Cppflags) // Used for arch-specific srcs later. baseSrcs = baseCompilerProps.Srcs @@ -310,9 +290,6 @@ func bp2BuildParseCompilerProps(ctx android.TopDownMutatorContext, module *Modul } copts.SetValueForArch(arch.Name, parseCopts(baseCompilerProps)) - asFlags.SetValueForArch(arch.Name, parseCommandLineFlags(baseCompilerProps.Asflags)) - conlyFlags.SetValueForArch(arch.Name, parseCommandLineFlags(baseCompilerProps.Conlyflags)) - cppFlags.SetValueForArch(arch.Name, parseCommandLineFlags(baseCompilerProps.Cppflags)) } } @@ -338,9 +315,6 @@ func bp2BuildParseCompilerProps(ctx android.TopDownMutatorContext, module *Modul // TODO(b/186153868): add support for os-specific srcs and exclude_srcs srcs.SetValueForOS(os.Name, bazel.SubtractBazelLabelList(srcsList, baseSrcsLabelList)) copts.SetValueForOS(os.Name, parseCopts(baseCompilerProps)) - asFlags.SetValueForOS(os.Name, parseCommandLineFlags(baseCompilerProps.Asflags)) - conlyFlags.SetValueForOS(os.Name, parseCommandLineFlags(baseCompilerProps.Conlyflags)) - cppFlags.SetValueForOS(os.Name, parseCommandLineFlags(baseCompilerProps.Cppflags)) } } @@ -359,28 +333,9 @@ func bp2BuildParseCompilerProps(ctx android.TopDownMutatorContext, module *Modul } } - // Branch srcs into three language-specific groups. - // C++ is the "catch-all" group, and comprises generated sources because we don't - // know the language of these sources until the genrule is executed. - // TODO(b/): Handle language detection of sources in a Bazel rule. - isCSrc := func(s string) bool { - return strings.HasSuffix(s, ".c") - } - isAsmSrc := func(s string) bool { - return strings.HasSuffix(s, ".S") || strings.HasSuffix(s, ".s") - } - cSrcs := bazel.FilterLabelListAttribute(srcs, isCSrc) - asSrcs := bazel.FilterLabelListAttribute(srcs, isAsmSrc) - srcs = bazel.SubtractBazelLabelListAttribute(srcs, cSrcs) - srcs = bazel.SubtractBazelLabelListAttribute(srcs, asSrcs) return compilerAttributes{ - copts: copts, - srcs: srcs, - asFlags: asFlags, - asSrcs: asSrcs, - cSrcs: cSrcs, - conlyFlags: conlyFlags, - cppFlags: cppFlags, + srcs: srcs, + copts: copts, } } diff --git a/cc/library.go b/cc/library.go index 9fb7a2451..c918b96ba 100644 --- a/cc/library.go +++ b/cc/library.go @@ -230,13 +230,6 @@ type bazelCcLibraryAttributes struct { Copts bazel.StringListAttribute Includes bazel.StringListAttribute Linkopts bazel.StringListAttribute - - Cppflags bazel.StringListAttribute - Srcs_c bazel.LabelListAttribute - Conlyflags bazel.StringListAttribute - Srcs_as bazel.LabelListAttribute - Asflags bazel.StringListAttribute - // Attributes pertaining to shared variant. Shared_copts bazel.StringListAttribute Shared_srcs bazel.LabelListAttribute @@ -246,7 +239,6 @@ type bazelCcLibraryAttributes struct { Whole_archive_deps_for_shared bazel.LabelListAttribute User_link_flags bazel.StringListAttribute Version_script bazel.LabelAttribute - // Attributes pertaining to static variant. Static_copts bazel.StringListAttribute Static_srcs bazel.LabelListAttribute @@ -302,27 +294,20 @@ func CcLibraryBp2Build(ctx android.TopDownMutatorContext) { srcs.Append(compilerAttrs.srcs) attrs := &bazelCcLibraryAttributes{ - Srcs: srcs, - Implementation_deps: linkerAttrs.deps, - Deps: linkerAttrs.exportedDeps, - Dynamic_deps: linkerAttrs.dynamicDeps, - Whole_archive_deps: linkerAttrs.wholeArchiveDeps, - Copts: compilerAttrs.copts, - Includes: exportedIncludes, - Linkopts: linkerAttrs.linkopts, - Cppflags: compilerAttrs.cppFlags, - Srcs_c: compilerAttrs.cSrcs, - Conlyflags: compilerAttrs.conlyFlags, - Srcs_as: compilerAttrs.asSrcs, - Asflags: compilerAttrs.asFlags, - + Srcs: srcs, + Implementation_deps: linkerAttrs.deps, + Deps: linkerAttrs.exportedDeps, + Dynamic_deps: linkerAttrs.dynamicDeps, + Whole_archive_deps: linkerAttrs.wholeArchiveDeps, + Copts: compilerAttrs.copts, + Includes: exportedIncludes, + Linkopts: linkerAttrs.linkopts, Shared_copts: sharedAttrs.copts, Shared_srcs: sharedAttrs.srcs, Static_deps_for_shared: sharedAttrs.staticDeps, Whole_archive_deps_for_shared: sharedAttrs.wholeArchiveDeps, Dynamic_deps_for_shared: sharedAttrs.dynamicDeps, Version_script: linkerAttrs.versionScript, - Static_copts: staticAttrs.copts, Static_srcs: staticAttrs.srcs, Static_deps_for_static: staticAttrs.staticDeps, @@ -2245,12 +2230,6 @@ type bazelCcLibraryStaticAttributes struct { Linkstatic bool Includes bazel.StringListAttribute Hdrs bazel.LabelListAttribute - - Cppflags bazel.StringListAttribute - Srcs_c bazel.LabelListAttribute - Conlyflags bazel.StringListAttribute - Srcs_as bazel.LabelListAttribute - Asflags bazel.StringListAttribute } type bazelCcLibraryStatic struct { @@ -2280,12 +2259,6 @@ func ccLibraryStaticBp2BuildInternal(ctx android.TopDownMutatorContext, module * Linkopts: linkerAttrs.linkopts, Linkstatic: true, Includes: exportedIncludes, - - Cppflags: compilerAttrs.cppFlags, - Srcs_c: compilerAttrs.cSrcs, - Conlyflags: compilerAttrs.conlyFlags, - Srcs_as: compilerAttrs.asSrcs, - Asflags: compilerAttrs.asFlags, } props := bazel.BazelTargetModuleProperties{ diff --git a/cc/object.go b/cc/object.go index cd711617b..704cb697d 100644 --- a/cc/object.go +++ b/cc/object.go @@ -185,14 +185,8 @@ func ObjectBp2Build(ctx android.TopDownMutatorContext) { } // TODO(b/183595872) warn/error if we're not handling product variables - // Don't split cc_object srcs across languages. Doing so would add complexity, - // and this isn't typically done for cc_object. - srcs := compilerAttrs.srcs - srcs.Append(compilerAttrs.cSrcs) - srcs.Append(compilerAttrs.asSrcs) - attrs := &bazelObjectAttributes{ - Srcs: srcs, + Srcs: compilerAttrs.srcs, Deps: deps, Copts: compilerAttrs.copts, Asflags: asFlags,