Remove Flags

These were reading system properties which caused security warnings in
some cases. Removing to allow us to do a more comprehensive design.

This reverts commit 0a6e9e56f7f3ac7750b38eaba83639ad47a2692f.
This reverts commit 3dba023d4fb47882fa215715c196cfa3be30c098.

Test: test.py --host
Change-Id: I04e8b7a934540b250e6fc56f5aa6ce7f18131d4d

12 files changed, 35 insertions, 332 deletions

diff --git a/cmdline/cmdline_parser.h b/cmdline/cmdline_parser.h
index 7e343f8ef2..22eb44c211 100644
--- a/cmdline/cmdline_parser.h
+++ b/cmdline/cmdline_parser.h
@@ -210,24 +210,6 @@ struct CmdlineParser {
       return parent_;
     }
 
-    // Write the results of this argument into a variable pointed to by destination.
-    // An optional is used to tell whether the command line argument was present.
-    CmdlineParser::Builder& IntoLocation(std::optional<TArg>* destination) {
-      save_value_ = [destination](TArg& value) {
-        *destination = value;
-      };
-
-      load_value_ = [destination]() -> TArg& {
-        return destination->value();
-      };
-
-      save_value_specified_ = true;
-      load_value_specified_ = true;
-
-      CompleteArgument();
-      return parent_;
-    }
-
     // Ensure we always move this when returning a new builder.
     ArgumentBuilder(ArgumentBuilder&&) = default;
 
diff --git a/cmdline/cmdline_types.h b/cmdline/cmdline_types.h
index 7f01be6472..2d7d5f1b78 100644
--- a/cmdline/cmdline_types.h
+++ b/cmdline/cmdline_types.h
@@ -21,7 +21,6 @@
 #include <list>
 #include <ostream>
 
-#include "android-base/parsebool.h"
 #include "android-base/stringprintf.h"
 #include "cmdline_type_parser.h"
 #include "detail/cmdline_debug_detail.h"
@@ -68,22 +67,6 @@ struct CmdlineType<Unit> : CmdlineTypeParser<Unit> {
 };
 
 template <>
