Gabe Black has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/45007 )
Change subject: base,python: Simplify how we check if a debug flag is
enabled.
......................................................................
base,python: Simplify how we check if a debug flag is enabled.
Compound debug flags are intended to be a way to enable or disable a
group of simple debug flags at once, so that you don't need to enumerate
every more specialized flag in an area to get a broad amount of
debugging, nor do you give up the ability to select a general area
easily by making more specific flags.
It doesn't, however, make a lot of sense to check the value of a
compound debug flag, since it could be enabled but then have individual
subflags disabled. Exactly whether that means the compound flag should
be enabled or not is not clear, and figuring it out takes a fair amount
of work since each member simple flag needs to be visited.
Also, by having different behavior depending on the flag type, the
"enabled" method needed to be virtual.
This change eliminates the virtual method and moves the _tracing bool
member into the base class. If a subclass (only SimpleFlag currently)
wants to start or stop tracing based on itself, it should set or clear
this flag. Also, the "enabled" method has been renamed to "tracing",
since that's actually what it tracked. Being enabled by itself is not
sufficient to be tracing since there is also a global enable.
Finally, rather than duplicate the logic to convert a flag to bool in
the python wrapper, we can just use a cast to bool and take advantage of
the version in C++.
Change-Id: I3dc64c2364f0239294093686ddac6fcc8441f306
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/45007
Tested-by: kokoro <[email protected]>
Reviewed-by: Gabe Black <[email protected]>
Maintainer: Gabe Black <[email protected]>
---
M src/base/debug.cc
M src/base/debug.hh
M src/base/debug.test.cc
M src/python/pybind11/debug.cc
4 files changed, 64 insertions(+), 81 deletions(-)
Approvals:
Gabe Black: Looks good to me, approved; Looks good to me, approved
kokoro: Regressions pass
diff --git a/src/base/debug.cc b/src/base/debug.cc
index 38eb25f..34bc4f4 100644
--- a/src/base/debug.cc
+++ b/src/base/debug.cc
@@ -137,20 +137,6 @@
}
bool
-CompoundFlag::enabled() const
-{
- if (_kids.empty())
- return false;
-
- for (auto& k : _kids) {
- if (!k->enabled())
- return false;
- }
-
- return true;
-}
-
-bool
changeFlag(const char *s, bool value)
{
Flag *f = findFlag(s);
@@ -188,7 +174,7 @@
FlagsMap::iterator end = allFlags().end();
for (; i != end; ++i) {
SimpleFlag *f = dynamic_cast<SimpleFlag *>(i->second);
- if (f && f->enabled())
+ if (f && f->tracing())
ccprintf(os, "%s\n", f->name());
}
}
diff --git a/src/base/debug.hh b/src/base/debug.hh
index 4b77e16..f5bfd91 100644
--- a/src/base/debug.hh
+++ b/src/base/debug.hh
@@ -57,6 +57,8 @@
protected:
static bool _globalEnable; // whether debug tracings are enabled
+ bool _tracing = false; // tracing is enabled and flag is on
+
const char *_name;
const char *_desc;
@@ -69,11 +71,12 @@
std::string name() const { return _name; }
std::string desc() const { return _desc; }
+ bool tracing() const { return _tracing; }
+
virtual void enable() = 0;
virtual void disable() = 0;
- virtual bool enabled() const = 0;
- operator bool() const { return enabled(); }
+ operator bool() const { return tracing(); }
static void globalEnable();
static void globalDisable();
@@ -85,7 +88,6 @@
/** Whether this flag changes debug formatting. */
const bool _isFormat = false;
- bool _tracing = false; // tracing is enabled and flag is on
bool _enabled = false; // flag enablement status
void sync() override { _tracing = _globalEnable && _enabled; }
@@ -95,8 +97,6 @@
: Flag(name, desc), _isFormat(is_format)
{}
- bool enabled() const override { return _tracing; }
-
void enable() override { _enabled = true; sync(); }
void disable() override { _enabled = false; sync(); }
@@ -127,7 +127,6 @@
void enable() override;
void disable() override;
- bool enabled() const override;
};
typedef std::map<std::string, Flag *> FlagsMap;
diff --git a/src/base/debug.test.cc b/src/base/debug.test.cc
index 22320d1..69ac095 100644
--- a/src/base/debug.test.cc
+++ b/src/base/debug.test.cc
@@ -78,23 +78,23 @@
Debug::SimpleFlag flag("SimpleFlagEnabledTest", "");
// By default flags are initialized disabled
- ASSERT_FALSE(flag.enabled());
+ ASSERT_FALSE(flag.tracing());
// Flags must be globally enabled before individual flags are enabled
flag.enable();
- ASSERT_FALSE(flag.enabled());
+ ASSERT_FALSE(flag.tracing());
Debug::Flag::globalEnable();
- ASSERT_TRUE(flag.enabled());
+ ASSERT_TRUE(flag.tracing());
// Verify that the global enabler works
Debug::Flag::globalDisable();
- ASSERT_FALSE(flag.enabled());
+ ASSERT_FALSE(flag.tracing());
Debug::Flag::globalEnable();
- ASSERT_TRUE(flag.enabled());
+ ASSERT_TRUE(flag.tracing());
// Test disabling the flag with global enabled
flag.disable();
- ASSERT_FALSE(flag.enabled());
+ ASSERT_FALSE(flag.tracing());
}
/**
@@ -110,29 +110,28 @@
{&flag_a, &flag_b});
// By default flags are initialized disabled
- ASSERT_FALSE(flag.enabled());
+ ASSERT_FALSE(flag.tracing());
// Flags must be globally enabled before individual flags are enabled
flag.enable();
- ASSERT_FALSE(flag_a.enabled());
- ASSERT_FALSE(flag_b.enabled());
- ASSERT_FALSE(flag.enabled());
+ ASSERT_FALSE(flag_a.tracing());
+ ASSERT_FALSE(flag_b.tracing());
+ ASSERT_FALSE(flag.tracing());
Debug::Flag::globalEnable();
for (auto &kid : flag.kids()) {
- ASSERT_TRUE(kid->enabled());
+ ASSERT_TRUE(kid->tracing());
}
- ASSERT_TRUE(flag_a.enabled());
- ASSERT_TRUE(flag_b.enabled());
- ASSERT_TRUE(flag.enabled());
+ ASSERT_TRUE(flag_a.tracing());
+ ASSERT_TRUE(flag_b.tracing());
// Test disabling the flag with global enabled
flag.disable();
for (auto &kid : flag.kids()) {
- ASSERT_FALSE(kid->enabled());
+ ASSERT_FALSE(kid->tracing());
}
- ASSERT_FALSE(flag_a.enabled());
- ASSERT_FALSE(flag_b.enabled());
- ASSERT_FALSE(flag.enabled());
+ ASSERT_FALSE(flag_a.tracing());
+ ASSERT_FALSE(flag_b.tracing());
+ ASSERT_FALSE(flag.tracing());
}
/** Test that the conversion operator matches the enablement status. */
@@ -141,9 +140,9 @@
Debug::Flag::globalEnable();
Debug::SimpleFlag flag("FlagConversionOperatorTest", "");
- ASSERT_EQ(flag, flag.enabled());
+ ASSERT_EQ(flag, flag.tracing());
flag.enable();
- ASSERT_EQ(flag, flag.enabled());
+ ASSERT_EQ(flag, flag.tracing());
flag.disable();
}
@@ -160,28 +159,27 @@
{&flag_a, &flag_b});
// Test enabling only flag A
- ASSERT_FALSE(flag_a.enabled());
- ASSERT_FALSE(flag_b.enabled());
- ASSERT_FALSE(flag.enabled());
+ ASSERT_FALSE(flag_a.tracing());
+ ASSERT_FALSE(flag_b.tracing());
+ ASSERT_FALSE(flag.tracing());
flag_a.enable();
- ASSERT_TRUE(flag_a.enabled());
- ASSERT_FALSE(flag_b.enabled());
- ASSERT_FALSE(flag.enabled());
+ ASSERT_TRUE(flag_a.tracing());
+ ASSERT_FALSE(flag_b.tracing());
+ ASSERT_FALSE(flag.tracing());
// Test that enabling both flags enables the compound flag
- ASSERT_TRUE(flag_a.enabled());
- ASSERT_FALSE(flag_b.enabled());
- ASSERT_FALSE(flag.enabled());
+ ASSERT_TRUE(flag_a.tracing());
+ ASSERT_FALSE(flag_b.tracing());
+ ASSERT_FALSE(flag.tracing());
flag_b.enable();
- ASSERT_TRUE(flag_a.enabled());
- ASSERT_TRUE(flag_b.enabled());
- ASSERT_TRUE(flag.enabled());
+ ASSERT_TRUE(flag_a.tracing());
+ ASSERT_TRUE(flag_b.tracing());
// Test that disabling one of the flags disables the compound flag
flag_a.disable();
- ASSERT_FALSE(flag_a.enabled());
- ASSERT_TRUE(flag_b.enabled());
- ASSERT_FALSE(flag.enabled());
+ ASSERT_FALSE(flag_a.tracing());
+ ASSERT_TRUE(flag_b.tracing());
+ ASSERT_FALSE(flag.tracing());
}
/** Search for existent and non-existent flags. */
@@ -195,13 +193,13 @@
// enabled too
Debug::Flag *flag;
EXPECT_TRUE(flag = Debug::findFlag("FlagFindFlagTestA"));
- ASSERT_FALSE(flag_a.enabled());
+ ASSERT_FALSE(flag_a.tracing());
flag->enable();
- ASSERT_TRUE(flag_a.enabled());
+ ASSERT_TRUE(flag_a.tracing());
EXPECT_TRUE(flag = Debug::findFlag("FlagFindFlagTestB"));
- ASSERT_FALSE(flag_b.enabled());
+ ASSERT_FALSE(flag_b.tracing());
flag->enable();
- ASSERT_TRUE(flag_b.enabled());
+ ASSERT_TRUE(flag_b.tracing());
// Search for a non-existent flag
EXPECT_FALSE(Debug::findFlag("FlagFindFlagTestC"));
@@ -216,18 +214,18 @@
// Enable the found flags and verify that the original flags are
// enabled too
- ASSERT_FALSE(flag_a.enabled());
+ ASSERT_FALSE(flag_a.tracing());
EXPECT_TRUE(Debug::changeFlag("FlagChangeFlagTestA", true));
- ASSERT_TRUE(flag_a.enabled());
+ ASSERT_TRUE(flag_a.tracing());
EXPECT_TRUE(Debug::changeFlag("FlagChangeFlagTestA", false));
- ASSERT_FALSE(flag_a.enabled());
+ ASSERT_FALSE(flag_a.tracing());
// Disable and enable a flag
- ASSERT_FALSE(flag_b.enabled());
+ ASSERT_FALSE(flag_b.tracing());
EXPECT_TRUE(Debug::changeFlag("FlagChangeFlagTestB", false));
- ASSERT_FALSE(flag_b.enabled());
+ ASSERT_FALSE(flag_b.tracing());
EXPECT_TRUE(Debug::changeFlag("FlagChangeFlagTestB", true));
- ASSERT_TRUE(flag_b.enabled());
+ ASSERT_TRUE(flag_b.tracing());
// Change a non-existent flag
ASSERT_FALSE(Debug::changeFlag("FlagChangeFlagTestC", true));
@@ -241,18 +239,18 @@
Debug::SimpleFlag flag_b("FlagSetClearDebugFlagTestB", "");
// Enable and disable a flag
- ASSERT_FALSE(flag_a.enabled());
+ ASSERT_FALSE(flag_a.tracing());
setDebugFlag("FlagSetClearDebugFlagTestA");
- ASSERT_TRUE(flag_a.enabled());
+ ASSERT_TRUE(flag_a.tracing());
clearDebugFlag("FlagSetClearDebugFlagTestA");
- ASSERT_FALSE(flag_a.enabled());
+ ASSERT_FALSE(flag_a.tracing());
// Disable and enable a flag
- ASSERT_FALSE(flag_b.enabled());
+ ASSERT_FALSE(flag_b.tracing());
clearDebugFlag("FlagSetClearDebugFlagTestB");
- ASSERT_FALSE(flag_b.enabled());
+ ASSERT_FALSE(flag_b.tracing());
setDebugFlag("FlagSetClearDebugFlagTestB");
- ASSERT_TRUE(flag_b.enabled());
+ ASSERT_TRUE(flag_b.tracing());
// Change a non-existent flag
setDebugFlag("FlagSetClearDebugFlagTestC");
@@ -270,7 +268,7 @@
dumpDebugFlags();
std::string output = gtestLogOutput.str();
EXPECT_EQ(output, "");
- ASSERT_FALSE(flag.enabled());
+ ASSERT_FALSE(flag.tracing());
}
/** Test dumping enabled debug flags with a larger set of flags. */
@@ -288,11 +286,11 @@
{&flag_e});
// Enable a few flags
- ASSERT_FALSE(flag_a.enabled());
- ASSERT_FALSE(flag_b.enabled());
- ASSERT_FALSE(flag_c.enabled());
- ASSERT_FALSE(flag_d.enabled());
- ASSERT_FALSE(flag_e.enabled());
+ ASSERT_FALSE(flag_a.tracing());
+ ASSERT_FALSE(flag_b.tracing());
+ ASSERT_FALSE(flag_c.tracing());
+ ASSERT_FALSE(flag_d.tracing());
+ ASSERT_FALSE(flag_e.tracing());
flag_a.enable();
flag_c.enable();
compound_flag_b.enable();
diff --git a/src/python/pybind11/debug.cc b/src/python/pybind11/debug.cc
index a2b5406..cca0f9c 100644
--- a/src/python/pybind11/debug.cc
+++ b/src/python/pybind11/debug.cc
@@ -94,9 +94,9 @@
.def_property_readonly("desc", &Debug::Flag::desc)
.def("enable", &Debug::Flag::enable)
.def("disable", &Debug::Flag::disable)
- .def_property("enabled",
+ .def_property("tracing",
[](const Debug::Flag *flag) {
- return flag->enabled();
+ return flag->tracing();
},
[](Debug::Flag *flag, bool state) {
if (state) {
@@ -106,7 +106,7 @@
}
})
.def("__bool__", [](const Debug::Flag *flag) {
- return flag->enabled();
+ return (bool)*flag;
})
;
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/45007
To unsubscribe, or for help writing mail filters, visit
https://gem5-review.googlesource.com/settings
Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: I3dc64c2364f0239294093686ddac6fcc8441f306
Gerrit-Change-Number: 45007
Gerrit-PatchSet: 9
Gerrit-Owner: Gabe Black <[email protected]>
Gerrit-Reviewer: Andreas Sandberg <[email protected]>
Gerrit-Reviewer: Bobby R. Bruce <[email protected]>
Gerrit-Reviewer: Daniel Carvalho <[email protected]>
Gerrit-Reviewer: Gabe Black <[email protected]>
Gerrit-Reviewer: Giacomo Travaglini <[email protected]>
Gerrit-Reviewer: Jason Lowe-Power <[email protected]>
Gerrit-Reviewer: Jason Lowe-Power <[email protected]>
Gerrit-Reviewer: kokoro <[email protected]>
Gerrit-MessageType: merged
_______________________________________________
gem5-dev mailing list -- [email protected]
To unsubscribe send an email to [email protected]
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
