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
diff --git a/cmdline/cmdline_parser.h b/cmdline/cmdline_parser.h
index 7e343f8..22eb44c 100644
--- a/cmdline/cmdline_parser.h
+++ b/cmdline/cmdline_parser.h
@@ -210,24 +210,6 @@
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 7f01be6..2d7d5f1 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 @@
};
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 77ff6b2..c3a6364 100644
--- a/libartbase/Android.bp
+++ b/libartbase/Android.bp
@@ -28,7 +28,6 @@
"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 4320c95..0000000
--- 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 6dd5898..0000000
--- 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 6ff182b..2004c7d 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 @@
}
}
-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 4b8afe6..a9cd1f3 100644
--- a/runtime/metrics_reporter.h
+++ b/runtime/metrics_reporter.h
@@ -28,7 +28,7 @@
// 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 @@
// 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 @@
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 0b6ba6f..b7a2b66 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 @@
.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 @@
.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 b174f2a..3c187d3 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -1736,7 +1736,7 @@
// 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 @@
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 c0a880e..b00ab6c 100644
--- a/runtime/runtime.h
+++ b/runtime/runtime.h
@@ -978,7 +978,7 @@
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 3b6c0fb..1961113 100644
--- a/runtime/runtime_options.def
+++ b/runtime/runtime_options.def
@@ -186,4 +186,10 @@
// 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 4d357e0..b170317 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