From a5a29de67755083aa7159b4e51a49cbbb4919140 Mon Sep 17 00:00:00 2001 From: Liz Kammer Date: Wed, 25 May 2022 23:19:37 -0400 Subject: [PATCH] Do not hardcode default/experimental c{pp}std Prevent bugs like b/232866078 by using the same values across Bazel and Soong Test: bp2build.sh Change-Id: If257f9f5f8e8a70bbf3a8cf5479758c703c25c3f --- bp2build/cc_library_conversion_test.go | 123 +++++++++++++------------ cc/bp2build.go | 39 ++++---- cc/compiler.go | 6 +- cc/config/global.go | 5 + 4 files changed, 93 insertions(+), 80 deletions(-) diff --git a/bp2build/cc_library_conversion_test.go b/bp2build/cc_library_conversion_test.go index ab9298116..50ec14be3 100644 --- a/bp2build/cc_library_conversion_test.go +++ b/bp2build/cc_library_conversion_test.go @@ -1879,76 +1879,78 @@ func TestCcLibraryCppStdWithGnuExtensions_ConvertsToFeatureAttr(t *testing.T) { // not set, only emit if gnu_extensions is disabled. the default (gnu+17 // is set in the toolchain.) {cpp_std: "", gnu_extensions: "", bazel_cpp_std: ""}, - {cpp_std: "", gnu_extensions: "false", bazel_cpp_std: "c++17", bazel_c_std: "c99"}, + {cpp_std: "", gnu_extensions: "false", bazel_cpp_std: "cpp_std_default_no_gnu", bazel_c_std: "c_std_default_no_gnu"}, {cpp_std: "", gnu_extensions: "true", bazel_cpp_std: ""}, // experimental defaults to gnu++2a - {cpp_std: "experimental", gnu_extensions: "", bazel_cpp_std: "gnu++2a"}, - {cpp_std: "experimental", gnu_extensions: "false", bazel_cpp_std: "c++2a", bazel_c_std: "c99"}, - {cpp_std: "experimental", gnu_extensions: "true", bazel_cpp_std: "gnu++2a"}, + {cpp_std: "experimental", gnu_extensions: "", bazel_cpp_std: "cpp_std_experimental"}, + {cpp_std: "experimental", gnu_extensions: "false", bazel_cpp_std: "cpp_std_experimental_no_gnu", bazel_c_std: "c_std_default_no_gnu"}, + {cpp_std: "experimental", gnu_extensions: "true", bazel_cpp_std: "cpp_std_experimental"}, // Explicitly setting a c++ std does not use replace gnu++ std even if // gnu_extensions is true. // "c++11", {cpp_std: "c++11", gnu_extensions: "", bazel_cpp_std: "c++11"}, - {cpp_std: "c++11", gnu_extensions: "false", bazel_cpp_std: "c++11", bazel_c_std: "c99"}, + {cpp_std: "c++11", gnu_extensions: "false", bazel_cpp_std: "c++11", bazel_c_std: "c_std_default_no_gnu"}, {cpp_std: "c++11", gnu_extensions: "true", bazel_cpp_std: "c++11"}, // "c++17", {cpp_std: "c++17", gnu_extensions: "", bazel_cpp_std: "c++17"}, - {cpp_std: "c++17", gnu_extensions: "false", bazel_cpp_std: "c++17", bazel_c_std: "c99"}, + {cpp_std: "c++17", gnu_extensions: "false", bazel_cpp_std: "c++17", bazel_c_std: "c_std_default_no_gnu"}, {cpp_std: "c++17", gnu_extensions: "true", bazel_cpp_std: "c++17"}, // "c++2a", {cpp_std: "c++2a", gnu_extensions: "", bazel_cpp_std: "c++2a"}, - {cpp_std: "c++2a", gnu_extensions: "false", bazel_cpp_std: "c++2a", bazel_c_std: "c99"}, + {cpp_std: "c++2a", gnu_extensions: "false", bazel_cpp_std: "c++2a", bazel_c_std: "c_std_default_no_gnu"}, {cpp_std: "c++2a", gnu_extensions: "true", bazel_cpp_std: "c++2a"}, // "c++98", {cpp_std: "c++98", gnu_extensions: "", bazel_cpp_std: "c++98"}, - {cpp_std: "c++98", gnu_extensions: "false", bazel_cpp_std: "c++98", bazel_c_std: "c99"}, + {cpp_std: "c++98", gnu_extensions: "false", bazel_cpp_std: "c++98", bazel_c_std: "c_std_default_no_gnu"}, {cpp_std: "c++98", gnu_extensions: "true", bazel_cpp_std: "c++98"}, // gnu++ is replaced with c++ if gnu_extensions is explicitly false. // "gnu++11", {cpp_std: "gnu++11", gnu_extensions: "", bazel_cpp_std: "gnu++11"}, - {cpp_std: "gnu++11", gnu_extensions: "false", bazel_cpp_std: "c++11", bazel_c_std: "c99"}, + {cpp_std: "gnu++11", gnu_extensions: "false", bazel_cpp_std: "c++11", bazel_c_std: "c_std_default_no_gnu"}, {cpp_std: "gnu++11", gnu_extensions: "true", bazel_cpp_std: "gnu++11"}, // "gnu++17", {cpp_std: "gnu++17", gnu_extensions: "", bazel_cpp_std: "gnu++17"}, - {cpp_std: "gnu++17", gnu_extensions: "false", bazel_cpp_std: "c++17", bazel_c_std: "c99"}, + {cpp_std: "gnu++17", gnu_extensions: "false", bazel_cpp_std: "c++17", bazel_c_std: "c_std_default_no_gnu"}, {cpp_std: "gnu++17", gnu_extensions: "true", bazel_cpp_std: "gnu++17"}, // some c_std test cases - {c_std: "experimental", gnu_extensions: "", bazel_c_std: "gnu17"}, - {c_std: "experimental", gnu_extensions: "false", bazel_cpp_std: "c++17", bazel_c_std: "c17"}, - {c_std: "experimental", gnu_extensions: "true", bazel_c_std: "gnu17"}, + {c_std: "experimental", gnu_extensions: "", bazel_c_std: "c_std_experimental"}, + {c_std: "experimental", gnu_extensions: "false", bazel_cpp_std: "cpp_std_default_no_gnu", bazel_c_std: "c_std_experimental_no_gnu"}, + {c_std: "experimental", gnu_extensions: "true", bazel_c_std: "c_std_experimental"}, {c_std: "gnu11", cpp_std: "gnu++17", gnu_extensions: "", bazel_cpp_std: "gnu++17", bazel_c_std: "gnu11"}, {c_std: "gnu11", cpp_std: "gnu++17", gnu_extensions: "false", bazel_cpp_std: "c++17", bazel_c_std: "c11"}, {c_std: "gnu11", cpp_std: "gnu++17", gnu_extensions: "true", bazel_cpp_std: "gnu++17", bazel_c_std: "gnu11"}, } for i, tc := range testCases { - name_prefix := fmt.Sprintf("a_%v", i) - cppStdProp := "" - if tc.cpp_std != "" { - cppStdProp = fmt.Sprintf(" cpp_std: \"%s\",", tc.cpp_std) - } - cStdProp := "" - if tc.c_std != "" { - cStdProp = fmt.Sprintf(" c_std: \"%s\",", tc.c_std) - } - gnuExtensionsProp := "" - if tc.gnu_extensions != "" { - gnuExtensionsProp = fmt.Sprintf(" gnu_extensions: %s,", tc.gnu_extensions) - } - attrs := attrNameToString{} - if tc.bazel_cpp_std != "" { - attrs["cpp_std"] = fmt.Sprintf(`"%s"`, tc.bazel_cpp_std) - } - if tc.bazel_c_std != "" { - attrs["c_std"] = fmt.Sprintf(`"%s"`, tc.bazel_c_std) - } + name := fmt.Sprintf("cpp std: %q, c std: %q, gnu_extensions: %q", tc.cpp_std, tc.c_std, tc.gnu_extensions) + t.Run(name, func(t *testing.T) { + name_prefix := fmt.Sprintf("a_%v", i) + cppStdProp := "" + if tc.cpp_std != "" { + cppStdProp = fmt.Sprintf(" cpp_std: \"%s\",", tc.cpp_std) + } + cStdProp := "" + if tc.c_std != "" { + cStdProp = fmt.Sprintf(" c_std: \"%s\",", tc.c_std) + } + gnuExtensionsProp := "" + if tc.gnu_extensions != "" { + gnuExtensionsProp = fmt.Sprintf(" gnu_extensions: %s,", tc.gnu_extensions) + } + attrs := attrNameToString{} + if tc.bazel_cpp_std != "" { + attrs["cpp_std"] = fmt.Sprintf(`"%s"`, tc.bazel_cpp_std) + } + if tc.bazel_c_std != "" { + attrs["c_std"] = fmt.Sprintf(`"%s"`, tc.bazel_c_std) + } - runCcLibraryTestCase(t, bp2buildTestCase{ - description: fmt.Sprintf( - "cc_library with cpp_std: %s and gnu_extensions: %s", tc.cpp_std, tc.gnu_extensions), - moduleTypeUnderTest: "cc_library", - moduleTypeUnderTestFactory: cc.LibraryFactory, - blueprint: soongCcLibraryPreamble + fmt.Sprintf(` + runCcLibraryTestCase(t, bp2buildTestCase{ + description: fmt.Sprintf( + "cc_library with cpp_std: %s and gnu_extensions: %s", tc.cpp_std, tc.gnu_extensions), + moduleTypeUnderTest: "cc_library", + moduleTypeUnderTestFactory: cc.LibraryFactory, + blueprint: soongCcLibraryPreamble + fmt.Sprintf(` cc_library { name: "%s_full", %s // cpp_std: *string @@ -1957,15 +1959,15 @@ cc_library { include_build_directory: false, } `, name_prefix, cppStdProp, cStdProp, gnuExtensionsProp), - expectedBazelTargets: makeCcLibraryTargets(name_prefix+"_full", attrs), - }) + expectedBazelTargets: makeCcLibraryTargets(name_prefix+"_full", attrs), + }) - runCcLibraryStaticTestCase(t, bp2buildTestCase{ - description: fmt.Sprintf( - "cc_library_static with cpp_std: %s and gnu_extensions: %s", tc.cpp_std, tc.gnu_extensions), - moduleTypeUnderTest: "cc_library_static", - moduleTypeUnderTestFactory: cc.LibraryStaticFactory, - blueprint: soongCcLibraryPreamble + fmt.Sprintf(` + runCcLibraryStaticTestCase(t, bp2buildTestCase{ + description: fmt.Sprintf( + "cc_library_static with cpp_std: %s and gnu_extensions: %s", tc.cpp_std, tc.gnu_extensions), + moduleTypeUnderTest: "cc_library_static", + moduleTypeUnderTestFactory: cc.LibraryStaticFactory, + blueprint: soongCcLibraryPreamble + fmt.Sprintf(` cc_library_static { name: "%s_static", %s // cpp_std: *string @@ -1974,17 +1976,17 @@ cc_library_static { include_build_directory: false, } `, name_prefix, cppStdProp, cStdProp, gnuExtensionsProp), - expectedBazelTargets: []string{ - makeBazelTarget("cc_library_static", name_prefix+"_static", attrs), - }, - }) + expectedBazelTargets: []string{ + makeBazelTarget("cc_library_static", name_prefix+"_static", attrs), + }, + }) - runCcLibrarySharedTestCase(t, bp2buildTestCase{ - description: fmt.Sprintf( - "cc_library_shared with cpp_std: %s and gnu_extensions: %s", tc.cpp_std, tc.gnu_extensions), - moduleTypeUnderTest: "cc_library_shared", - moduleTypeUnderTestFactory: cc.LibrarySharedFactory, - blueprint: soongCcLibraryPreamble + fmt.Sprintf(` + runCcLibrarySharedTestCase(t, bp2buildTestCase{ + description: fmt.Sprintf( + "cc_library_shared with cpp_std: %s and gnu_extensions: %s", tc.cpp_std, tc.gnu_extensions), + moduleTypeUnderTest: "cc_library_shared", + moduleTypeUnderTestFactory: cc.LibrarySharedFactory, + blueprint: soongCcLibraryPreamble + fmt.Sprintf(` cc_library_shared { name: "%s_shared", %s // cpp_std: *string @@ -1993,9 +1995,10 @@ cc_library_shared { include_build_directory: false, } `, name_prefix, cppStdProp, cStdProp, gnuExtensionsProp), - expectedBazelTargets: []string{ - makeBazelTarget("cc_library_shared", name_prefix+"_shared", attrs), - }, + expectedBazelTargets: []string{ + makeBazelTarget("cc_library_shared", name_prefix+"_shared", attrs), + }, + }) }) } } diff --git a/cc/bp2build.go b/cc/bp2build.go index 4155aa326..ae77336b7 100644 --- a/cc/bp2build.go +++ b/cc/bp2build.go @@ -429,32 +429,33 @@ func parseSrcs(ctx android.BazelConversionPathContext, props *BaseCompilerProper return bazel.AppendBazelLabelLists(allSrcsLabelList, generatedSrcsLabelList), anySrcs } -func bp2buildResolveCppStdValue(c_std *string, cpp_std *string, gnu_extensions *bool) (*string, *string) { - var cStdVal, cppStdVal string +func bp2buildStdVal(std *string, prefix string, useGnu bool) *string { + defaultVal := prefix + "_std_default" // If c{,pp}std properties are not specified, don't generate them in the BUILD file. // Defaults are handled by the toolchain definition. // However, if gnu_extensions is false, then the default gnu-to-c version must be specified. - if cpp_std != nil { - cppStdVal = parseCppStd(cpp_std) - } else if gnu_extensions != nil && !*gnu_extensions { - cppStdVal = "c++17" - } - if c_std != nil { - cStdVal = parseCStd(c_std) - } else if gnu_extensions != nil && !*gnu_extensions { - cStdVal = "c99" + stdVal := proptools.StringDefault(std, defaultVal) + if stdVal == "experimental" || stdVal == defaultVal { + if stdVal == "experimental" { + stdVal = prefix + "_std_experimental" + } + if !useGnu { + stdVal += "_no_gnu" + } + } else if !useGnu { + stdVal = gnuToCReplacer.Replace(stdVal) } - cStdVal, cppStdVal = maybeReplaceGnuToC(gnu_extensions, cStdVal, cppStdVal) - var c_std_prop, cpp_std_prop *string - if cStdVal != "" { - c_std_prop = &cStdVal - } - if cppStdVal != "" { - cpp_std_prop = &cppStdVal + if stdVal == defaultVal { + return nil } + return &stdVal +} - return c_std_prop, cpp_std_prop +func bp2buildResolveCppStdValue(c_std *string, cpp_std *string, gnu_extensions *bool) (*string, *string) { + useGnu := useGnuExtensions(gnu_extensions) + + return bp2buildStdVal(c_std, "c", useGnu), bp2buildStdVal(cpp_std, "cpp", useGnu) } // packageFromLabel extracts package from a fully-qualified or relative Label and whether the label diff --git a/cc/compiler.go b/cc/compiler.go index 773a642e2..c7e9c9a95 100644 --- a/cc/compiler.go +++ b/cc/compiler.go @@ -295,8 +295,12 @@ func addToModuleList(ctx ModuleContext, key android.OnceKey, module string) { getNamedMapForConfig(ctx.Config(), key).Store(module, true) } +func useGnuExtensions(gnuExtensions *bool) bool { + return proptools.BoolDefault(gnuExtensions, true) +} + func maybeReplaceGnuToC(gnuExtensions *bool, cStd string, cppStd string) (string, string) { - if gnuExtensions != nil && *gnuExtensions == false { + if !useGnuExtensions(gnuExtensions) { cStd = gnuToCReplacer.Replace(cStd) cppStd = gnuToCReplacer.Replace(cppStd) } diff --git a/cc/config/global.go b/cc/config/global.go index 1c4ad7f6a..c1563baef 100644 --- a/cc/config/global.go +++ b/cc/config/global.go @@ -372,6 +372,11 @@ func init() { exportedVars.ExportStringListStaticVariable("CommonGlobalCppflags", commonGlobalCppflags) exportedVars.ExportStringListStaticVariable("ExternalCflags", extraExternalCflags) + exportedVars.ExportString("CStdVersion", CStdVersion) + exportedVars.ExportString("CppStdVersion", CppStdVersion) + exportedVars.ExportString("ExperimentalCStdVersion", ExperimentalCStdVersion) + exportedVars.ExportString("ExperimentalCppStdVersion", ExperimentalCppStdVersion) + // Everything in these lists is a crime against abstraction and dependency tracking. // Do not add anything to this list. commonGlobalIncludes := []string{