From 85d55c28478ded14468fa1b43e50b9df4baa645c Mon Sep 17 00:00:00 2001 From: Trevor Radcliffe Date: Thu, 21 Sep 2023 17:04:43 +0000 Subject: [PATCH] Revert "Block CFI on static libraries" This reverts commit f9abec0987feb81132201179217ad0ea66ae4074. Reason for revert: https://b.corp.google.com/issues/301444094 https://b.corp.google.com/issues/301443813 https://b.corp.google.com/issues/301437374 Change-Id: I6fd03e4d3c0930005178ad347f53156be8f15efc --- bazel/properties.go | 36 ------------------- bp2build/cc_library_conversion_test.go | 10 +++++- bp2build/cc_library_shared_conversion_test.go | 4 +-- bp2build/cc_library_static_conversion_test.go | 18 +++++++--- cc/Android.bp | 1 - cc/bp2build.go | 4 +-- cc/constants.go | 19 ---------- cc/library.go | 4 --- 8 files changed, 26 insertions(+), 70 deletions(-) delete mode 100644 cc/constants.go diff --git a/bazel/properties.go b/bazel/properties.go index 29e44afb3..9c63bc04b 100644 --- a/bazel/properties.go +++ b/bazel/properties.go @@ -1296,42 +1296,6 @@ func (sla StringListAttribute) IsEmpty() bool { return len(sla.Value) == 0 && !sla.HasConfigurableValues() } -// RemoveFromAllConfigs removes all instances of the specified value from all configurations -// of the givenStringListAttribute -func (sla *StringListAttribute) RemoveFromAllConfigs(toRemove string) { - if removed, removalResult := removeFromList(toRemove, sla.Value); removed { - if len(removalResult) > 0 { - sla.Value = removalResult - } else { - sla.Value = nil - } - } - for axis, slsv := range sla.ConfigurableValues { - for config, sl := range slsv { - if removed, removalResult := removeFromList(toRemove, sl); removed { - if len(removalResult) > 0 { - sla.SetSelectValue(axis, config, removalResult) - } else { - sla.SetSelectValue(axis, config, nil) - } - } - } - } -} - -func removeFromList(s string, list []string) (bool, []string) { - result := make([]string, 0, len(list)) - var removed bool - for _, item := range list { - if item != s { - result = append(result, item) - } else { - removed = true - } - } - return removed, result -} - type configurableStringLists map[ConfigurationAxis]stringListSelectValues func (csl configurableStringLists) Append(other configurableStringLists) { diff --git a/bp2build/cc_library_conversion_test.go b/bp2build/cc_library_conversion_test.go index 82b9e4ab8..28dbf7ead 100644 --- a/bp2build/cc_library_conversion_test.go +++ b/bp2build/cc_library_conversion_test.go @@ -4785,6 +4785,7 @@ cc_library { }`, ExpectedBazelTargets: []string{ MakeBazelTarget("cc_library_static", "foo_bp2build_cc_library_static", AttrNameToString{ + "features": `["android_cfi"]`, "local_includes": `["."]`, }), MakeBazelTarget("cc_library_shared", "foo", AttrNameToString{ @@ -4813,6 +4814,10 @@ cc_library { }`, ExpectedBazelTargets: []string{ MakeBazelTarget("cc_library_static", "foo_bp2build_cc_library_static", AttrNameToString{ + "features": `select({ + "//build/bazel/platforms/os:android": ["android_cfi"], + "//conditions:default": [], + })`, "local_includes": `["."]`, }), MakeBazelTarget("cc_library_shared", "foo", AttrNameToString{ @@ -4843,7 +4848,10 @@ cc_library { }`, ExpectedBazelTargets: []string{ MakeBazelTarget("cc_library_static", "foo_bp2build_cc_library_static", AttrNameToString{ - "features": `["android_cfi_assembly_support"]`, + "features": `[ + "android_cfi", + "android_cfi_assembly_support", + ]`, "local_includes": `["."]`, }), MakeBazelTarget("cc_library_shared", "foo", AttrNameToString{ diff --git a/bp2build/cc_library_shared_conversion_test.go b/bp2build/cc_library_shared_conversion_test.go index b9df2e786..90b13b03f 100644 --- a/bp2build/cc_library_shared_conversion_test.go +++ b/bp2build/cc_library_shared_conversion_test.go @@ -1522,7 +1522,7 @@ func TestCcLibrarySharedWithCfiAndCfiAssemblySupport(t *testing.T) { runCcLibrarySharedTestCase(t, Bp2buildTestCase{ Description: "cc_library_shared has correct features when cfi is enabled with cfi assembly support", Blueprint: ` -cc_library_shared { +cc_library_static { name: "foo", sanitize: { cfi: true, @@ -1532,7 +1532,7 @@ cc_library_shared { }, }`, ExpectedBazelTargets: []string{ - MakeBazelTarget("cc_library_shared", "foo", AttrNameToString{ + MakeBazelTarget("cc_library_static", "foo", AttrNameToString{ "features": `[ "android_cfi", "android_cfi_assembly_support", diff --git a/bp2build/cc_library_static_conversion_test.go b/bp2build/cc_library_static_conversion_test.go index e7a75b42e..89ec8f9a7 100644 --- a/bp2build/cc_library_static_conversion_test.go +++ b/bp2build/cc_library_static_conversion_test.go @@ -2135,9 +2135,9 @@ cc_library_static { }) } -func TestCcLibraryStaticNoCfi(t *testing.T) { +func TestCcLibraryStaticWithCfi(t *testing.T) { runCcLibraryStaticTestCase(t, Bp2buildTestCase{ - Description: "cc_library_static never explicitly enables CFI", + Description: "cc_library_static has correct features when cfi is enabled", Blueprint: ` cc_library_static { name: "foo", @@ -2147,6 +2147,7 @@ cc_library_static { }`, ExpectedBazelTargets: []string{ MakeBazelTarget("cc_library_static", "foo", AttrNameToString{ + "features": `["android_cfi"]`, "local_includes": `["."]`, }), }, @@ -2155,7 +2156,7 @@ cc_library_static { func TestCcLibraryStaticWithCfiOsSpecific(t *testing.T) { runCcLibraryStaticTestCase(t, Bp2buildTestCase{ - Description: "cc_library_static never explicitly enables CFI even for specific variants", + Description: "cc_library_static has correct features when cfi is enabled for specific variants", Blueprint: ` cc_library_static { name: "foo", @@ -2169,6 +2170,10 @@ cc_library_static { }`, ExpectedBazelTargets: []string{ MakeBazelTarget("cc_library_static", "foo", AttrNameToString{ + "features": `select({ + "//build/bazel/platforms/os:android": ["android_cfi"], + "//conditions:default": [], + })`, "local_includes": `["."]`, }), }, @@ -2177,7 +2182,7 @@ cc_library_static { func TestCcLibraryStaticWithCfiAndCfiAssemblySupport(t *testing.T) { runCcLibraryStaticTestCase(t, Bp2buildTestCase{ - Description: "cc_library_static will specify cfi_assembly_support feature but not cfi feature", + Description: "cc_library_static has correct features when cfi is enabled with cfi_assembly_support", Blueprint: ` cc_library_static { name: "foo", @@ -2190,7 +2195,10 @@ cc_library_static { }`, ExpectedBazelTargets: []string{ MakeBazelTarget("cc_library_static", "foo", AttrNameToString{ - "features": `["android_cfi_assembly_support"]`, + "features": `[ + "android_cfi", + "android_cfi_assembly_support", + ]`, "local_includes": `["."]`, }), }, diff --git a/cc/Android.bp b/cc/Android.bp index 8f6709f90..c32d85490 100644 --- a/cc/Android.bp +++ b/cc/Android.bp @@ -30,7 +30,6 @@ bootstrap_go_package { "cc.go", "ccdeps.go", "check.go", - "constants.go", "coverage.go", "gen.go", "generated_cc_library.go", diff --git a/cc/bp2build.go b/cc/bp2build.go index bbdf67284..7f78e283d 100644 --- a/cc/bp2build.go +++ b/cc/bp2build.go @@ -1953,9 +1953,9 @@ func bp2buildSanitizerFeatures(ctx android.BazelConversionPathContext, m *Module sanitizerCompilerInputs.SetSelectValue(bazel.SanitizersEnabledAxis, bazel.SanitizersEnabled, bazel.MakeLabelListFromTargetNames([]string{*blocklist})) } if sanitizerProps.Sanitize.Cfi != nil && !proptools.Bool(sanitizerProps.Sanitize.Cfi) { - features = append(features, "-"+cfiFeatureName) + features = append(features, "-android_cfi") } else if proptools.Bool(sanitizerProps.Sanitize.Cfi) { - features = append(features, cfiFeatureName) + features = append(features, "android_cfi") if proptools.Bool(sanitizerProps.Sanitize.Config.Cfi_assembly_support) { features = append(features, "android_cfi_assembly_support") } diff --git a/cc/constants.go b/cc/constants.go deleted file mode 100644 index f188d4e16..000000000 --- a/cc/constants.go +++ /dev/null @@ -1,19 +0,0 @@ -// Copyright 2016 Google Inc. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package cc - -var ( - cfiFeatureName = "android_cfi" -) diff --git a/cc/library.go b/cc/library.go index c5a922291..2d4d60440 100644 --- a/cc/library.go +++ b/cc/library.go @@ -331,7 +331,6 @@ func libraryBp2Build(ctx android.TopDownMutatorContext, m *Module) { sharedFeatures.DeduplicateAxesFromBase() staticFeatures := baseAttributes.features.Clone().Append(staticAttrs.Features) staticFeatures.DeduplicateAxesFromBase() - staticFeatures.RemoveFromAllConfigs(cfiFeatureName) staticCommonAttrs := staticOrSharedAttributes{ Srcs: *srcs.Clone().Append(staticAttrs.Srcs), @@ -2947,9 +2946,6 @@ func sharedOrStaticLibraryBp2Build(ctx android.TopDownMutatorContext, module *Mo features := baseAttributes.features.Clone().Append(libSharedOrStaticAttrs.Features) features.DeduplicateAxesFromBase() - if isStatic { - features.RemoveFromAllConfigs(cfiFeatureName) - } commonAttrs := staticOrSharedAttributes{ Srcs: compilerAttrs.srcs,