From c1bf72420725567ab4d5af5e9cadb3546ef74542 Mon Sep 17 00:00:00 2001 From: Nicolas Geoffray Date: Fri, 18 Oct 2019 14:51:38 +0100 Subject: [PATCH] Remove support for stripping dex. Stripping is incompatible with ART module updatability. Bug: 65154345 Bug: 138851227 Test: build and observe no change in output (stripping is not used by default). Change-Id: I4bbc01d9979605b7dd2f73ee99a74c1e817c3e8c --- dexpreopt/config.go | 15 +--- dexpreopt/dexpreopt.go | 90 +----------------------- dexpreopt/dexpreopt_gen/dexpreopt_gen.go | 25 +------ dexpreopt/dexpreopt_test.go | 72 ------------------- java/app_test.go | 8 +-- java/dexpreopt.go | 22 +----- 6 files changed, 8 insertions(+), 224 deletions(-) diff --git a/dexpreopt/config.go b/dexpreopt/config.go index f39ec4aa3..9215eff3f 100644 --- a/dexpreopt/config.go +++ b/dexpreopt/config.go @@ -24,8 +24,6 @@ import ( // GlobalConfig stores the configuration for dex preopting set by the product type GlobalConfig struct { - DefaultNoStripping bool // don't strip dex files by default - DisablePreopt bool // disable preopt for all modules DisablePreoptModules []string // modules with preopt disabled by product-specific config @@ -55,8 +53,7 @@ type GlobalConfig struct { DefaultCompilerFilter string // default compiler filter to pass to dex2oat, overridden by --compiler-filter= in module-specific dex2oat flags SystemServerCompilerFilter string // default compiler filter to pass to dex2oat for system server jars - GenerateDMFiles bool // generate Dex Metadata files - NeverAllowStripping bool // whether stripping should not be done - used as build time check to make sure dex files are always available + GenerateDMFiles bool // generate Dex Metadata files NoDebugInfo bool // don't generate debug info by default DontResolveStartupStrings bool // don't resolve string literals loaded during application startup. @@ -133,10 +130,6 @@ type ModuleConfig struct { ForceCreateAppImage bool PresignedPrebuilt bool - - NoStripping bool - StripInputPath android.Path - StripOutputPath android.WritablePath } func constructPath(ctx android.PathContext, path string) android.Path { @@ -233,8 +226,6 @@ func LoadModuleConfig(ctx android.PathContext, path string) (ModuleConfig, error LibraryPaths map[string]string DexPreoptImages []string PreoptBootClassPathDexFiles []string - StripInputPath string - StripOutputPath string } config := ModuleJSONConfig{} @@ -252,8 +243,6 @@ func LoadModuleConfig(ctx android.PathContext, path string) (ModuleConfig, error config.ModuleConfig.LibraryPaths = constructPathMap(ctx, config.LibraryPaths) config.ModuleConfig.DexPreoptImages = constructPaths(ctx, config.DexPreoptImages) config.ModuleConfig.PreoptBootClassPathDexFiles = constructPaths(ctx, config.PreoptBootClassPathDexFiles) - config.ModuleConfig.StripInputPath = constructPath(ctx, config.StripInputPath) - config.ModuleConfig.StripOutputPath = constructWritablePath(ctx, config.StripOutputPath) // This needs to exist, but dependencies are already handled in Make, so we don't need to pass them through JSON. config.ModuleConfig.DexPreoptImagesDeps = make([]android.Paths, len(config.ModuleConfig.DexPreoptImages)) @@ -283,7 +272,6 @@ func loadConfig(ctx android.PathContext, path string, config interface{}) ([]byt func GlobalConfigForTests(ctx android.PathContext) GlobalConfig { return GlobalConfig{ - DefaultNoStripping: false, DisablePreopt: false, DisablePreoptModules: nil, OnlyPreoptBootImageAndSystemServer: false, @@ -302,7 +290,6 @@ func GlobalConfigForTests(ctx android.PathContext) GlobalConfig { DefaultCompilerFilter: "", SystemServerCompilerFilter: "", GenerateDMFiles: false, - NeverAllowStripping: false, NoDebugInfo: false, DontResolveStartupStrings: false, AlwaysSystemServerDebugInfo: false, diff --git a/dexpreopt/dexpreopt.go b/dexpreopt/dexpreopt.go index 40644a35e..6f9215392 100644 --- a/dexpreopt/dexpreopt.go +++ b/dexpreopt/dexpreopt.go @@ -13,7 +13,7 @@ // limitations under the License. // The dexpreopt package converts a global dexpreopt config and a module dexpreopt config into rules to perform -// dexpreopting and to strip the dex files from the APK or JAR. +// dexpreopting. // // It is used in two places; in the dexpeopt_gen binary for modules defined in Make, and directly linked into Soong. // @@ -22,8 +22,7 @@ // changed. One script takes an APK or JAR as an input and produces a zip file containing any outputs of preopting, // in the location they should be on the device. The Make build rules will unzip the zip file into $(PRODUCT_OUT) when // installing the APK, which will install the preopt outputs into $(PRODUCT_OUT)/system or $(PRODUCT_OUT)/system_other -// as necessary. The zip file may be empty if preopting was disabled for any reason. The second script takes an APK or -// JAR as an input and strips the dex files in it as necessary. +// as necessary. The zip file may be empty if preopting was disabled for any reason. // // The intermediate shell scripts allow changes to this package or to the global config to regenerate the shell scripts // but only require re-executing preopting if the script has changed. @@ -48,45 +47,6 @@ import ( const SystemPartition = "/system/" const SystemOtherPartition = "/system_other/" -// GenerateStripRule generates a set of commands that will take an APK or JAR as an input and strip the dex files if -// they are no longer necessary after preopting. -func GenerateStripRule(global GlobalConfig, module ModuleConfig) (rule *android.RuleBuilder, err error) { - defer func() { - if r := recover(); r != nil { - if _, ok := r.(runtime.Error); ok { - panic(r) - } else if e, ok := r.(error); ok { - err = e - rule = nil - } else { - panic(r) - } - } - }() - - tools := global.Tools - - rule = android.NewRuleBuilder() - - strip := shouldStripDex(module, global) - - if strip { - if global.NeverAllowStripping { - panic(fmt.Errorf("Stripping requested on %q, though the product does not allow it", module.DexLocation)) - } - // Only strips if the dex files are not already uncompressed - rule.Command(). - Textf(`if (zipinfo %s '*.dex' 2>/dev/null | grep -v ' stor ' >/dev/null) ; then`, module.StripInputPath). - Tool(tools.Zip2zip).FlagWithInput("-i ", module.StripInputPath).FlagWithOutput("-o ", module.StripOutputPath). - FlagWithArg("-x ", `"classes*.dex"`). - Textf(`; else cp -f %s %s; fi`, module.StripInputPath, module.StripOutputPath) - } else { - rule.Command().Text("cp -f").Input(module.StripInputPath).Output(module.StripOutputPath) - } - - return rule, nil -} - // GenerateDexpreoptRule generates a set of commands that will preopt a module based on a GlobalConfig and a // ModuleConfig. The produced files and their install locations will be available through rule.Installs(). func GenerateDexpreoptRule(ctx android.PathContext, @@ -120,7 +80,6 @@ func GenerateDexpreoptRule(ctx android.PathContext, if !dexpreoptDisabled(global, module) { // Don't preopt individual boot jars, they will be preopted together. - // This check is outside dexpreoptDisabled because they still need to be stripped. if !contains(global.BootJars, module.Name) { appImage := (generateProfile || module.ForceCreateAppImage || global.DefaultAppImages) && !module.NoCreateAppImage @@ -515,51 +474,6 @@ func dexpreoptCommand(ctx android.PathContext, global GlobalConfig, module Modul rule.Install(vdexPath, vdexInstallPath) } -// Return if the dex file in the APK should be stripped. If an APK is found to contain uncompressed dex files at -// dex2oat time it will not be stripped even if strip=true. -func shouldStripDex(module ModuleConfig, global GlobalConfig) bool { - strip := !global.DefaultNoStripping - - if dexpreoptDisabled(global, module) { - strip = false - } - - if module.NoStripping { - strip = false - } - - // Don't strip modules that are not on the system partition in case the oat/vdex version in system ROM - // doesn't match the one in other partitions. It needs to be able to fall back to the APK for that case. - if !strings.HasPrefix(module.DexLocation, SystemPartition) { - strip = false - } - - // system_other isn't there for an OTA, so don't strip if module is on system, and odex is on system_other. - if odexOnSystemOther(module, global) { - strip = false - } - - if module.HasApkLibraries { - strip = false - } - - // Don't strip with dex files we explicitly uncompress (dexopt will not store the dex code). - if module.UncompressedDex { - strip = false - } - - if shouldGenerateDM(module, global) { - strip = false - } - - if module.PresignedPrebuilt { - // Only strip out files if we can re-sign the package. - strip = false - } - - return strip -} - func shouldGenerateDM(module ModuleConfig, global GlobalConfig) bool { // Generating DM files only makes sense for verify, avoid doing for non verify compiler filter APKs. // No reason to use a dm file if the dex is already uncompressed. diff --git a/dexpreopt/dexpreopt_gen/dexpreopt_gen.go b/dexpreopt/dexpreopt_gen/dexpreopt_gen.go index d54ddb1f0..009e90680 100644 --- a/dexpreopt/dexpreopt_gen/dexpreopt_gen.go +++ b/dexpreopt/dexpreopt_gen/dexpreopt_gen.go @@ -31,7 +31,6 @@ import ( var ( dexpreoptScriptPath = flag.String("dexpreopt_script", "", "path to output dexpreopt script") - stripScriptPath = flag.String("strip_script", "", "path to output strip script") globalConfigPath = flag.String("global", "", "path to global configuration file") moduleConfigPath = flag.String("module", "", "path to module configuration file") outDir = flag.String("out_dir", "", "path to output directory") @@ -64,10 +63,6 @@ func main() { usage("path to output dexpreopt script is required") } - if *stripScriptPath == "" { - usage("path to output strip script is required") - } - if *globalConfigPath == "" { usage("path to global configuration file is required") } @@ -90,10 +85,6 @@ func main() { os.Exit(2) } - // This shouldn't be using *PathForTesting, but it's outside of soong_build so its OK for now. - moduleConfig.StripInputPath = android.PathForTesting("$1") - moduleConfig.StripOutputPath = android.WritablePathForTesting("$2") - moduleConfig.DexPath = android.PathForTesting("$1") defer func() { @@ -110,11 +101,11 @@ func main() { } }() - writeScripts(ctx, globalConfig, moduleConfig, *dexpreoptScriptPath, *stripScriptPath) + writeScripts(ctx, globalConfig, moduleConfig, *dexpreoptScriptPath) } func writeScripts(ctx android.PathContext, global dexpreopt.GlobalConfig, module dexpreopt.ModuleConfig, - dexpreoptScriptPath, stripScriptPath string) { + dexpreoptScriptPath string) { dexpreoptRule, err := dexpreopt.GenerateDexpreoptRule(ctx, global, module) if err != nil { panic(err) @@ -135,11 +126,6 @@ func writeScripts(ctx android.PathContext, global dexpreopt.GlobalConfig, module FlagWithArg("-C ", installDir.String()). FlagWithArg("-D ", installDir.String()) - stripRule, err := dexpreopt.GenerateStripRule(global, module) - if err != nil { - panic(err) - } - write := func(rule *android.RuleBuilder, file string) { script := &bytes.Buffer{} script.WriteString(scriptHeader) @@ -180,15 +166,8 @@ func writeScripts(ctx android.PathContext, global dexpreopt.GlobalConfig, module if module.DexPath.String() != "$1" { panic(fmt.Errorf("module.DexPath must be '$1', was %q", module.DexPath)) } - if module.StripInputPath.String() != "$1" { - panic(fmt.Errorf("module.StripInputPath must be '$1', was %q", module.StripInputPath)) - } - if module.StripOutputPath.String() != "$2" { - panic(fmt.Errorf("module.StripOutputPath must be '$2', was %q", module.StripOutputPath)) - } write(dexpreoptRule, dexpreoptScriptPath) - write(stripRule, stripScriptPath) } const scriptHeader = `#!/bin/bash diff --git a/dexpreopt/dexpreopt_test.go b/dexpreopt/dexpreopt_test.go index aca5e63af..820f9e789 100644 --- a/dexpreopt/dexpreopt_test.go +++ b/dexpreopt/dexpreopt_test.go @@ -17,8 +17,6 @@ package dexpreopt import ( "android/soong/android" "fmt" - "reflect" - "strings" "testing" ) @@ -58,9 +56,6 @@ func testModuleConfig(ctx android.PathContext, name, partition string) ModuleCon NoCreateAppImage: false, ForceCreateAppImage: false, PresignedPrebuilt: false, - NoStripping: false, - StripInputPath: android.PathForOutput(ctx, fmt.Sprintf("unstripped/%s.apk", name)), - StripOutputPath: android.PathForOutput(ctx, fmt.Sprintf("stripped/%s.apk", name)), } } @@ -83,20 +78,6 @@ func TestDexPreopt(t *testing.T) { } } -func TestDexPreoptStrip(t *testing.T) { - // Test that we panic if we strip in a configuration where stripping is not allowed. - ctx := android.PathContextForTesting(android.TestConfig("out", nil), nil) - global, module := GlobalConfigForTests(ctx), testSystemModuleConfig(ctx, "test") - - global.NeverAllowStripping = true - module.NoStripping = false - - _, err := GenerateStripRule(global, module) - if err == nil { - t.Errorf("Expected an error when calling GenerateStripRule on a stripped module") - } -} - func TestDexPreoptSystemOther(t *testing.T) { ctx := android.PathContextForTesting(android.TestConfig("out", nil), nil) global := GlobalConfigForTests(ctx) @@ -177,56 +158,3 @@ func TestDexPreoptProfile(t *testing.T) { t.Errorf("\nwant installs:\n %v\ngot:\n %v", wantInstalls, rule.Installs()) } } - -func TestStripDex(t *testing.T) { - tests := []struct { - name string - setup func(global *GlobalConfig, module *ModuleConfig) - strip bool - }{ - { - name: "default strip", - setup: func(global *GlobalConfig, module *ModuleConfig) {}, - strip: true, - }, - { - name: "global no stripping", - setup: func(global *GlobalConfig, module *ModuleConfig) { global.DefaultNoStripping = true }, - strip: false, - }, - { - name: "module no stripping", - setup: func(global *GlobalConfig, module *ModuleConfig) { module.NoStripping = true }, - strip: false, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - - ctx := android.PathContextForTesting(android.TestConfig("out", nil), nil) - global, module := GlobalConfigForTests(ctx), testSystemModuleConfig(ctx, "test") - - test.setup(&global, &module) - - rule, err := GenerateStripRule(global, module) - if err != nil { - t.Fatal(err) - } - - if test.strip { - want := `zip2zip -i out/unstripped/test.apk -o out/stripped/test.apk -x "classes*.dex"` - if len(rule.Commands()) < 1 || !strings.Contains(rule.Commands()[0], want) { - t.Errorf("\nwant commands[0] to have:\n %v\ngot:\n %v", want, rule.Commands()[0]) - } - } else { - wantCommands := []string{ - "cp -f out/unstripped/test.apk out/stripped/test.apk", - } - if !reflect.DeepEqual(rule.Commands(), wantCommands) { - t.Errorf("\nwant commands:\n %v\ngot:\n %v", wantCommands, rule.Commands()) - } - } - }) - } -} diff --git a/java/app_test.go b/java/app_test.go index f2aaec344..05ab856e4 100644 --- a/java/app_test.go +++ b/java/app_test.go @@ -1149,13 +1149,7 @@ func TestAndroidAppImport_Presigned(t *testing.T) { variant.MaybeOutput("dexpreopt/oat/arm64/package.odex").Rule == nil { t.Errorf("can't find dexpreopt outputs") } - // Make sure stripping wasn't done. - stripRule := variant.Output("dexpreopt/foo.apk") - if !strings.HasPrefix(stripRule.RuleParams.Command, "cp -f") { - t.Errorf("unexpected, non-skipping strip command: %q", stripRule.RuleParams.Command) - } - - // Make sure signing was skipped and aligning was done instead. + // Make sure signing was skipped and aligning was done. if variant.MaybeOutput("signed/foo.apk").Rule != nil { t.Errorf("signing rule shouldn't be included.") } diff --git a/java/dexpreopt.go b/java/dexpreopt.go index b48871e43..2b1c9941e 100644 --- a/java/dexpreopt.go +++ b/java/dexpreopt.go @@ -40,13 +40,9 @@ type dexpreopter struct { type DexpreoptProperties struct { Dex_preopt struct { - // If false, prevent dexpreopting and stripping the dex file from the final jar. Defaults to - // true. + // If false, prevent dexpreopting. Defaults to true. Enabled *bool - // If true, never strip the dex files from the final jar when dexpreopting. Defaults to false. - No_stripping *bool - // If true, generate an app image (.art file) for this module. App_image *bool @@ -136,8 +132,6 @@ func (d *dexpreopter) dexpreopt(ctx android.ModuleContext, dexJarFile android.Mo dexLocation := android.InstallPathToOnDevicePath(ctx, d.installPath) - strippedDexJarFile := android.PathForModuleOut(ctx, "dexpreopt", dexJarFile.Base()) - var profileClassListing android.OptionalPath var profileBootListing android.OptionalPath profileIsTextListing := false @@ -191,10 +185,6 @@ func (d *dexpreopter) dexpreopt(ctx android.ModuleContext, dexJarFile android.Mo ForceCreateAppImage: BoolDefault(d.dexpreoptProperties.Dex_preopt.App_image, false), PresignedPrebuilt: d.isPresignedPrebuilt, - - NoStripping: Bool(d.dexpreoptProperties.Dex_preopt.No_stripping), - StripInputPath: dexJarFile, - StripOutputPath: strippedDexJarFile.OutputPath, } dexpreoptRule, err := dexpreopt.GenerateDexpreoptRule(ctx, global, dexpreoptConfig) @@ -207,13 +197,5 @@ func (d *dexpreopter) dexpreopt(ctx android.ModuleContext, dexJarFile android.Mo d.builtInstalled = dexpreoptRule.Installs().String() - stripRule, err := dexpreopt.GenerateStripRule(global, dexpreoptConfig) - if err != nil { - ctx.ModuleErrorf("error generating dexpreopt strip rule: %s", err.Error()) - return dexJarFile - } - - stripRule.Build(pctx, ctx, "dexpreopt_strip", "dexpreopt strip") - - return strippedDexJarFile + return dexJarFile }