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

@@ -16,6 +16,9 @@
 
 #include "odr_common.h"
 
+#include <sys/system_properties.h>
+
+#include <functional>
 #include <initializer_list>
 #include <regex>
 #include <sstream>
@@ -80,5 +83,21 @@ bool ShouldDisableRefresh(const std::string& sdk_version_str) {
   return sdk_version >= 32;
 }
 
+void SystemPropertyForeach(std::function<void(const char* name, const char* value)> 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<std::function<void(const char* name, const char* value)>*>(
+                      cookie);
+              (*action)(name, value);
+            },
+            cookie);
+      },
+      &action);
+}
+
 }  // namespace odrefresh
 }  // namespace art

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<void(const char* name, const char* value)> action);
+
 }  // namespace odrefresh
 }  // namespace art
 

odrefresh/odr_config.h

@@ -17,13 +17,15 @@
 #ifndef ART_ODREFRESH_ODR_CONFIG_H_
 #define ART_ODREFRESH_ODR_CONFIG_H_
 
+#include <algorithm>
 #include <optional>
 #include <string>
 #include <unordered_map>
 #include <vector>
 
 #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<std::vector<SystemPropertyConfig>> kSystemProperties{
     {SystemPropertyConfig{.name = "persist.device_config.runtime_native_boot.enable_uffd_gc",
                           .default_value = "false"}}};

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<std::string, std::string>& 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<std::string, std::string> cached_system_properties;
+  std::unordered_set<std::string> 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<std::string, std::string>& 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;
     }
   }

odrefresh/odrefresh.h

@@ -141,11 +141,12 @@ class OnDeviceRefresh final {
       /*out*/ std::vector<std::string>* 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;
 

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<std::string, std::string>* 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);

test/odsign/test-src/com/android/tests/odsign/OdrefreshHostTest.java

@@ -200,6 +200,60 @@ public void verifyEnableUffdGcChangeTriggersCompilation() throws Exception {
         }
     }
 
+    @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();