From ee2096237846c1cae2835c77af4204f1a6ec30a3 Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Wed, 6 May 2020 10:23:19 +0100 Subject: [PATCH 1/3] Detect invalid arch specific properties in snapshot Previously, the snapshot code did not know whether a specific property could be arch specific or not and assumed that they all were which meant that it could generate snapshots containing arch specific values for properties that are not arch specific and so would fail when unpacked. This change requires arch specific fields in SdkMemberProperties to be tagged as such using `android:"arch_variant"` (just as in module input property structures). Any property without that must have properties that are common across all variants. Bug: 155628860 Test: m nothing Change-Id: Ifc8116e11d987cfe7aec2eeaa964f3bbf36b5dc2 --- android/sdk.go | 8 +++++++- cc/library_sdk_member.go | 10 +++++----- java/java.go | 2 +- sdk/sdk_test.go | 33 ++++++++++++++++++++++++++++----- sdk/update.go | 32 +++++++++++++++++++++++++++++--- 5 files changed, 70 insertions(+), 15 deletions(-) diff --git a/android/sdk.go b/android/sdk.go index 873e08942..36afc3df2 100644 --- a/android/sdk.go +++ b/android/sdk.go @@ -342,9 +342,15 @@ type SdkMemberType interface { // // * The variant property structs are analysed to find exported (capitalized) fields which // have common values. Those fields are cleared and the common value added to the common - // properties. A field annotated with a tag of `sdk:"keep"` will be treated as if it + // properties. + // + // A field annotated with a tag of `sdk:"keep"` will be treated as if it // was not capitalized, i.e. not optimized for common values. // + // A field annotated with a tag of `android:"arch_variant"` will be allowed to have + // values that differ by arch, fields not tagged as such must have common values across + // all variants. + // // * The sdk module type populates the BpModule structure, creating the arch specific // structure and calls AddToPropertySet(...) on the properties struct to add the member // specific properties in the correct place in the structure. diff --git a/cc/library_sdk_member.go b/cc/library_sdk_member.go index 2c8e31158..730012c5b 100644 --- a/cc/library_sdk_member.go +++ b/cc/library_sdk_member.go @@ -307,7 +307,7 @@ type nativeLibInfoProperties struct { // The list of possibly common exported include dirs. // // This field is exported as its contents may not be arch specific. - ExportedIncludeDirs android.Paths + ExportedIncludeDirs android.Paths `android:"arch_variant"` // The list of arch specific exported generated include dirs. // @@ -322,23 +322,23 @@ type nativeLibInfoProperties struct { // The list of possibly common exported system include dirs. // // This field is exported as its contents may not be arch specific. - ExportedSystemIncludeDirs android.Paths + ExportedSystemIncludeDirs android.Paths `android:"arch_variant"` // The list of possibly common exported flags. // // This field is exported as its contents may not be arch specific. - ExportedFlags []string + ExportedFlags []string `android:"arch_variant"` // The set of shared libraries // // This field is exported as its contents may not be arch specific. - SharedLibs []string + SharedLibs []string `android:"arch_variant"` // The set of system shared libraries. Note nil and [] are semantically // distinct - see BaseLinkerProperties.System_shared_libs. // // This field is exported as its contents may not be arch specific. - SystemSharedLibs []string + SystemSharedLibs []string `android:"arch_variant"` // The specific stubs version for the lib variant, or empty string if stubs // are not in use. diff --git a/java/java.go b/java/java.go index 472d3dac4..9d75c74c7 100644 --- a/java/java.go +++ b/java/java.go @@ -1906,7 +1906,7 @@ func (mt *librarySdkMemberType) CreateVariantPropertiesStruct() android.SdkMembe type librarySdkMemberProperties struct { android.SdkMemberPropertiesBase - JarToExport android.Path + JarToExport android.Path `android:"arch_variant"` AidlIncludeDirs android.Paths } diff --git a/sdk/sdk_test.go b/sdk/sdk_test.go index 095f83607..898ecea68 100644 --- a/sdk/sdk_test.go +++ b/sdk/sdk_test.go @@ -226,8 +226,8 @@ func TestSDkInstall(t *testing.T) { } type EmbeddedPropertiesStruct struct { - S_Embedded_Common string - S_Embedded_Different string + S_Embedded_Common string `android:"arch_variant"` + S_Embedded_Different string `android:"arch_variant"` } type testPropertiesStruct struct { @@ -235,11 +235,11 @@ type testPropertiesStruct struct { private string Public_Kept string `sdk:"keep"` S_Common string - S_Different string + S_Different string `android:"arch_variant"` A_Common []string - A_Different []string + A_Different []string `android:"arch_variant"` F_Common *bool - F_Different *bool + F_Different *bool `android:"arch_variant"` EmbeddedPropertiesStruct } @@ -346,3 +346,26 @@ func TestCommonValueOptimization(t *testing.T) { }, structs[1]) } + +func TestCommonValueOptimization_InvalidArchSpecificVariants(t *testing.T) { + common := &testPropertiesStruct{name: "common"} + structs := []propertiesContainer{ + &testPropertiesStruct{ + name: "struct-0", + S_Common: "should-be-but-is-not-common0", + }, + &testPropertiesStruct{ + name: "struct-1", + S_Common: "should-be-but-is-not-common1", + }, + } + + extractor := newCommonValueExtractor(common) + + h := TestHelper{t} + + err := extractor.extractCommonProperties(common, structs) + h.AssertErrorMessageEquals("unexpected error", `field "S_Common" is not tagged as "arch_variant" but has arch specific properties: + "struct-0" has value "should-be-but-is-not-common0" + "struct-1" has value "should-be-but-is-not-common1"`, err) +} diff --git a/sdk/update.go b/sdk/update.go index 03a5c0379..bcc2c778b 100644 --- a/sdk/update.go +++ b/sdk/update.go @@ -1225,6 +1225,9 @@ type extractorProperty struct { // The empty value for the field. emptyValue reflect.Value + + // True if the property can support arch variants false otherwise. + archVariant bool } func (p extractorProperty) String() string { @@ -1303,6 +1306,7 @@ func (e *commonValueExtractor) gatherFields(structType reflect.Type, containingS name, fieldGetter, reflect.Zero(field.Type), + proptools.HasTag(field, "android", "arch_variant"), } e.properties = append(e.properties, property) } @@ -1370,10 +1374,16 @@ func (e *commonValueExtractor) extractCommonProperties(commonProperties interfac fieldGetter := property.getter // Check to see if all the structures have the same value for the field. The commonValue - // is nil on entry to the loop and if it is nil on exit then there is no common value, - // otherwise it points to the common value. + // is nil on entry to the loop and if it is nil on exit then there is no common value or + // all the values have been filtered out, otherwise it points to the common value. var commonValue *reflect.Value + // Assume that all the values will be the same. + // + // While similar to this is not quite the same as commonValue == nil. If all the values + // have been filtered out then this will be false but commonValue == nil will be true. + valuesDiffer := false + for i := 0; i < sliceValue.Len(); i++ { container := sliceValue.Index(i).Interface().(propertiesContainer) itemValue := reflect.ValueOf(container.optimizableProperties()) @@ -1387,12 +1397,13 @@ func (e *commonValueExtractor) extractCommonProperties(commonProperties interfac // no value in common so break out. if !reflect.DeepEqual(fieldValue.Interface(), commonValue.Interface()) { commonValue = nil + valuesDiffer = true break } } } - // If the fields all have a common value then store it in the common struct field + // If the fields all have common value then store it in the common struct field // and set the input struct's field to the empty value. if commonValue != nil { emptyValue := property.emptyValue @@ -1404,6 +1415,21 @@ func (e *commonValueExtractor) extractCommonProperties(commonProperties interfac fieldValue.Set(emptyValue) } } + + if valuesDiffer && !property.archVariant { + // The values differ but the property does not support arch variants so it + // is an error. + var details strings.Builder + for i := 0; i < sliceValue.Len(); i++ { + container := sliceValue.Index(i).Interface().(propertiesContainer) + itemValue := reflect.ValueOf(container.optimizableProperties()) + fieldValue := fieldGetter(itemValue) + + _, _ = fmt.Fprintf(&details, "\n %q has value %q", container.String(), fieldValue.Interface()) + } + + return fmt.Errorf("field %q is not tagged as \"arch_variant\" but has arch specific properties:%s", property.String(), details.String()) + } } return nil From 12f67bcf42b0bfcd87478f0c2c1e7c6d0def7989 Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Thu, 30 Apr 2020 18:08:29 +0100 Subject: [PATCH 2/3] Adds support for 'ignored-on-host' Adds a filter mechanism that can exclude property values from being included in the common value extraction. That is needed to prevent the snapshot mechanism from generating invalid output for properties that are ignored on host (and have their values cleared) and which are not tagged with `android:"arch_variant"`. Changes: * Updates the documentation of SdkMemberType to explain what effect the 'ignored-on-host' tag has. * Adds some tests for this new mechanism. Bug: 155628860 Test: m nothing Change-Id: I7ebd333079619dba546bc8c4911d567e0287b676 --- android/sdk.go | 9 +++++++++ sdk/sdk_test.go | 5 ++++- sdk/update.go | 45 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 1 deletion(-) diff --git a/android/sdk.go b/android/sdk.go index 36afc3df2..e823106e8 100644 --- a/android/sdk.go +++ b/android/sdk.go @@ -351,6 +351,15 @@ type SdkMemberType interface { // values that differ by arch, fields not tagged as such must have common values across // all variants. // + // * Additional field tags can be specified on a field that will ignore certain values + // for the purpose of common value optimization. A value that is ignored must have the + // default value for the property type. This is to ensure that significant value are not + // ignored by accident. The purpose of this is to allow the snapshot generation to reflect + // the behavior of the runtime. e.g. if a property is ignored on the host then a property + // that is common for android can be treated as if it was common for android and host as + // the setting for host is ignored anyway. + // * `sdk:"ignored-on-host" - this indicates the property is ignored on the host variant. + // // * The sdk module type populates the BpModule structure, creating the arch specific // structure and calls AddToPropertySet(...) on the properties struct to add the member // specific properties in the correct place in the structure. diff --git a/sdk/sdk_test.go b/sdk/sdk_test.go index 898ecea68..ae1a4923a 100644 --- a/sdk/sdk_test.go +++ b/sdk/sdk_test.go @@ -289,9 +289,12 @@ func TestCommonValueOptimization(t *testing.T) { } extractor := newCommonValueExtractor(common) - extractor.extractCommonProperties(common, structs) h := TestHelper{t} + + err := extractor.extractCommonProperties(common, structs) + h.AssertDeepEquals("unexpected error", nil, err) + h.AssertDeepEquals("common properties not correct", &testPropertiesStruct{ name: "common", diff --git a/sdk/update.go b/sdk/update.go index bcc2c778b..991428eec 100644 --- a/sdk/update.go +++ b/sdk/update.go @@ -1215,11 +1215,26 @@ func (s *sdk) getPossibleOsTypes() []android.OsType { // struct (or one of its embedded structs). type fieldAccessorFunc func(structValue reflect.Value) reflect.Value +// Checks the metadata to determine whether the property should be ignored for the +// purposes of common value extraction or not. +type extractorMetadataPredicate func(metadata propertiesContainer) bool + +// Indicates whether optimizable properties are provided by a host variant or +// not. +type isHostVariant interface { + isHostVariant() bool +} + // A property that can be optimized by the commonValueExtractor. type extractorProperty struct { // The name of the field for this property. name string + // Filter that can use metadata associated with the properties being optimized + // to determine whether the field should be ignored during common value + // optimization. + filter extractorMetadataPredicate + // Retrieves the value on which common value optimization will be performed. getter fieldAccessorFunc @@ -1273,6 +1288,20 @@ func (e *commonValueExtractor) gatherFields(structType reflect.Type, containingS continue } + var filter extractorMetadataPredicate + + // Add a filter + if proptools.HasTag(field, "sdk", "ignored-on-host") { + filter = func(metadata propertiesContainer) bool { + if m, ok := metadata.(isHostVariant); ok { + if m.isHostVariant() { + return false + } + } + return true + } + } + // Save a copy of the field index for use in the function. fieldIndex := f @@ -1304,6 +1333,7 @@ func (e *commonValueExtractor) gatherFields(structType reflect.Type, containingS } else { property := extractorProperty{ name, + filter, fieldGetter, reflect.Zero(field.Type), proptools.HasTag(field, "android", "arch_variant"), @@ -1372,6 +1402,12 @@ func (e *commonValueExtractor) extractCommonProperties(commonProperties interfac for _, property := range e.properties { fieldGetter := property.getter + filter := property.filter + if filter == nil { + filter = func(metadata propertiesContainer) bool { + return true + } + } // Check to see if all the structures have the same value for the field. The commonValue // is nil on entry to the loop and if it is nil on exit then there is no common value or @@ -1389,6 +1425,15 @@ func (e *commonValueExtractor) extractCommonProperties(commonProperties interfac itemValue := reflect.ValueOf(container.optimizableProperties()) fieldValue := fieldGetter(itemValue) + if !filter(container) { + expectedValue := property.emptyValue.Interface() + actualValue := fieldValue.Interface() + if !reflect.DeepEqual(expectedValue, actualValue) { + return fmt.Errorf("field %q is supposed to be ignored for %q but is set to %#v instead of %#v", property, container, actualValue, expectedValue) + } + continue + } + if commonValue == nil { // Use the first value as the commonProperties value. commonValue = &fieldValue From 2af52380be107029ac4d8fbc25c34669fbfce536 Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Mon, 4 May 2020 15:32:08 +0100 Subject: [PATCH 3/3] Fix snapshot of a host/device cc_library with stubs Adds a test that fails with unknown property android.stubs.versions and then fixes that by marking the field from which that property is created with 'ignored-on-host' and implemented the isHostVariant on *osTypeSpecificInfo. Bug: 155628860 Test: m nothing Change-Id: I167b47a1374f541aa09d7e045972d740f1d9009c --- cc/library_sdk_member.go | 6 ++- sdk/cc_sdk_test.go | 83 ++++++++++++++++++++++++++++++++++++++++ sdk/update.go | 7 ++++ 3 files changed, 95 insertions(+), 1 deletion(-) diff --git a/cc/library_sdk_member.go b/cc/library_sdk_member.go index 730012c5b..a7a1de251 100644 --- a/cc/library_sdk_member.go +++ b/cc/library_sdk_member.go @@ -342,7 +342,11 @@ type nativeLibInfoProperties struct { // The specific stubs version for the lib variant, or empty string if stubs // are not in use. - StubsVersion string + // + // Marked 'ignored-on-host' as the StubsVersion() from which this is initialized is + // not set on host and the stubs.versions property which this is written to is does + // not vary by arch so cannot be android specific. + StubsVersion string `sdk:"ignored-on-host"` // outputFile is not exported as it is always arch specific. outputFile android.Path diff --git a/sdk/cc_sdk_test.go b/sdk/cc_sdk_test.go index 733f7ac22..b77447aea 100644 --- a/sdk/cc_sdk_test.go +++ b/sdk/cc_sdk_test.go @@ -1805,3 +1805,86 @@ sdk_snapshot { } `)) } + +func TestDeviceAndHostSnapshotWithStubsLibrary(t *testing.T) { + result := testSdkWithCc(t, ` + sdk { + name: "mysdk", + host_supported: true, + native_shared_libs: ["stubslib"], + } + + cc_library { + name: "internaldep", + host_supported: true, + } + + cc_library { + name: "stubslib", + host_supported: true, + shared_libs: ["internaldep"], + stubs: { + symbol_file: "some/where/stubslib.map.txt", + versions: ["1", "2", "3"], + }, + } + `) + + result.CheckSnapshot("mysdk", "", + checkAndroidBpContents(` +// This is auto-generated. DO NOT EDIT. + +cc_prebuilt_library_shared { + name: "mysdk_stubslib@current", + sdk_member_name: "stubslib", + host_supported: true, + installable: false, + stubs: { + versions: ["3"], + }, + target: { + android_arm64: { + srcs: ["android/arm64/lib/stubslib.so"], + }, + android_arm: { + srcs: ["android/arm/lib/stubslib.so"], + }, + linux_glibc_x86_64: { + srcs: ["linux_glibc/x86_64/lib/stubslib.so"], + }, + linux_glibc_x86: { + srcs: ["linux_glibc/x86/lib/stubslib.so"], + }, + }, +} + +cc_prebuilt_library_shared { + name: "stubslib", + prefer: false, + host_supported: true, + stubs: { + versions: ["3"], + }, + target: { + android_arm64: { + srcs: ["android/arm64/lib/stubslib.so"], + }, + android_arm: { + srcs: ["android/arm/lib/stubslib.so"], + }, + linux_glibc_x86_64: { + srcs: ["linux_glibc/x86_64/lib/stubslib.so"], + }, + linux_glibc_x86: { + srcs: ["linux_glibc/x86/lib/stubslib.so"], + }, + }, +} + +sdk_snapshot { + name: "mysdk@current", + host_supported: true, + native_shared_libs: ["mysdk_stubslib@current"], +} +`)) +} diff --git a/sdk/update.go b/sdk/update.go index 991428eec..d43a42d6e 100644 --- a/sdk/update.go +++ b/sdk/update.go @@ -982,6 +982,13 @@ func (osInfo *osTypeSpecificInfo) addToPropertySet(ctx *memberContext, bpModule } } +func (osInfo *osTypeSpecificInfo) isHostVariant() bool { + osClass := osInfo.osType.Class + return osClass == android.Host || osClass == android.HostCross +} + +var _ isHostVariant = (*osTypeSpecificInfo)(nil) + func (osInfo *osTypeSpecificInfo) String() string { return fmt.Sprintf("OsType{%s}", osInfo.osType) }