-struct CmdlineType<bool> : CmdlineTypeParser<bool> {
-  Result Parse(const std::string& args) {
-    switch (::android::base::ParseBool(args)) {
-      case ::android::base::ParseBoolResult::kError:
-        return Result::Failure("Could not parse '" + args + "' as boolean");
-      case ::android::base::ParseBoolResult::kTrue:
-        return Result::Success(true);
-      case ::android::base::ParseBoolResult::kFalse:
-        return Result::Success(false);
-    }
-  }
-
-  static const char* DescribeType() { return "true|false|0|1|y|n|yes|no|on|off"; }
-};
-
-template <>
 struct CmdlineType<JdwpProvider> : CmdlineTypeParser<JdwpProvider> {
   /*
    * Handle a single JDWP provider name. Must be either 'internal', 'default', or the file name of
diff --git a/libartbase/Android.bp b/libartbase/Android.bp
index 77ff6b2a64..c3a63643e4 100644
--- a/libartbase/Android.bp
+++ b/libartbase/Android.bp
@@ -28,7 +28,6 @@ cc_defaults {
         "base/enums.cc",
         "base/file_magic.cc",
         "base/file_utils.cc",
-        "base/flags.cc",
         "base/hex_dump.cc",
         "base/hiddenapi_flags.cc",
         "base/logging.cc",
diff --git a/libartbase/base/flags.cc b/libartbase/base/flags.cc
deleted file mode 100644
index 4320c954a8..0000000000
--- a/libartbase/base/flags.cc
+++ /dev/null
@@ -1,122 +0,0 @@
-/*
- * Copyright (C) 2021 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#include "flags.h"
-
-#include <algorithm>
-
-#include "android-base/parsebool.h"
-#include "android-base/parseint.h"
-#include "android-base/properties.h"
-
-#pragma clang diagnostic push
-#pragma clang diagnostic error "-Wconversion"
-
-namespace {
-constexpr const char* kUndefinedValue = "UNSET";
-
-// The various ParseValue functions store the parsed value into *destination. If parsing fails for
-// some reason, ParseValue makes no changes to *destination.
-
-void ParseValue(const std::string_view value, std::optional<bool>* destination) {
-  switch (::android::base::ParseBool(value)) {
-    case ::android::base::ParseBoolResult::kError:
-      return;
-    case ::android::base::ParseBoolResult::kTrue:
-      *destination = true;
-      return;
-    case ::android::base::ParseBoolResult::kFalse:
-      *destination = false;
-      return;
-  }
-}
-
-void ParseValue(const std::string_view value, std::optional<int>* destination) {
-  int parsed_value = 0;
-  if (::android::base::ParseInt(std::string{value}, &parsed_value)) {
-    *destination = parsed_value;
-  }
-}
-
-void ParseValue(const std::string_view value, std::optional<std::string>* destination) {
-  *destination = value;
-}
-
-}  // namespace
-
-namespace art {
-
-template <>
-std::forward_list<FlagBase*> FlagBase::ALL_FLAGS{};
-
-// gFlags must be defined after FlagBase::ALL_FLAGS so the constructors run in the right order.
-Flags gFlags;
-
-template <typename Value>
-Flag<Value>::Flag(const std::string& name, Value default_value) : default_{default_value} {
-  command_line_argument_name_ = "-X" + name + "=_";
-  std::replace(command_line_argument_name_.begin(), command_line_argument_name_.end(), '.', '-');
-  system_property_name_ = "dalvik.vm." + name;
-
-  ALL_FLAGS.push_front(this);
-}
-
-template <typename Value>
-Value Flag<Value>::operator()() {
-  std::optional<Value> value{GetOptional()};
-  if (value.has_value()) {
-    return value.value();
-  }
-  return default_;
-}
-
-template <typename Value>
-std::optional<Value> Flag<Value>::GetOptional() {
-  if (from_command_line_.has_value()) {
-    return from_command_line_.value();
-  }
-  // If the value comes from the command line, there's no point in checking system properties or the
-  // server settings.
-  if (!initialized_) {
-    Reload();
-  }
-  if (from_system_property_.has_value()) {
-    return from_system_property_.value();
-  }
-  return std::nullopt;
-}
-
-template <typename Value>
-void Flag<Value>::Reload() {
-  initialized_ = true;
-  // Command line argument cannot be reloaded. It must be set during initial command line parsing.
-
-  // Check system properties
-  from_system_property_ = std::nullopt;
-  const std::string sysprop = ::android::base::GetProperty(system_property_name_, kUndefinedValue);
-  if (sysprop != kUndefinedValue) {
-    ParseValue(sysprop, &from_system_property_);
-    return;
-  }
-}
-
-template class Flag<bool>;
-template class Flag<int>;
-template class Flag<std::string>;
-
-}  // namespace art
-
-#pragma clang diagnostic pop  // -Wconversion
diff --git a/libartbase/base/flags.h b/libartbase/base/flags.h
deleted file mode 100644
index 6dd5898945..0000000000
--- a/libartbase/base/flags.h
+++ /dev/null
@@ -1,150 +0,0 @@
-/*
- * Copyright (C) 2021 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#ifndef ART_LIBARTBASE_BASE_FLAGS_H_
-#define ART_LIBARTBASE_BASE_FLAGS_H_
-
-#include <forward_list>
-#include <optional>
-#include <string>
-#include <variant>
-
-// This file defines a set of flags that can be used to enable/disable features within ART or
-// otherwise tune ART's behavior. Flags can be set through command line options, system properties,
-// or default values. This flexibility enables easier development and also larger experiments.
-//
-// The flags are defined in the Flags struct near the bottom of the file. To define a new flag, add
-// a Flag field to the struct. Then to read the value of the flag, use gFlag.MyNewFlag().
-
-#pragma clang diagnostic push
-#pragma clang diagnostic error "-Wconversion"
-
-namespace art {
-
-// FlagMetaBase handles automatically adding flags to the command line parser. It is parameterized
-// by all supported flag types. In general, this should be treated as though it does not exist and
-// FlagBase, which is already specialized to the types we support, should be used instead.
-template <typename... T>
-class FlagMetaBase {
- public:
-  virtual ~FlagMetaBase() {}
-
-  template <typename Builder>
-  static void AddFlagsToCmdlineParser(Builder* builder) {
-    for (auto* flag : ALL_FLAGS) {
-      // Each flag can return a pointer to where its command line value is stored. Because these can
-      // be different types, the return value comes as a variant. The cases list below contains a
-      // lambda that is specialized to handle each branch of the variant and call the correct
-      // methods on the command line parser builder.
-      FlagValuePointer location = flag->GetLocation();
-      auto cases = {std::function<void()>([&]() {
-        if (std::holds_alternative<std::optional<T>*>(location)) {
-          builder = &builder->Define(flag->command_line_argument_name_.c_str())
-                         .template WithType<T>()
-                         .IntoLocation(std::get<std::optional<T>*>(location));
-        }
-      })...};
-      for (auto c : cases) {
-        c();
-      }
-    }
-  }
-
- protected:
-  using FlagValuePointer = std::variant<std::optional<T>*...>;
-  static std::forward_list<FlagMetaBase<T...>*> ALL_FLAGS;
-
-  std::string command_line_argument_name_;
-  std::string system_property_name_;
-
-  virtual FlagValuePointer GetLocation() = 0;
-};
-
-using FlagBase = FlagMetaBase<bool, int, std::string>;
-
-template <>
-std::forward_list<FlagBase*> FlagBase::ALL_FLAGS;
-
-// This class defines a flag with a value of a particular type.
-template <typename Value>
-class Flag : public FlagBase {
- public:
-  // Create a new Flag. The name parameter is used to generate the names from the various parameter
-  // sources. See the documentation on the Flags struct for an example.
-  explicit Flag(const std::string& name, Value default_value = {});
-  virtual ~Flag() {}
-
-  // Returns the value of the flag.
-  //
-  // The value returned will be the command line argument, if present, otherwise the
-  // server-configured value, if present, otherwise the system property value, if present, and
-  // finally, the default value.
-  Value operator()();
-
-  // Returns the value of the flag or an empty option if it was not set.
-  std::optional<Value> GetOptional();
-
-  // Reload the system property values. In general this should not be used directly, but it can be
-  // used to support reloading the value without restarting the device.
-  void Reload();
-
- protected:
-  FlagValuePointer GetLocation() override { return &from_command_line_; }
-
- private:
-  bool initialized_{false};
-  const Value default_;
-  std::optional<Value> from_command_line_;
-  std::optional<Value> from_system_property_;
-};
-
-// This struct contains the list of ART flags. Flags are parameterized by the type of value they
-// support (bool, int, string, etc.). In addition to field name, flags have a name for the parameter
-// as well.
-//
-// Example:
-//
-//     Flag<bool> WriteMetricsToLog{"metrics.write-to-log", false};
-//
-// This creates a boolean flag that can be read through gFlags.WriteMetricsToLog(). The default
-// value is false. Note that the default value can be left unspecified, in which the value of the
-// type's default constructor will be used.
-//
-// The flag can be set through the following generated means:
-//
-// Command Line:
-//
-//     -Xmetrics-write-to-log=true
-//
-// System Property:
-//
-//     setprop dalvik.vm.metrics.write-to-log true
-struct Flags {
-  Flag<bool> WriteMetricsToLog{"metrics.write-to-log", false};
-  Flag<bool> WriteMetricsToStatsd{"metrics.write-to-statsd", false};
-  Flag<bool> ReportMetricsOnShutdown{"metrics.report-on-shutdown", true};
-  Flag<int> MetricsReportingPeriod{"metrics.reporting-period"};
-  Flag<std::string> WriteMetricsToFile{"metrics.write-to-file"};
-};
-
-// This is the actual instance of all the flags.
-extern Flags gFlags;
-
-}  // namespace art
-
-#pragma clang diagnostic pop  // -Wconversion
-
-#endif  // ART_LIBARTBASE_BASE_FLAGS_H_
diff --git a/runtime/metrics_reporter.cc b/runtime/metrics_reporter.cc
index 6ff182bac4..2004c7d918 100644
--- a/runtime/metrics_reporter.cc
+++ b/runtime/metrics_reporter.cc
@@ -16,7 +16,6 @@
 
 #include "metrics_reporter.h"
 
-#include "base/flags.h"
 #include "runtime.h"
 #include "runtime_options.h"
 #include "thread-current-inl.h"
@@ -130,13 +129,13 @@ void MetricsReporter::ReportMetrics() const {
   }
 }
 
-ReportingConfig ReportingConfig::FromFlags() {
+ReportingConfig ReportingConfig::FromRuntimeArguments(const RuntimeArgumentMap& args) {
+  using M = RuntimeArgumentMap;
   return {
-      .dump_to_logcat = gFlags.WriteMetricsToLog(),
-      .dump_to_file = gFlags.WriteMetricsToFile.GetOptional(),
-      .dump_to_statsd = gFlags.WriteMetricsToStatsd(),
-      .report_metrics_on_shutdown = gFlags.ReportMetricsOnShutdown(),
-      .periodic_report_seconds = gFlags.MetricsReportingPeriod.GetOptional(),
+      .dump_to_logcat = args.Exists(M::WriteMetricsToLog),
+      .dump_to_file = args.GetOptional(M::WriteMetricsToFile),
+      .report_metrics_on_shutdown = !args.Exists(M::DisableFinalMetricsReport),
+      .periodic_report_seconds = args.GetOptional(M::MetricsReportingPeriod),
   };
 }
 
diff --git a/runtime/metrics_reporter.h b/runtime/metrics_reporter.h
index 4b8afe6955..a9cd1f381d 100644
--- a/runtime/metrics_reporter.h
+++ b/runtime/metrics_reporter.h
@@ -28,7 +28,7 @@ namespace metrics {
 
 // Defines the set of options for how metrics reporting happens.
 struct ReportingConfig {
-  static ReportingConfig FromFlags();
+  static ReportingConfig FromRuntimeArguments(const RuntimeArgumentMap& args);
 
   // Causes metrics to be written to the log, which makes them show up in logcat.
   bool dump_to_logcat{false};
@@ -36,8 +36,6 @@ struct ReportingConfig {
   // If set, provides a file name to enable metrics logging to a file.
   std::optional<std::string> dump_to_file;
 
-  bool dump_to_statsd{false};
-
   // Indicates whether to report the final state of metrics on shutdown.
   //
   // Note that reporting only happens if some output, such as logcat, is enabled.
@@ -47,9 +45,7 @@ struct ReportingConfig {
   std::optional<unsigned int> periodic_report_seconds;
 
   // Returns whether any options are set that enables metrics reporting.
-  constexpr bool ReportingEnabled() const {
-    return dump_to_logcat || dump_to_file.has_value() || dump_to_statsd;
-  }
+  constexpr bool ReportingEnabled() const { return dump_to_logcat || dump_to_file.has_value(); }
 };
 
 // MetricsReporter handles periodically reporting ART metrics.
diff --git a/runtime/parsed_options.cc b/runtime/parsed_options.cc
index 0b6ba6f20c..b7a2b66277 100644
--- a/runtime/parsed_options.cc
+++ b/runtime/parsed_options.cc
@@ -23,7 +23,6 @@
 #include <android-base/strings.h>
 
 #include "base/file_utils.h"
-#include "base/flags.h"
 #include "base/indenter.h"
 #include "base/macros.h"
 #include "base/utils.h"
@@ -395,6 +394,20 @@ std::unique_ptr<RuntimeParser> ParsedOptions::MakeParser(bool ignore_unrecognize
           .IntoKey(M::CorePlatformApiPolicy)
       .Define("-Xuse-stderr-logger")
           .IntoKey(M::UseStderrLogger)
+      .Define("-Xwrite-metrics-to-log")
+          .WithHelp("Enables writing ART metrics to logcat")
+          .IntoKey(M::WriteMetricsToLog)
+      .Define("-Xwrite-metrics-to-file=_")
+          .WithHelp("Enables writing ART metrics to the given file")
+          .WithType<std::string>()
+          .IntoKey(M::WriteMetricsToFile)
+      .Define("-Xdisable-final-metrics-report")
+          .WithHelp("Disables reporting metrics when ART shuts down")
+          .IntoKey(M::DisableFinalMetricsReport)
+      .Define("-Xmetrics-reporting-period=_")
+          .WithHelp("The time in seconds between metrics reports")
+          .WithType<unsigned int>()
+          .IntoKey(M::MetricsReportingPeriod)
       .Define("-Xonly-use-system-oat-files")
           .IntoKey(M::OnlyUseSystemOatFiles)
       .Define("-Xverifier-logging-threshold=_")
@@ -429,11 +442,8 @@ std::unique_ptr<RuntimeParser> ParsedOptions::MakeParser(bool ignore_unrecognize
       .Define("-XX:PerfettoJavaHeapStackProf=_")
           .WithType<bool>()
           .WithValueMap({{"false", false}, {"true", true}})
-          .IntoKey(M::PerfettoJavaHeapStackProf);
-
-      FlagBase::AddFlagsToCmdlineParser(parser_builder.get());
-
-      parser_builder->Ignore({
+          .IntoKey(M::PerfettoJavaHeapStackProf)
+      .Ignore({
           "-ea", "-da", "-enableassertions", "-disableassertions", "--runtime-arg", "-esa",
           "-dsa", "-enablesystemassertions", "-disablesystemassertions", "-Xrs", "-Xint:_",
           "-Xdexopt:_", "-Xnoquithandler", "-Xjnigreflimit:_", "-Xgenregmap", "-Xnogenregmap",
diff --git a/runtime/runtime.cc b/runtime/runtime.cc
index b174f2a5c2..3c187d3a83 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -1736,7 +1736,7 @@ bool Runtime::Init(RuntimeArgumentMap&& runtime_options_in) {
   // Class-roots are setup, we can now finish initializing the JniIdManager.
   GetJniIdManager()->Init(self);
 
-  InitMetrics();
+  InitMetrics(runtime_options);
 
   // Runtime initialization is largely done now.
   // We load plugins first since that can modify the runtime state slightly.
@@ -1844,8 +1844,8 @@ bool Runtime::Init(RuntimeArgumentMap&& runtime_options_in) {
   return true;
 }
 
-void Runtime::InitMetrics() {
-  auto metrics_config = metrics::ReportingConfig::FromFlags();
+void Runtime::InitMetrics(const RuntimeArgumentMap& runtime_options) {
+  auto metrics_config = metrics::ReportingConfig::FromRuntimeArguments(runtime_options);
   if (metrics_config.ReportingEnabled()) {
     metrics_reporter_ = metrics::MetricsReporter::Create(metrics_config, this);
   }
diff --git a/runtime/runtime.h b/runtime/runtime.h
index c0a880ed75..b00ab6c246 100644
--- a/runtime/runtime.h
+++ b/runtime/runtime.h
@@ -978,7 +978,7 @@ class Runtime {
       SHARED_TRYLOCK_FUNCTION(true, Locks::mutator_lock_);
   void InitNativeMethods() REQUIRES(!Locks::mutator_lock_);
   void RegisterRuntimeNativeMethods(JNIEnv* env);
-  void InitMetrics();
+  void InitMetrics(const RuntimeArgumentMap& runtime_options);
 
   void StartDaemonThreads();
   void StartSignalCatcher();
diff --git a/runtime/runtime_options.def b/runtime/runtime_options.def
index 3b6c0fb768..1961113aff 100644
--- a/runtime/runtime_options.def
+++ b/runtime/runtime_options.def
@@ -186,4 +186,10 @@ RUNTIME_OPTIONS_KEY (bool,                PerfettoHprof,                  false)
 // This is to enable/disable Perfetto Java Heap Stack Profiling
 RUNTIME_OPTIONS_KEY (bool,                PerfettoJavaHeapStackProf,      false)
 
+// Whether to dump ART metrics to logcat
+RUNTIME_OPTIONS_KEY (Unit,                WriteMetricsToLog)
+RUNTIME_OPTIONS_KEY (std::string,         WriteMetricsToFile)
+RUNTIME_OPTIONS_KEY (Unit,                DisableFinalMetricsReport)
+RUNTIME_OPTIONS_KEY (unsigned int,        MetricsReportingPeriod)
+
 #undef RUNTIME_OPTIONS_KEY
diff --git a/test/2232-write-metrics-to-log/run b/test/2232-write-metrics-to-log/run
index 4d357e045e..b170317971 100755
--- a/test/2232-write-metrics-to-log/run
+++ b/test/2232-write-metrics-to-log/run
@@ -15,4 +15,4 @@
 # limitations under the License.
 
 export ANDROID_LOG_TAGS="*:i"
-exec ${RUN} $@ --external-log-tags --runtime-option -Xmetrics-write-to-log=true
+exec ${RUN} $@ --external-log-tags --runtime-option -Xwrite-metrics-to-log