From 38fd254e85d723ddcd156572e0f5f833f703fc9d Mon Sep 17 00:00:00 2001 From: Jiakai Zhang Date: Mon, 7 Mar 2022 15:58:59 +0000 Subject: odrefresh: Disable partial compilation on devices without security fix Bug: 206090748 Test: atest art_standalone_odrefresh_tests Ignore-AOSP-First: Security fix Change-Id: I96e28d2284a3a0d275fc6556681dd5f3537e433b --- odrefresh/odrefresh_main.cc | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'odrefresh/odrefresh_main.cc') diff --git a/odrefresh/odrefresh_main.cc b/odrefresh/odrefresh_main.cc index c2298f038c..ffce81c692 100644 --- a/odrefresh/odrefresh_main.cc +++ b/odrefresh/odrefresh_main.cc @@ -41,6 +41,7 @@ using ::art::odrefresh::OdrConfig; using ::art::odrefresh::OdrMetrics; using ::art::odrefresh::OnDeviceRefresh; using ::art::odrefresh::QuotePath; +using ::art::odrefresh::ShouldDisablePartialCompilation; using ::art::odrefresh::ShouldDisableRefresh; using ::art::odrefresh::ZygoteKind; @@ -169,6 +170,12 @@ int InitializeConfig(int argc, char** argv, OdrConfig* config) { config->SetSystemServerCompilerFilter(filter); } + if (!config->HasPartialCompilation() && + ShouldDisablePartialCompilation( + android::base::GetProperty("ro.build.version.security_patch", /*default_value=*/""))) { + config->SetPartialCompilation(false); + } + if (ShouldDisableRefresh( android::base::GetProperty("ro.build.version.sdk", /*default_value=*/""))) { config->SetRefresh(false); -- cgit v1.2.3-59-g8ed1b From 45d0881f7444307955b51db5be88c5f890647c5e Mon Sep 17 00:00:00 2001 From: Jiakai Zhang Date: Wed, 4 May 2022 14:34:01 +0100 Subject: Re-compile on userfaultfd phenotype flag change. After this change, odrefresh re-compiles everything when the phenotype flag `runtime_native_boot.enable_uffd_gc` changes. It writes the value of the flag to cache-info.xml for change detection. According to go/platform-experiment-namespaces#namespace-types, the phenotype flag is back by a persistent system property. Therefore, we can directly read the flag from the system property instead of depending on the `server_configurable_flags` library. This behavior is consistent with the existing ART code (particularly, `art::Flag`), which reads other phenotype flags. Bug: 231298279 Test: atest odsign_e2e_tests_full Ignore-AOSP-First: Merge conflict. Will cherry-pick later. Change-Id: I02b70569ecd96d4ded6d2d3be22c34b2c6a4c5b4 --- odrefresh/CacheInfo.xsd | 14 +++ odrefresh/odr_config.h | 23 ++++ odrefresh/odrefresh.cc | 134 +++++++++++++++++++-- odrefresh/odrefresh.h | 19 +++ odrefresh/odrefresh_main.cc | 24 ++-- odrefresh/schema/current.txt | 16 +++ .../android/tests/odsign/OdrefreshHostTest.java | 54 +++++++++ 7 files changed, 264 insertions(+), 20 deletions(-) (limited to 'odrefresh/odrefresh_main.cc') diff --git a/odrefresh/CacheInfo.xsd b/odrefresh/CacheInfo.xsd index 1cbef8a23e..196caf7527 100644 --- a/odrefresh/CacheInfo.xsd +++ b/odrefresh/CacheInfo.xsd @@ -27,6 +27,7 @@ + @@ -36,6 +37,19 @@ + + + + + + + + + + + + + diff --git a/odrefresh/odr_config.h b/odrefresh/odr_config.h index ec0290ccfe..c3f3c8010a 100644 --- a/odrefresh/odr_config.h +++ b/odrefresh/odr_config.h @@ -19,9 +19,11 @@ #include #include +#include #include #include "android-base/file.h" +#include "android-base/no_destructor.h" #include "arch/instruction_set.h" #include "base/file_utils.h" #include "base/globals.h" @@ -32,6 +34,17 @@ namespace art { namespace odrefresh { +struct SystemPropertyConfig { + const char* name; + const char* default_value; +}; + +// The system properties that odrefresh keeps track of. Odrefresh will recompile everything if any +// property changes. +const android::base::NoDestructor> kSystemProperties{ + {SystemPropertyConfig{.name = "persist.device_config.runtime_native_boot.enable_uffd_gc", + .default_value = "false"}}}; + // An enumeration of the possible zygote configurations on Android. enum class ZygoteKind : uint8_t { // 32-bit primary zygote, no secondary zygote. @@ -66,6 +79,9 @@ class OdrConfig final { bool compilation_os_mode_ = false; bool minimal_ = false; + // The current values of system properties listed in `kSystemProperties`. + std::unordered_map system_properties_; + // Staging directory for artifacts. The directory must exist and will be automatically removed // after compilation. If empty, use the default directory. std::string staging_dir_; @@ -148,6 +164,9 @@ class OdrConfig final { } bool GetCompilationOsMode() const { return compilation_os_mode_; } bool GetMinimal() const { return minimal_; } + const std::unordered_map& GetSystemProperties() const { + return system_properties_; + } void SetApexInfoListFile(const std::string& file_path) { apex_info_list_file_ = file_path; } void SetArtBinDir(const std::string& art_bin_dir) { art_bin_dir_ = art_bin_dir; } @@ -199,6 +218,10 @@ class OdrConfig final { void SetMinimal(bool value) { minimal_ = value; } + std::unordered_map* MutableSystemProperties() { + return &system_properties_; + } + private: // Returns a pair for the possible instruction sets for the configured instruction set // architecture. The first item is the 32-bit architecture and the second item is the 64-bit diff --git a/odrefresh/odrefresh.cc b/odrefresh/odrefresh.cc index 1d12b94fce..70cf48e918 100644 --- a/odrefresh/odrefresh.cc +++ b/odrefresh/odrefresh.cc @@ -648,6 +648,11 @@ Result OnDeviceRefresh::WriteCacheInfo() const { return Errorf("Could not create directory {}", QuotePath(dir_name)); } + std::vector system_properties; + for (const auto& [key, value] : config_.GetSystemProperties()) { + system_properties.emplace_back(key, value); + } + std::optional> apex_info_list = GetApexInfoList(); if (!apex_info_list.has_value()) { return Errorf("Could not update {}: no APEX info", QuotePath(cache_info_filename_)); @@ -685,15 +690,16 @@ Result OnDeviceRefresh::WriteCacheInfo() const { return Errorf("Cannot open {} for writing.", QuotePath(cache_info_filename_)); } - art_apex::CacheInfo info( + std::unique_ptr info(new art_apex::CacheInfo( + {art_apex::KeyValuePairList(system_properties)}, {art_module_info}, {art_apex::ModuleInfoList(module_info_list)}, {art_apex::Classpath(bcp_components.value())}, {art_apex::Classpath(bcp_compilable_components.value())}, {art_apex::SystemServerComponents(system_server_components.value())}, - config_.GetCompilationOsMode() ? std::make_optional(true) : std::nullopt); + config_.GetCompilationOsMode() ? std::make_optional(true) : std::nullopt)); - art_apex::write(out, info); + art_apex::write(out, *info); out.close(); if (out.fail()) { return Errorf("Cannot write to {}", QuotePath(cache_info_filename_)); @@ -836,16 +842,106 @@ WARN_UNUSED bool OnDeviceRefresh::SystemServerArtifactsExist( return jars_missing_artifacts->empty(); } +WARN_UNUSED bool OnDeviceRefresh::CheckSystemPropertiesAreDefault() const { + const std::unordered_map& system_properties = + config_.GetSystemProperties(); + + for (const SystemPropertyConfig& system_property_config : *kSystemProperties.get()) { + auto property = system_properties.find(system_property_config.name); + DCHECK(property != system_properties.end()); + + if (property->second != system_property_config.default_value) { + LOG(INFO) << "System property " << system_property_config.name << " has a non-default value (" + << property->second << ")."; + return false; + } + } + + return true; +} + +WARN_UNUSED bool OnDeviceRefresh::CheckSystemPropertiesHaveNotChanged( + const art_apex::CacheInfo& cache_info) const { + std::unordered_map cached_system_properties; + const art_apex::KeyValuePairList* list = cache_info.getFirstSystemProperties(); + if (list == nullptr) { + // This should never happen. We have already checked the ART module version, and the cache + // info is generated by the latest version of the ART module if it exists. + LOG(ERROR) << "Missing system properties from cache-info."; + return false; + } + + for (const art_apex::KeyValuePair& pair : list->getItem()) { + cached_system_properties[pair.getK()] = pair.getV(); + } + + const std::unordered_map& system_properties = + config_.GetSystemProperties(); + + for (const SystemPropertyConfig& system_property_config : *kSystemProperties.get()) { + auto property = system_properties.find(system_property_config.name); + DCHECK(property != system_properties.end()); + + auto cached_property = cached_system_properties.find(system_property_config.name); + if (cached_property == cached_system_properties.end()) { + // This should never happen. We have already checked the ART module version, and the cache + // info is generated by the latest version of the ART module if it exists. + LOG(ERROR) << "Missing system property from cache-info (" << system_property_config.name + << ")"; + return false; + } + + if (property->second != cached_property->second) { + LOG(INFO) << "System property " << system_property_config.name + << " value changed (before: " << cached_property->second + << ", now: " << property->second << ")."; + return false; + } + } + + return true; +} + +WARN_UNUSED bool OnDeviceRefresh::BootClasspathArtifactsOnSystemUsable( + const apex::ApexInfo& art_apex_info) const { + if (!art_apex_info.getIsFactory()) { + return false; + } + LOG(INFO) << "Factory ART APEX mounted."; + + if (!CheckSystemPropertiesAreDefault()) { + return false; + } + LOG(INFO) << "System properties are set to default values."; + + return true; +} + +WARN_UNUSED bool OnDeviceRefresh::SystemServerArtifactsOnSystemUsable( + const std::vector& apex_info_list) const { + if (std::any_of(apex_info_list.begin(), + apex_info_list.end(), + [](const apex::ApexInfo& apex_info) { return !apex_info.getIsFactory(); })) { + return false; + } + LOG(INFO) << "Factory APEXes mounted."; + + if (!CheckSystemPropertiesAreDefault()) { + return false; + } + LOG(INFO) << "System properties are set to default values."; + + return true; +} + WARN_UNUSED bool OnDeviceRefresh::CheckBootClasspathArtifactsAreUpToDate( OdrMetrics& metrics, const InstructionSet isa, const apex::ApexInfo& art_apex_info, const std::optional& cache_info, /*out*/ std::vector* checked_artifacts) const { - if (art_apex_info.getIsFactory()) { - LOG(INFO) << "Factory ART APEX mounted."; - - // ART is not updated, so we can use the artifacts on /system. Check if they exist. + if (BootClasspathArtifactsOnSystemUsable(art_apex_info)) { + // We can use the artifacts on /system. Check if they exist. std::string error_msg; if (BootClasspathArtifactsExist(/*on_system=*/true, /*minimal=*/false, isa, &error_msg)) { return true; @@ -900,6 +996,14 @@ WARN_UNUSED bool OnDeviceRefresh::CheckBootClasspathArtifactsAreUpToDate( return false; } + if (!CheckSystemPropertiesHaveNotChanged(cache_info.value())) { + // We don't have a trigger kind for system property changes. For now, we reuse + // `kApexVersionMismatch` as it implies the expected behavior: re-compile regardless of the last + // compilation attempt. + metrics.SetTrigger(OdrMetrics::Trigger::kApexVersionMismatch); + return false; + } + // Check boot class components. // // This checks the size and checksums of odrefresh compilable files on the DEX2OATBOOTCLASSPATH @@ -961,12 +1065,8 @@ bool OnDeviceRefresh::CheckSystemServerArtifactsAreUpToDate( std::set jars_missing_artifacts_on_system; bool artifacts_on_system_up_to_date = false; - if (std::all_of(apex_info_list.begin(), - apex_info_list.end(), - [](const apex::ApexInfo& apex_info) { return apex_info.getIsFactory(); })) { - LOG(INFO) << "Factory APEXes mounted."; - - // APEXes are not updated, so we can use the artifacts on /system. Check if they exist. + if (SystemServerArtifactsOnSystemUsable(apex_info_list)) { + // We can use the artifacts on /system. Check if they exist. std::string error_msg; if (SystemServerArtifactsExist( /*on_system=*/true, &error_msg, &jars_missing_artifacts_on_system)) { @@ -1052,6 +1152,14 @@ bool OnDeviceRefresh::CheckSystemServerArtifactsAreUpToDate( } } + if (!CheckSystemPropertiesHaveNotChanged(cache_info.value())) { + // We don't have a trigger kind for system property changes. For now, we reuse + // `kApexVersionMismatch` as it implies the expected behavior: re-compile regardless of the last + // compilation attempt. + metrics.SetTrigger(OdrMetrics::Trigger::kApexVersionMismatch); + return false; + } + // Check system server components. // // This checks the size and checksums of odrefresh compilable files on the diff --git a/odrefresh/odrefresh.h b/odrefresh/odrefresh.h index 58da57a1cd..9dd4009016 100644 --- a/odrefresh/odrefresh.h +++ b/odrefresh/odrefresh.h @@ -140,6 +140,25 @@ class OnDeviceRefresh final { /*out*/ std::set* jars_missing_artifacts, /*out*/ std::vector* checked_artifacts = nullptr) const; + // Returns true if all of the system properties listed in `kSystemProperties` are set to the + // default values. + WARN_UNUSED bool CheckSystemPropertiesAreDefault() const; + + // Returns true if none of the system properties listed in `kSystemProperties` has changed since + // the last compilation. + WARN_UNUSED bool CheckSystemPropertiesHaveNotChanged( + const com::android::art::CacheInfo& cache_info) const; + + // Returns true if boot classpath artifacts on /system are usable if they exist. Note that this + // function does not check file existence. + WARN_UNUSED bool BootClasspathArtifactsOnSystemUsable( + const com::android::apex::ApexInfo& art_apex_info) const; + + // Returns true if system_server artifacts on /system are usable if they exist. Note that this + // function does not check file existence. + WARN_UNUSED bool SystemServerArtifactsOnSystemUsable( + const std::vector& apex_info_list) const; + // Checks whether all boot classpath artifacts are up to date. Returns true if all are present, // false otherwise. // If `checked_artifacts` is present, adds checked artifacts to `checked_artifacts`. diff --git a/odrefresh/odrefresh_main.cc b/odrefresh/odrefresh_main.cc index ffce81c692..f8cc0b15dc 100644 --- a/odrefresh/odrefresh_main.cc +++ b/odrefresh/odrefresh_main.cc @@ -18,6 +18,7 @@ #include #include +#include #include "android-base/properties.h" #include "android-base/stringprintf.h" @@ -34,8 +35,10 @@ namespace { +using ::android::base::GetProperty; using ::art::odrefresh::CompilationOptions; using ::art::odrefresh::ExitCode; +using ::art::odrefresh::kSystemProperties; using ::art::odrefresh::OdrCompilationLog; using ::art::odrefresh::OdrConfig; using ::art::odrefresh::OdrMetrics; @@ -43,6 +46,7 @@ using ::art::odrefresh::OnDeviceRefresh; using ::art::odrefresh::QuotePath; using ::art::odrefresh::ShouldDisablePartialCompilation; using ::art::odrefresh::ShouldDisableRefresh; +using ::art::odrefresh::SystemPropertyConfig; using ::art::odrefresh::ZygoteKind; void UsageMsgV(const char* fmt, va_list ap) { @@ -156,7 +160,7 @@ int InitializeConfig(int argc, char** argv, OdrConfig* config) { if (zygote.empty()) { // Use ro.zygote by default, if not overridden by --zygote-arch flag. - zygote = android::base::GetProperty("ro.zygote", {}); + zygote = GetProperty("ro.zygote", {}); } ZygoteKind zygote_kind; if (!ParseZygoteKind(zygote.c_str(), &zygote_kind)) { @@ -165,25 +169,30 @@ int InitializeConfig(int argc, char** argv, OdrConfig* config) { config->SetZygoteKind(zygote_kind); if (config->GetSystemServerCompilerFilter().empty()) { - std::string filter = - android::base::GetProperty("dalvik.vm.systemservercompilerfilter", "speed"); + std::string filter = GetProperty("dalvik.vm.systemservercompilerfilter", "speed"); config->SetSystemServerCompilerFilter(filter); } if (!config->HasPartialCompilation() && ShouldDisablePartialCompilation( - android::base::GetProperty("ro.build.version.security_patch", /*default_value=*/""))) { + GetProperty("ro.build.version.security_patch", /*default_value=*/""))) { config->SetPartialCompilation(false); } - if (ShouldDisableRefresh( - android::base::GetProperty("ro.build.version.sdk", /*default_value=*/""))) { + if (ShouldDisableRefresh(GetProperty("ro.build.version.sdk", /*default_value=*/""))) { config->SetRefresh(false); } return n; } +void GetSystemProperties(std::unordered_map* system_properties) { + for (const SystemPropertyConfig& system_property_config : *kSystemProperties.get()) { + (*system_properties)[system_property_config.name] = + GetProperty(system_property_config.name, system_property_config.default_value); + } +} + NO_RETURN void UsageHelp(const char* argv0) { std::string name(android::base::Basename(argv0)); UsageMsg("Usage: %s [OPTION...] ACTION", name.c_str()); @@ -236,6 +245,7 @@ int main(int argc, char** argv) { if (argc != 1) { ArgumentError("Expected 1 argument, but have %d.", argc); } + GetSystemProperties(config.MutableSystemProperties()); OdrMetrics metrics(config.GetArtifactDirectory()); OnDeviceRefresh odr(config); @@ -275,6 +285,6 @@ int main(int argc, char** argv) { } else if (action == "--help") { UsageHelp(argv[0]); } else { - ArgumentError("Unknown argument: ", action); + ArgumentError("Unknown argument: %s", action.data()); } } diff --git a/odrefresh/schema/current.txt b/odrefresh/schema/current.txt index 958f2a060a..c45870b6e9 100644 --- a/odrefresh/schema/current.txt +++ b/odrefresh/schema/current.txt @@ -8,12 +8,14 @@ package com.android.art { method public boolean getCompilationOsMode(); method public com.android.art.Classpath getDex2oatBootClasspath(); method public com.android.art.ModuleInfoList getModuleInfoList(); + method public com.android.art.KeyValuePairList getSystemProperties(); method public com.android.art.SystemServerComponents getSystemServerComponents(); method public void setArtModuleInfo(com.android.art.ModuleInfo); method public void setBootClasspath(com.android.art.Classpath); method public void setCompilationOsMode(boolean); method public void setDex2oatBootClasspath(com.android.art.Classpath); method public void setModuleInfoList(com.android.art.ModuleInfoList); + method public void setSystemProperties(com.android.art.KeyValuePairList); method public void setSystemServerComponents(com.android.art.SystemServerComponents); } @@ -33,6 +35,20 @@ package com.android.art { method public void setSize(java.math.BigInteger); } + public class KeyValuePair { + ctor public KeyValuePair(); + method public String getK(); + method public String getV(); + method public void setK(String); + method public void setV(String); + } + + public class KeyValuePairList { + ctor public KeyValuePairList(); + method public com.android.art.KeyValuePair getItem(); + method public void setItem(com.android.art.KeyValuePair); + } + public class ModuleInfo { ctor public ModuleInfo(); method public long getLastUpdateMillis(); diff --git a/test/odsign/test-src/com/android/tests/odsign/OdrefreshHostTest.java b/test/odsign/test-src/com/android/tests/odsign/OdrefreshHostTest.java index d27902f140..1b852d7ac2 100644 --- a/test/odsign/test-src/com/android/tests/odsign/OdrefreshHostTest.java +++ b/test/odsign/test-src/com/android/tests/odsign/OdrefreshHostTest.java @@ -143,6 +143,60 @@ public class OdrefreshHostTest extends BaseHostJUnit4Test { assertArtifactsModifiedAfter(missingArtifacts, timeMs); } + @Test + public void verifyEnableUffdGcChangeTriggersCompilation() throws Exception { + try { + // Disable phenotype flag syncing. + getDevice().executeShellV2Command( + "device_config set_sync_disabled_for_tests until_reboot"); + + // Simulate that the phenotype flag is set to the default value. + getDevice().executeShellV2Command( + "device_config put runtime_native_boot enable_uffd_gc false"); + + long timeMs = mTestUtils.getCurrentTimeMs(); + getDevice().executeShellV2Command(ODREFRESH_COMMAND); + + // Artifacts should not be re-compiled. + assertArtifactsNotModifiedAfter(getZygoteArtifacts(), timeMs); + assertArtifactsNotModifiedAfter(getSystemServerArtifacts(), timeMs); + + // Simulate that the phenotype flag is set to true. + getDevice().executeShellV2Command( + "device_config put runtime_native_boot enable_uffd_gc true"); + + timeMs = mTestUtils.getCurrentTimeMs(); + getDevice().executeShellV2Command(ODREFRESH_COMMAND); + + // Artifacts should be re-compiled. + assertArtifactsModifiedAfter(getZygoteArtifacts(), timeMs); + assertArtifactsModifiedAfter(getSystemServerArtifacts(), timeMs); + + // Run odrefresh again with the flag unchanged. + timeMs = mTestUtils.getCurrentTimeMs(); + getDevice().executeShellV2Command(ODREFRESH_COMMAND); + + // Artifacts should not be re-compiled. + assertArtifactsNotModifiedAfter(getZygoteArtifacts(), timeMs); + assertArtifactsNotModifiedAfter(getSystemServerArtifacts(), timeMs); + + // Simulate that the phenotype flag is set to false. + getDevice().executeShellV2Command( + "device_config put runtime_native_boot enable_uffd_gc false"); + + timeMs = mTestUtils.getCurrentTimeMs(); + getDevice().executeShellV2Command(ODREFRESH_COMMAND); + + // Artifacts should be re-compiled. + assertArtifactsModifiedAfter(getZygoteArtifacts(), timeMs); + assertArtifactsModifiedAfter(getSystemServerArtifacts(), timeMs); + } finally { + getDevice().executeShellV2Command("device_config set_sync_disabled_for_tests none"); + getDevice().executeShellV2Command( + "device_config delete runtime_native_boot enable_uffd_gc"); + } + } + @Test public void verifyNoCompilationWhenCacheIsGood() throws Exception { mTestUtils.removeCompilationLogToAvoidBackoff(); -- cgit v1.2.3-59-g8ed1b From dff7de431edb81f5fb5cc25df10fe8eda83d0e6e Mon Sep 17 00:00:00 2001 From: Jiakai Zhang Date: Fri, 13 May 2022 18:47:09 +0100 Subject: Check all art-related system properties. After this change, odrefresh writes the following system properties to cache-info.xml and checks them on boot. - all properties starting with `dalvik.vm.` - all properties starting with `ro.dalvik.vm.` - `persist.device_config.runtime_native_boot.enable_uffd_gc` Bug: 231974881 Test: atest odsign_e2e_tests_full Ignore-AOSP-First: Will cherry-pick later Change-Id: I4f6790a2c18409c7ec93ae0369b3160a29bb49c3 --- odrefresh/odr_common.cc | 19 ++++++++ odrefresh/odr_common.h | 3 ++ odrefresh/odr_config.h | 16 ++++++- odrefresh/odrefresh.cc | 35 ++++++++------ odrefresh/odrefresh.h | 5 +- odrefresh/odrefresh_main.cc | 13 ++++++ .../android/tests/odsign/OdrefreshHostTest.java | 54 ++++++++++++++++++++++ 7 files changed, 126 insertions(+), 19 deletions(-) (limited to 'odrefresh/odrefresh_main.cc') diff --git a/odrefresh/odr_common.cc b/odrefresh/odr_common.cc index a83e18ec2e..94674bc1d0 100644 --- a/odrefresh/odr_common.cc +++ b/odrefresh/odr_common.cc @@ -16,6 +16,9 @@ #include "odr_common.h" +#include + +#include #include #include #include @@ -80,5 +83,21 @@ bool ShouldDisableRefresh(const std::string& sdk_version_str) { return sdk_version >= 32; } +void SystemPropertyForeach(std::function action) { + __system_property_foreach( + [](const prop_info* pi, void* cookie) { + __system_property_read_callback( + pi, + [](void* cookie, const char* name, const char* value, unsigned) { + auto action = + reinterpret_cast*>( + cookie); + (*action)(name, value); + }, + cookie); + }, + &action); +} + } // namespace odrefresh } // namespace art diff --git a/odrefresh/odr_common.h b/odrefresh/odr_common.h index 63f35f904d..1257ab77f8 100644 --- a/odrefresh/odr_common.h +++ b/odrefresh/odr_common.h @@ -44,6 +44,9 @@ bool ShouldDisablePartialCompilation(const std::string& security_patch_str); // `ro.build.version.sdk`, which represents the SDK version. bool ShouldDisableRefresh(const std::string& sdk_version_str); +// Passes the name and the value for each system property to the provided callback. +void SystemPropertyForeach(std::function action); + } // namespace odrefresh } // namespace art diff --git a/odrefresh/odr_config.h b/odrefresh/odr_config.h index c3f3c8010a..04754663a3 100644 --- a/odrefresh/odr_config.h +++ b/odrefresh/odr_config.h @@ -17,6 +17,7 @@ #ifndef ART_ODREFRESH_ODR_CONFIG_H_ #define ART_ODREFRESH_ODR_CONFIG_H_ +#include #include #include #include @@ -24,6 +25,7 @@ #include "android-base/file.h" #include "android-base/no_destructor.h" +#include "android-base/strings.h" #include "arch/instruction_set.h" #include "base/file_utils.h" #include "base/globals.h" @@ -34,13 +36,23 @@ namespace art { namespace odrefresh { +// The prefixes of system properties that odrefresh keeps track of. Odrefresh will recompile +// everything if any property matching a prefix changes. +constexpr const char* kCheckedSystemPropertyPrefixes[]{"dalvik.vm.", "ro.dalvik.vm."}; + struct SystemPropertyConfig { const char* name; const char* default_value; }; -// The system properties that odrefresh keeps track of. Odrefresh will recompile everything if any -// property changes. +// The system properties that odrefresh keeps track of, in addition to the ones matching the +// prefixes in `kCheckedSystemPropertyPrefixes`. Odrefresh will recompile everything if any property +// changes. +// All phenotype flags under the `runtime_native_boot` namespace that affects the compiler's +// behavior must be explicitly listed below. We cannot use a prefix to match all phenotype flags +// because a default value is required for each flag. Changing the flag value from empty to the +// default value should not trigger re-compilation. This is to comply with the phenotype flag +// requirement (go/platform-experiments-flags#pre-requisites). const android::base::NoDestructor> kSystemProperties{ {SystemPropertyConfig{.name = "persist.device_config.runtime_native_boot.enable_uffd_gc", .default_value = "false"}}}; diff --git a/odrefresh/odrefresh.cc b/odrefresh/odrefresh.cc index 70cf48e918..f829bb8ee1 100644 --- a/odrefresh/odrefresh.cc +++ b/odrefresh/odrefresh.cc @@ -843,6 +843,13 @@ WARN_UNUSED bool OnDeviceRefresh::SystemServerArtifactsExist( } WARN_UNUSED bool OnDeviceRefresh::CheckSystemPropertiesAreDefault() const { + // We don't have to check properties that match `kCheckedSystemPropertyPrefixes` here because none + // of them is persistent. This only applies when `cache-info.xml` does not exist. When + // `cache-info.xml` exists, we call `CheckSystemPropertiesHaveNotChanged` instead. + DCHECK(std::none_of(std::begin(kCheckedSystemPropertyPrefixes), + std::end(kCheckedSystemPropertyPrefixes), + [](const char* prefix) { return StartsWith(prefix, "persist."); })); + const std::unordered_map& system_properties = config_.GetSystemProperties(); @@ -863,6 +870,8 @@ WARN_UNUSED bool OnDeviceRefresh::CheckSystemPropertiesAreDefault() const { WARN_UNUSED bool OnDeviceRefresh::CheckSystemPropertiesHaveNotChanged( const art_apex::CacheInfo& cache_info) const { std::unordered_map cached_system_properties; + std::unordered_set checked_properties; + const art_apex::KeyValuePairList* list = cache_info.getFirstSystemProperties(); if (list == nullptr) { // This should never happen. We have already checked the ART module version, and the cache @@ -873,28 +882,24 @@ WARN_UNUSED bool OnDeviceRefresh::CheckSystemPropertiesHaveNotChanged( for (const art_apex::KeyValuePair& pair : list->getItem()) { cached_system_properties[pair.getK()] = pair.getV(); + checked_properties.insert(pair.getK()); } const std::unordered_map& system_properties = config_.GetSystemProperties(); - for (const SystemPropertyConfig& system_property_config : *kSystemProperties.get()) { - auto property = system_properties.find(system_property_config.name); - DCHECK(property != system_properties.end()); + for (const auto& [key, value] : system_properties) { + checked_properties.insert(key); + } - auto cached_property = cached_system_properties.find(system_property_config.name); - if (cached_property == cached_system_properties.end()) { - // This should never happen. We have already checked the ART module version, and the cache - // info is generated by the latest version of the ART module if it exists. - LOG(ERROR) << "Missing system property from cache-info (" << system_property_config.name - << ")"; - return false; - } + for (const std::string& name : checked_properties) { + auto property_it = system_properties.find(name); + std::string property = property_it != system_properties.end() ? property_it->second : ""; + std::string cached_property = cached_system_properties[name]; - if (property->second != cached_property->second) { - LOG(INFO) << "System property " << system_property_config.name - << " value changed (before: " << cached_property->second - << ", now: " << property->second << ")."; + if (property != cached_property) { + LOG(INFO) << "System property " << name << " value changed (before: \"" << cached_property + << "\", now: \"" << property << "\")."; return false; } } diff --git a/odrefresh/odrefresh.h b/odrefresh/odrefresh.h index 9dd4009016..e48567fced 100644 --- a/odrefresh/odrefresh.h +++ b/odrefresh/odrefresh.h @@ -141,11 +141,12 @@ class OnDeviceRefresh final { /*out*/ std::vector* checked_artifacts = nullptr) const; // Returns true if all of the system properties listed in `kSystemProperties` are set to the - // default values. + // default values. This function is usually called when cache-info.xml does not exist (i.e., + // compilation has not been done before). WARN_UNUSED bool CheckSystemPropertiesAreDefault() const; // Returns true if none of the system properties listed in `kSystemProperties` has changed since - // the last compilation. + // the last compilation. This function is usually called when cache-info.xml exists. WARN_UNUSED bool CheckSystemPropertiesHaveNotChanged( const com::android::art::CacheInfo& cache_info) const; diff --git a/odrefresh/odrefresh_main.cc b/odrefresh/odrefresh_main.cc index f8cc0b15dc..58ef28fdf7 100644 --- a/odrefresh/odrefresh_main.cc +++ b/odrefresh/odrefresh_main.cc @@ -36,8 +36,10 @@ namespace { using ::android::base::GetProperty; +using ::android::base::StartsWith; using ::art::odrefresh::CompilationOptions; using ::art::odrefresh::ExitCode; +using ::art::odrefresh::kCheckedSystemPropertyPrefixes; using ::art::odrefresh::kSystemProperties; using ::art::odrefresh::OdrCompilationLog; using ::art::odrefresh::OdrConfig; @@ -47,6 +49,7 @@ using ::art::odrefresh::QuotePath; using ::art::odrefresh::ShouldDisablePartialCompilation; using ::art::odrefresh::ShouldDisableRefresh; using ::art::odrefresh::SystemPropertyConfig; +using ::art::odrefresh::SystemPropertyForeach; using ::art::odrefresh::ZygoteKind; void UsageMsgV(const char* fmt, va_list ap) { @@ -187,6 +190,16 @@ int InitializeConfig(int argc, char** argv, OdrConfig* config) { } void GetSystemProperties(std::unordered_map* system_properties) { + SystemPropertyForeach([&](const char* name, const char* value) { + if (strlen(value) == 0) { + return; + } + for (const char* prefix : kCheckedSystemPropertyPrefixes) { + if (StartsWith(name, prefix)) { + (*system_properties)[name] = value; + } + } + }); for (const SystemPropertyConfig& system_property_config : *kSystemProperties.get()) { (*system_properties)[system_property_config.name] = GetProperty(system_property_config.name, system_property_config.default_value); diff --git a/test/odsign/test-src/com/android/tests/odsign/OdrefreshHostTest.java b/test/odsign/test-src/com/android/tests/odsign/OdrefreshHostTest.java index de497c52ac..731ea38999 100644 --- a/test/odsign/test-src/com/android/tests/odsign/OdrefreshHostTest.java +++ b/test/odsign/test-src/com/android/tests/odsign/OdrefreshHostTest.java @@ -200,6 +200,60 @@ public class OdrefreshHostTest extends BaseHostJUnit4Test { } } + @Test + public void verifySystemPropertyMismatchTriggersCompilation() throws Exception { + // Change a system property from empty to a value. + getDevice().setProperty("dalvik.vm.foo", "1"); + long timeMs = mTestUtils.getCurrentTimeMs(); + getDevice().executeShellV2Command(ODREFRESH_COMMAND); + + // Artifacts should be re-compiled. + assertArtifactsModifiedAfter(getZygoteArtifacts(), timeMs); + assertArtifactsModifiedAfter(getSystemServerArtifacts(), timeMs); + + // Run again with the same value. + timeMs = mTestUtils.getCurrentTimeMs(); + getDevice().executeShellV2Command(ODREFRESH_COMMAND); + + // Artifacts should not be re-compiled. + assertArtifactsNotModifiedAfter(getZygoteArtifacts(), timeMs); + assertArtifactsNotModifiedAfter(getSystemServerArtifacts(), timeMs); + + // Change the system property to another value. + getDevice().setProperty("dalvik.vm.foo", "2"); + timeMs = mTestUtils.getCurrentTimeMs(); + getDevice().executeShellV2Command(ODREFRESH_COMMAND); + + // Artifacts should be re-compiled. + assertArtifactsModifiedAfter(getZygoteArtifacts(), timeMs); + assertArtifactsModifiedAfter(getSystemServerArtifacts(), timeMs); + + // Run again with the same value. + timeMs = mTestUtils.getCurrentTimeMs(); + getDevice().executeShellV2Command(ODREFRESH_COMMAND); + + // Artifacts should not be re-compiled. + assertArtifactsNotModifiedAfter(getZygoteArtifacts(), timeMs); + assertArtifactsNotModifiedAfter(getSystemServerArtifacts(), timeMs); + + // Change the system property to empty. + getDevice().setProperty("dalvik.vm.foo", ""); + timeMs = mTestUtils.getCurrentTimeMs(); + getDevice().executeShellV2Command(ODREFRESH_COMMAND); + + // Artifacts should be re-compiled. + assertArtifactsModifiedAfter(getZygoteArtifacts(), timeMs); + assertArtifactsModifiedAfter(getSystemServerArtifacts(), timeMs); + + // Run again with the same value. + timeMs = mTestUtils.getCurrentTimeMs(); + getDevice().executeShellV2Command(ODREFRESH_COMMAND); + + // Artifacts should not be re-compiled. + assertArtifactsNotModifiedAfter(getZygoteArtifacts(), timeMs); + assertArtifactsNotModifiedAfter(getSystemServerArtifacts(), timeMs); + } + @Test public void verifyNoCompilationWhenCacheIsGood() throws Exception { mTestUtils.removeCompilationLogToAvoidBackoff(); -- cgit v1.2.3-59-g8ed1b