From af24cdd99f1fbfc6831dfdddc8e896d5e6e82ba8 Mon Sep 17 00:00:00 2001 From: Chris Parsons Date: Fri, 21 May 2021 18:50:44 -0400 Subject: [PATCH] Split asm and c flags and srcs in bp2build output This allows removal of almost all current items from the mixed build denylist, which were previously broken due to being unable to separately control flags for compilations of different languages within the same target. Note that this does not appropriately implement asm/c srcs and flags for either the shared variant or the static variant. This will require a followup. Test: bp2build.sh and mixed_libc.sh CI scripts Test: Updated b2build tests Change-Id: I28cf7437ee96cdf2fdbcb1eda2303691cff08ba4 --- 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 | 63 ++++++++++++++++--- cc/library.go | 43 ++++++++++--- cc/object.go | 8 ++- 7 files changed, 171 insertions(+), 51 deletions(-) diff --git a/android/bazel.go b/android/bazel.go index f56c24e04..c7669ac96 100644 --- a/android/bazel.go +++ b/android/bazel.go @@ -220,15 +220,7 @@ var ( // Per-module denylist to opt modules out of mixed builds. Such modules will // still be generated via bp2build. mixedBuildsDisabledList = []string{ - "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) + "libc_common_shared", // cparsons@ cc_library_static, version script assignment of 'LIBC' to symbol '__cxa_atexit' failed: symbol not defined } // Used for quicker lookups diff --git a/bazel/properties.go b/bazel/properties.go index 640275f6f..49c99c27d 100644 --- a/bazel/properties.go +++ b/bazel/properties.go @@ -137,6 +137,54 @@ 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 0a729374e..454b15320 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 = ["math/cosf.c"], + srcs_c = ["math/cosf.c"], )`}, }) } @@ -618,7 +618,7 @@ cc_library { func TestCcLibraryCppFlagsGoesIntoCopts(t *testing.T) { runCcLibraryTestCase(t, bp2buildTestCase{ - description: "cc_library cppflags goes into copts", + description: "cc_library cppflags usage", moduleTypeUnderTest: "cc_library", moduleTypeUnderTestFactory: cc.LibraryFactory, moduleTypeUnderTestBp2BuildMutator: cc.CcLibraryBp2Build, @@ -654,10 +654,12 @@ func TestCcLibraryCppFlagsGoesIntoCopts(t *testing.T) { name = "a", copts = [ "-Wall", - "-fsigned-char", - "-pedantic", "-Ifoo/bar", "-I$(BINDIR)/foo/bar", + ], + cppflags = [ + "-fsigned-char", + "-pedantic", ] + 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 44c6ad4d1..6c2b31731 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 = [ + srcs_c = [ "common.c", "foo-a.c", ], @@ -682,7 +682,7 @@ cc_library_static { "-I$(BINDIR)/.", ], linkstatic = True, - srcs = ["common.c"] + select({ + srcs_c = ["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 = ["common.c"] + select({ + srcs_c = ["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 = ["common.c"] + select({ + srcs_c = ["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 = ["common.c"] + select({ + srcs_c = ["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 = ["common.c"] + select({ + srcs_c = ["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 = ["common.c"] + select({ + srcs_c = ["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 = ["common.c"] + select({ + srcs_c = ["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.c": "", - "for-x86.c": "", - "not-for-x86.c": "", - "not-for-everything.c": "", + "common.cpp": "", + "for-x86.cpp": "", + "not-for-x86.cpp": "", + "not-for-everything.cpp": "", "dep/Android.bp": ` genrule { name: "generated_src_other_pkg", @@ -1118,14 +1118,14 @@ genrule { cc_library_static { name: "foo_static3", - srcs: ["common.c", "not-for-*.c"], - exclude_srcs: ["not-for-everything.c"], + srcs: ["common.cpp", "not-for-*.cpp"], + exclude_srcs: ["not-for-everything.cpp"], generated_sources: ["generated_src", "generated_src_other_pkg"], generated_headers: ["generated_hdr", "generated_hdr_other_pkg"], arch: { x86: { - srcs: ["for-x86.c"], - exclude_srcs: ["not-for-x86.c"], + srcs: ["for-x86.cpp"], + exclude_srcs: ["not-for-x86.cpp"], 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.c", + "common.cpp", ] + select({ "//build/bazel/platforms/arch:x86": [ "//dep:generated_hdr_other_pkg_x86", ":generated_src_x86", - "for-x86.c", + "for-x86.cpp", ], - "//conditions:default": ["not-for-x86.c"], + "//conditions:default": ["not-for-x86.cpp"], }), )`}, }) @@ -1197,7 +1197,7 @@ cc_library_static { "//conditions:default": [], }), linkstatic = True, - srcs = ["common.c"], + srcs_c = ["common.c"], )`}, }) } diff --git a/cc/bp2build.go b/cc/bp2build.go index 0c827c5ed..e417c6907 100644 --- a/cc/bp2build.go +++ b/cc/bp2build.go @@ -183,15 +183,26 @@ func bp2BuildParseStaticProps(ctx android.TopDownMutatorContext, module *Module) // Convenience struct to hold all attributes parsed from compiler properties. type compilerAttributes struct { - copts bazel.StringListAttribute + // 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 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. @@ -215,15 +226,21 @@ func bp2BuildParseCompilerProps(ctx android.TopDownMutatorContext, module *Modul return append(includeDirs, baseCompilerProps.Local_include_dirs...) } - // Parse the list of copts. - parseCopts := func(baseCompilerProps *BaseCompilerProperties) []string { - var copts []string - for _, flag := range append(baseCompilerProps.Cflags, baseCompilerProps.Cppflags...) { + 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. - copts = append(copts, strings.Split(flag, " ")...) + 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 _, dir := range parseLocalIncludeDirs(baseCompilerProps) { copts = append(copts, includeFlags(dir)...) } @@ -260,6 +277,9 @@ 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 @@ -290,6 +310,9 @@ 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)) } } @@ -315,6 +338,9 @@ 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)) } } @@ -333,9 +359,28 @@ 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{ - srcs: srcs, - copts: copts, + copts: copts, + srcs: srcs, + asFlags: asFlags, + asSrcs: asSrcs, + cSrcs: cSrcs, + conlyFlags: conlyFlags, + cppFlags: cppFlags, } } diff --git a/cc/library.go b/cc/library.go index c918b96ba..9fb7a2451 100644 --- a/cc/library.go +++ b/cc/library.go @@ -230,6 +230,13 @@ 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 @@ -239,6 +246,7 @@ 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 @@ -294,20 +302,27 @@ 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, + 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, + 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, @@ -2230,6 +2245,12 @@ 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 { @@ -2259,6 +2280,12 @@ 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 704cb697d..cd711617b 100644 --- a/cc/object.go +++ b/cc/object.go @@ -185,8 +185,14 @@ 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: compilerAttrs.srcs, + Srcs: srcs, Deps: deps, Copts: compilerAttrs.copts, Asflags: asFlags,