From 74920cf6ee18d7f82cd27501719cac823313bdaf Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Tue, 4 Oct 2022 15:21:47 +0100 Subject: [PATCH 1/3] Document bootImageConfig and bootImageVariant structs Add warnings to the structs to explain how they are supposed to be used and deprecate fields that are used incorrectly. Bug: 245956352 Test: m nothing Change-Id: I090698287b96fd37102b88beb5d7252977bddc54 (cherry picked from commit a8df7e1c5bfce9115293ddd3bee9682c3c2c5728) Merged-In: I090698287b96fd37102b88beb5d7252977bddc54 --- java/dexpreopt_bootjars.go | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/java/dexpreopt_bootjars.go b/java/dexpreopt_bootjars.go index 2d6ee758c..d6f74b1b0 100644 --- a/java/dexpreopt_bootjars.go +++ b/java/dexpreopt_bootjars.go @@ -234,6 +234,11 @@ func init() { } // Target-independent description of a boot image. +// +// WARNING: All fields in this struct should be initialized in the genBootImageConfigs function. +// Failure to do so can lead to data races if there is no synchronization enforced ordering between +// the writer and the reader. Fields which break this rule are marked as deprecated and should be +// removed and replaced with something else, e.g. providers. type bootImageConfig struct { // If this image is an extension, the image that it extends. extends *bootImageConfig @@ -274,12 +279,18 @@ type bootImageConfig struct { zip android.WritablePath // Rules which should be used in make to install the outputs. + // + // Deprecated: Not initialized correctly, see struct comment. profileInstalls android.RuleBuilderInstalls // Path to the license metadata file for the module that built the profile. + // + // Deprecated: Not initialized correctly, see struct comment. profileLicenseMetadataFile android.OptionalPath // Path to the image profile file on host (or empty, if profile is not generated). + // + // Deprecated: Not initialized correctly, see struct comment. profilePathOnHost android.Path // Target-dependent fields. @@ -290,6 +301,8 @@ type bootImageConfig struct { } // Target-dependent description of a boot image. +// +// WARNING: The warning comment on bootImageConfig applies here too. type bootImageVariant struct { *bootImageConfig @@ -320,14 +333,28 @@ type bootImageVariant struct { primaryImagesDeps android.Paths // Rules which should be used in make to install the outputs on host. - installs android.RuleBuilderInstalls - vdexInstalls android.RuleBuilderInstalls + // + // Deprecated: Not initialized correctly, see struct comment. + installs android.RuleBuilderInstalls + + // Rules which should be used in make to install the vdex outputs on host. + // + // Deprecated: Not initialized correctly, see struct comment. + vdexInstalls android.RuleBuilderInstalls + + // Rules which should be used in make to install the unstripped outputs on host. + // + // Deprecated: Not initialized correctly, see struct comment. unstrippedInstalls android.RuleBuilderInstalls // Rules which should be used in make to install the outputs on device. + // + // Deprecated: Not initialized correctly, see struct comment. deviceInstalls android.RuleBuilderInstalls // Path to the license metadata file for the module that built the image. + // + // Deprecated: Not initialized correctly, see struct comment. licenseMetadataFile android.OptionalPath } From 5fe904240e32d7122543ac47a1ac9a5197533fd0 Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Tue, 4 Oct 2022 15:36:44 +0100 Subject: [PATCH 2/3] Remove profilePathOnHost from bootImageConfig The use of this field to return information from bootImageProfileRule up the call stack to one of the users resulted in data races being detected. This change simply returns the profile path back up the call stack. Bug: 245956352 Test: m nothing go test -race ./sdk/... -run TestSnapshotWithBootclasspathFragment_ImageName -test.count 100 # Run the previous command without this change and sometimes it # shows the data race around profilePathOnHost. With this change # that data race is not reported. Although there is still another # data race. Change-Id: I03b09e514cc94f2a6c9d5117d3b2f130cc2e4f5b (cherry picked from commit 9f6ac0bb42a8c229c6a1f85b10f4881b7cbe551f) Merged-In: I03b09e514cc94f2a6c9d5117d3b2f130cc2e4f5b --- java/bootclasspath_fragment.go | 45 ++++++++++++++++++---------------- java/dexpreopt_bootjars.go | 26 ++++++++++++-------- java/platform_bootclasspath.go | 4 +-- 3 files changed, 42 insertions(+), 33 deletions(-) diff --git a/java/bootclasspath_fragment.go b/java/bootclasspath_fragment.go index 841489e80..d9a85e668 100644 --- a/java/bootclasspath_fragment.go +++ b/java/bootclasspath_fragment.go @@ -264,7 +264,7 @@ type commonBootclasspathFragment interface { // // If it could not create the files then it will return nil. Otherwise, it will return a map from // android.ArchType to the predefined paths of the boot image files. - produceBootImageFiles(ctx android.ModuleContext, imageConfig *bootImageConfig) bootImageFilesByArch + produceBootImageFiles(ctx android.ModuleContext, imageConfig *bootImageConfig) bootImageOutputs } var _ commonBootclasspathFragment = (*BootclasspathFragmentModule)(nil) @@ -583,16 +583,16 @@ func (b *BootclasspathFragmentModule) GenerateAndroidBuildActions(ctx android.Mo // Perform hidden API processing. hiddenAPIOutput := b.generateHiddenAPIBuildActions(ctx, contents, fragments) - var bootImageFilesByArch bootImageFilesByArch + var bootImageFiles bootImageOutputs if imageConfig != nil { // Delegate the production of the boot image files to a module type specific method. common := ctx.Module().(commonBootclasspathFragment) - bootImageFilesByArch = common.produceBootImageFiles(ctx, imageConfig) + bootImageFiles = common.produceBootImageFiles(ctx, imageConfig) if shouldCopyBootFilesToPredefinedLocations(ctx, imageConfig) { // Zip the boot image files up, if available. This will generate the zip file in a // predefined location. - buildBootImageZipInPredefinedLocation(ctx, imageConfig, bootImageFilesByArch) + buildBootImageZipInPredefinedLocation(ctx, imageConfig, bootImageFiles.byArch) // Copy the dex jars of this fragment's content modules to their predefined locations. copyBootJarsToPredefinedLocations(ctx, hiddenAPIOutput.EncodedBootDexFilesByModule, imageConfig.dexPathsByModule) @@ -620,7 +620,7 @@ func (b *BootclasspathFragmentModule) GenerateAndroidBuildActions(ctx android.Mo // A prebuilt fragment cannot contribute to an apex. if !android.IsModulePrebuilt(ctx.Module()) { // Provide the apex content info. - b.provideApexContentInfo(ctx, imageConfig, hiddenAPIOutput, bootImageFilesByArch) + b.provideApexContentInfo(ctx, imageConfig, hiddenAPIOutput, bootImageFiles) } } else { // Versioned fragments are not needed by make. @@ -663,7 +663,7 @@ func shouldCopyBootFilesToPredefinedLocations(ctx android.ModuleContext, imageCo // provideApexContentInfo creates, initializes and stores the apex content info for use by other // modules. -func (b *BootclasspathFragmentModule) provideApexContentInfo(ctx android.ModuleContext, imageConfig *bootImageConfig, hiddenAPIOutput *HiddenAPIOutput, bootImageFilesByArch bootImageFilesByArch) { +func (b *BootclasspathFragmentModule) provideApexContentInfo(ctx android.ModuleContext, imageConfig *bootImageConfig, hiddenAPIOutput *HiddenAPIOutput, bootImageFiles bootImageOutputs) { // Construct the apex content info from the config. info := BootclasspathFragmentApexContentInfo{ // Populate the apex content info with paths to the dex jars. @@ -674,14 +674,14 @@ func (b *BootclasspathFragmentModule) provideApexContentInfo(ctx android.ModuleC info.modules = imageConfig.modules global := dexpreopt.GetGlobalConfig(ctx) if !global.DisableGenerateProfile { - info.profilePathOnHost = imageConfig.profilePathOnHost + info.profilePathOnHost = bootImageFiles.profile info.profileInstallPathInApex = imageConfig.profileInstallPathInApex } info.shouldInstallBootImageInApex = imageConfig.shouldInstallInApex() } - info.bootImageFilesByArch = bootImageFilesByArch + info.bootImageFilesByArch = bootImageFiles.byArch // Make the apex content info available for other modules. ctx.SetProvider(BootclasspathFragmentApexContentInfoProvider, info) @@ -936,9 +936,9 @@ func (b *BootclasspathFragmentModule) produceHiddenAPIOutput(ctx android.ModuleC } // produceBootImageFiles builds the boot image files from the source if it is required. -func (b *BootclasspathFragmentModule) produceBootImageFiles(ctx android.ModuleContext, imageConfig *bootImageConfig) bootImageFilesByArch { +func (b *BootclasspathFragmentModule) produceBootImageFiles(ctx android.ModuleContext, imageConfig *bootImageConfig) bootImageOutputs { if SkipDexpreoptBootJars(ctx) { - return nil + return bootImageOutputs{} } // Only generate the boot image if the configuration does not skip it. @@ -950,21 +950,21 @@ func (b *BootclasspathFragmentModule) produceBootImageFiles(ctx android.ModuleCo // // If it could not create the files then it will return nil. Otherwise, it will return a map from // android.ArchType to the predefined paths of the boot image files. -func (b *BootclasspathFragmentModule) generateBootImageBuildActions(ctx android.ModuleContext, imageConfig *bootImageConfig) bootImageFilesByArch { +func (b *BootclasspathFragmentModule) generateBootImageBuildActions(ctx android.ModuleContext, imageConfig *bootImageConfig) bootImageOutputs { global := dexpreopt.GetGlobalConfig(ctx) if !shouldBuildBootImages(ctx.Config(), global) { - return nil + return bootImageOutputs{} } // Bootclasspath fragment modules that are for the platform do not produce a boot image. apexInfo := ctx.Provider(android.ApexInfoProvider).(android.ApexInfo) if apexInfo.IsForPlatform() { - return nil + return bootImageOutputs{} } // Bootclasspath fragment modules that are versioned do not produce a boot image. if android.IsModuleInVersionedSdk(ctx.Module()) { - return nil + return bootImageOutputs{} } // Build a profile for the image config and then use that to build the boot image. @@ -974,11 +974,11 @@ func (b *BootclasspathFragmentModule) generateBootImageBuildActions(ctx android. buildBootImageVariantsForBuildOs(ctx, imageConfig, profile) // Build boot image files for the android variants. - androidBootImageFilesByArch := buildBootImageVariantsForAndroidOs(ctx, imageConfig, profile) + bootImageFiles := buildBootImageVariantsForAndroidOs(ctx, imageConfig, profile) // Return the boot image files for the android variants for inclusion in an APEX and to be zipped // up for the dist. - return androidBootImageFilesByArch + return bootImageFiles } func (b *BootclasspathFragmentModule) AndroidMkEntries() []android.AndroidMkEntries { @@ -1279,14 +1279,14 @@ func (module *PrebuiltBootclasspathFragmentModule) produceHiddenAPIOutput(ctx an } // produceBootImageFiles extracts the boot image files from the APEX if available. -func (module *PrebuiltBootclasspathFragmentModule) produceBootImageFiles(ctx android.ModuleContext, imageConfig *bootImageConfig) bootImageFilesByArch { +func (module *PrebuiltBootclasspathFragmentModule) produceBootImageFiles(ctx android.ModuleContext, imageConfig *bootImageConfig) bootImageOutputs { if !shouldCopyBootFilesToPredefinedLocations(ctx, imageConfig) { - return nil + return bootImageOutputs{} } di := android.FindDeapexerProviderForModule(ctx) if di == nil { - return nil // An error has been reported by FindDeapexerProviderForModule. + return bootImageOutputs{} // An error has been reported by FindDeapexerProviderForModule. } profile := (android.WritablePath)(nil) @@ -1323,11 +1323,14 @@ func (module *PrebuiltBootclasspathFragmentModule) produceBootImageFiles(ctx and }) } } - return files + return bootImageOutputs{ + files, + profile, + } } else { if profile == nil { ctx.ModuleErrorf("Unable to produce boot image files: neither boot image files nor profiles exists in the prebuilt apex") - return nil + return bootImageOutputs{} } // Build boot image files for the android variants from the dex files provided by the contents // of this module. diff --git a/java/dexpreopt_bootjars.go b/java/dexpreopt_bootjars.go index d6f74b1b0..744ae23ac 100644 --- a/java/dexpreopt_bootjars.go +++ b/java/dexpreopt_bootjars.go @@ -288,11 +288,6 @@ type bootImageConfig struct { // Deprecated: Not initialized correctly, see struct comment. profileLicenseMetadataFile android.OptionalPath - // Path to the image profile file on host (or empty, if profile is not generated). - // - // Deprecated: Not initialized correctly, see struct comment. - profilePathOnHost android.Path - // Target-dependent fields. variants []*bootImageVariant @@ -581,7 +576,7 @@ func copyBootJarsToPredefinedLocations(ctx android.ModuleContext, srcBootDexJars // boot image files. // // The paths are returned because they are needed elsewhere in Soong, e.g. for populating an APEX. -func buildBootImageVariantsForAndroidOs(ctx android.ModuleContext, image *bootImageConfig, profile android.WritablePath) bootImageFilesByArch { +func buildBootImageVariantsForAndroidOs(ctx android.ModuleContext, image *bootImageConfig, profile android.WritablePath) bootImageOutputs { return buildBootImageForOsType(ctx, image, profile, android.Android) } @@ -596,12 +591,22 @@ func buildBootImageVariantsForBuildOs(ctx android.ModuleContext, image *bootImag buildBootImageForOsType(ctx, image, profile, ctx.Config().BuildOS) } +// bootImageOutputs encapsulates information about boot images that were created/obtained by +// commonBootclasspathFragment.produceBootImageFiles. +type bootImageOutputs struct { + // Map from arch to the paths to the boot image files created/obtained for that arch. + byArch bootImageFilesByArch + + // The path to the profile file created/obtained for the boot image. + profile android.WritablePath +} + // buildBootImageForOsType takes a bootImageConfig, a profile file and an android.OsType // boot image files are required for and it creates rules to build the boot image // files for all the required architectures for them. // // It returns a map from android.ArchType to the predefined paths of the boot image files. -func buildBootImageForOsType(ctx android.ModuleContext, image *bootImageConfig, profile android.WritablePath, requiredOsType android.OsType) bootImageFilesByArch { +func buildBootImageForOsType(ctx android.ModuleContext, image *bootImageConfig, profile android.WritablePath, requiredOsType android.OsType) bootImageOutputs { filesByArch := bootImageFilesByArch{} for _, variant := range image.variants { if variant.target.Os == requiredOsType { @@ -610,7 +615,10 @@ func buildBootImageForOsType(ctx android.ModuleContext, image *bootImageConfig, } } - return filesByArch + return bootImageOutputs{ + filesByArch, + profile, + } } // buildBootImageZipInPredefinedLocation generates a zip file containing all the boot image files. @@ -855,8 +863,6 @@ func bootImageProfileRule(ctx android.ModuleContext, image *bootImageConfig) and rule.Build("bootJarsProfile", "profile boot jars") - image.profilePathOnHost = profile - return profile } diff --git a/java/platform_bootclasspath.go b/java/platform_bootclasspath.go index 24f8253ae..f0de7a4d8 100644 --- a/java/platform_bootclasspath.go +++ b/java/platform_bootclasspath.go @@ -436,10 +436,10 @@ func (b *platformBootclasspathModule) generateBootImageBuildActions(ctx android. profile := bootImageProfileRule(ctx, imageConfig) // Build boot image files for the android variants. - androidBootImageFilesByArch := buildBootImageVariantsForAndroidOs(ctx, imageConfig, profile) + androidBootImageFiles := buildBootImageVariantsForAndroidOs(ctx, imageConfig, profile) // Zip the android variant boot image files up. - buildBootImageZipInPredefinedLocation(ctx, imageConfig, androidBootImageFilesByArch) + buildBootImageZipInPredefinedLocation(ctx, imageConfig, androidBootImageFiles.byArch) // Build boot image files for the host variants. There are use directly by ART host side tests. buildBootImageVariantsForBuildOs(ctx, imageConfig, profile) From 9d8b46abab26f9f2449fb2bcf2f44abd3f5d603b Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Tue, 4 Oct 2022 16:39:18 +0100 Subject: [PATCH 3/3] Remove deviceInstalls from bootImageVariant The use of this field to return information from buildBootImageVariant up the call stack to one of the callers resulted in data races being detected. This change simply passes the deviceInstalls up the call stack. Bug: 245956352 Test: m nothing go test -race ./sdk/... -run TestSnapshotWithBootclasspathFragment_ImageName -test.count 100 # Run the previous command without this change and sometimes it # shows the data race around deviceInstalls. When run with this # change it reports no data races. Change-Id: I3c73920dcb17a6c89a63c6a9c3a0bb049a98a690 (cherry picked from commit e10a9f2e5e1b853e834debc692c3802540777533) Merged-In: I3c73920dcb17a6c89a63c6a9c3a0bb049a98a690 --- java/bootclasspath_fragment.go | 19 +++++++++++++------ java/dexpreopt_bootjars.go | 32 ++++++++++++++++++++------------ 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/java/bootclasspath_fragment.go b/java/bootclasspath_fragment.go index d9a85e668..6bd98d482 100644 --- a/java/bootclasspath_fragment.go +++ b/java/bootclasspath_fragment.go @@ -598,8 +598,9 @@ func (b *BootclasspathFragmentModule) GenerateAndroidBuildActions(ctx android.Mo copyBootJarsToPredefinedLocations(ctx, hiddenAPIOutput.EncodedBootDexFilesByModule, imageConfig.dexPathsByModule) } - for _, variant := range imageConfig.apexVariants() { - arch := variant.target.Arch.ArchType.String() + for _, variant := range bootImageFiles.variants { + archType := variant.config.target.Arch.ArchType + arch := archType.String() for _, install := range variant.deviceInstalls { // Remove the "/" prefix because the path should be relative to $ANDROID_PRODUCT_OUT. installDir := strings.TrimPrefix(filepath.Dir(install.To), "/") @@ -1304,8 +1305,17 @@ func (module *PrebuiltBootclasspathFragmentModule) produceBootImageFiles(ctx and // If the boot image files for the android variants are in the prebuilt apex, we must use those // rather than building new ones because those boot image files are going to be used on device. files := bootImageFilesByArch{} + bootImageFiles := bootImageOutputs{ + byArch: files, + profile: profile, + } for _, variant := range imageConfig.apexVariants() { arch := variant.target.Arch.ArchType + bootImageFiles.variants = append(bootImageFiles.variants, bootImageVariantOutputs{ + variant, + // No device installs needed when installed in APEX. + nil, + }) for _, toPath := range variant.imagesDeps { apexRelativePath := apexRootRelativePathToBootImageFile(arch, toPath.Base()) // Get the path to the file that the deapexer extracted from the prebuilt apex file. @@ -1323,10 +1333,7 @@ func (module *PrebuiltBootclasspathFragmentModule) produceBootImageFiles(ctx and }) } } - return bootImageOutputs{ - files, - profile, - } + return bootImageFiles } else { if profile == nil { ctx.ModuleErrorf("Unable to produce boot image files: neither boot image files nor profiles exists in the prebuilt apex") diff --git a/java/dexpreopt_bootjars.go b/java/dexpreopt_bootjars.go index 744ae23ac..323423d0a 100644 --- a/java/dexpreopt_bootjars.go +++ b/java/dexpreopt_bootjars.go @@ -342,11 +342,6 @@ type bootImageVariant struct { // Deprecated: Not initialized correctly, see struct comment. unstrippedInstalls android.RuleBuilderInstalls - // Rules which should be used in make to install the outputs on device. - // - // Deprecated: Not initialized correctly, see struct comment. - deviceInstalls android.RuleBuilderInstalls - // Path to the license metadata file for the module that built the image. // // Deprecated: Not initialized correctly, see struct comment. @@ -597,6 +592,8 @@ type bootImageOutputs struct { // Map from arch to the paths to the boot image files created/obtained for that arch. byArch bootImageFilesByArch + variants []bootImageVariantOutputs + // The path to the profile file created/obtained for the boot image. profile android.WritablePath } @@ -608,17 +605,19 @@ type bootImageOutputs struct { // It returns a map from android.ArchType to the predefined paths of the boot image files. func buildBootImageForOsType(ctx android.ModuleContext, image *bootImageConfig, profile android.WritablePath, requiredOsType android.OsType) bootImageOutputs { filesByArch := bootImageFilesByArch{} + imageOutputs := bootImageOutputs{ + byArch: filesByArch, + profile: profile, + } for _, variant := range image.variants { if variant.target.Os == requiredOsType { - buildBootImageVariant(ctx, variant, profile) + variantOutputs := buildBootImageVariant(ctx, variant, profile) + imageOutputs.variants = append(imageOutputs.variants, variantOutputs) filesByArch[variant.target.Arch.ArchType] = variant.imagesDeps.Paths() } } - return bootImageOutputs{ - filesByArch, - profile, - } + return imageOutputs } // buildBootImageZipInPredefinedLocation generates a zip file containing all the boot image files. @@ -646,8 +645,13 @@ func buildBootImageZipInPredefinedLocation(ctx android.ModuleContext, image *boo rule.Build("zip_"+image.name, "zip "+image.name+" image") } +type bootImageVariantOutputs struct { + config *bootImageVariant + deviceInstalls android.RuleBuilderInstalls +} + // Generate boot image build rules for a specific target. -func buildBootImageVariant(ctx android.ModuleContext, image *bootImageVariant, profile android.Path) { +func buildBootImageVariant(ctx android.ModuleContext, image *bootImageVariant, profile android.Path) bootImageVariantOutputs { globalSoong := dexpreopt.GetGlobalSoongConfig(ctx) global := dexpreopt.GetGlobalConfig(ctx) @@ -808,8 +812,12 @@ func buildBootImageVariant(ctx android.ModuleContext, image *bootImageVariant, p image.installs = rule.Installs() image.vdexInstalls = vdexInstalls image.unstrippedInstalls = unstrippedInstalls - image.deviceInstalls = deviceInstalls image.licenseMetadataFile = android.OptionalPathForPath(ctx.LicenseMetadataFile()) + + return bootImageVariantOutputs{ + image, + deviceInstalls, + } } const failureMessage = `ERROR: Dex2oat failed to compile a boot image.