Allow LOG_DCHECK to be configured via Feature in SyzyASAN builds.
This CL supports an experiment to enable DCHECKs in some Canary builds,
in order to gather data on broken DCHECK'd invariants, e.g. due to
faulty calling code or incorrect/over-zealous DCHECKing.
This CL changes the LOG_DCHECK LogSeverity to be switchable at run-
time in SyzyASAN builds with DCHECKs enabled, based on the setting of
a "DcheckIsFatal" Feature.
The Feature defaults to off, with an exception for death-test child
processes to allow death-tests to continue to work without modification.
This means that DCHECKs will be compiled-in and generate INFO level log
output, and can be switched via the command-line, or via Finch
experiment, to be made fatal.
Bug: 596231
Change-Id: I390aecdba40c8a8b396d6a4a03162ff071e9c3d5
Reviewed-on: https://chromium-review.googlesource.com/559809
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alexei Svitkine (very slow) <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497183}
CrOS-Libchrome-Original-Commit: 289477f6c6cfc2ca788955623aede8ed02c08012
diff --git a/base/feature_list.cc b/base/feature_list.cc
index 61043ce..ce4503b 100644
--- a/base/feature_list.cc
+++ b/base/feature_list.cc
@@ -276,6 +276,22 @@
// Note: Intentional leak of global singleton.
g_instance = instance.release();
+
+#if DCHECK_IS_ON() && defined(SYZYASAN)
+ // Update the behaviour of LOG_DCHECK to match the Feature configuration.
+ // DCHECK is also forced to be FATAL if we are running a death-test.
+ // TODO(asvitkine): If we find other use-cases that need integrating here
+ // then define a proper API/hook for the purpose.
+ constexpr base::Feature kDCheckIsFatalFeature{
+ "DcheckIsFatal", base::FEATURE_DISABLED_BY_DEFAULT};
+ if (base::FeatureList::IsEnabled(kDCheckIsFatalFeature) ||
+ base::CommandLine::ForCurrentProcess()->HasSwitch(
+ "gtest_internal_run_death_test")) {
+ logging::LOG_DCHECK = logging::LOG_FATAL;
+ } else {
+ logging::LOG_DCHECK = logging::LOG_INFO;
+ }
+#endif // DCHECK_IS_ON() && defined(SYZYASAN)
}
// static
diff --git a/base/logging.cc b/base/logging.cc
index 92aa256..c0f318c 100644
--- a/base/logging.cc
+++ b/base/logging.cc
@@ -351,6 +351,13 @@
} // namespace
+#if DCHECK_IS_ON() && defined(SYZYASAN)
+// In DCHECK-enabled SyzyASAN builds, allow the meaning of LOG_DCHECK to be
+// determined at run-time. We default it to INFO, to avoid it triggering
+// crashes before the run-time has explicitly chosen the behaviour.
+BASE_EXPORT logging::LogSeverity LOG_DCHECK = LOG_INFO;
+#endif
+
// This is never instantiated, it's just used for EAT_STREAM_PARAMETERS to have
// an object of the correct type on the LHS of the unused part of the ternary
// operator.
diff --git a/base/logging.h b/base/logging.h
index 3267ad5..66c1e8a 100644
--- a/base/logging.h
+++ b/base/logging.h
@@ -365,17 +365,15 @@
::logging::ClassName(__FILE__, __LINE__, ::logging::LOG_FATAL, ##__VA_ARGS__)
#define COMPACT_GOOGLE_LOG_EX_DFATAL(ClassName, ...) \
::logging::ClassName(__FILE__, __LINE__, ::logging::LOG_DFATAL, ##__VA_ARGS__)
+#define COMPACT_GOOGLE_LOG_EX_DCHECK(ClassName, ...) \
+ ::logging::ClassName(__FILE__, __LINE__, ::logging::LOG_DCHECK, ##__VA_ARGS__)
-#define COMPACT_GOOGLE_LOG_INFO \
- COMPACT_GOOGLE_LOG_EX_INFO(LogMessage)
-#define COMPACT_GOOGLE_LOG_WARNING \
- COMPACT_GOOGLE_LOG_EX_WARNING(LogMessage)
-#define COMPACT_GOOGLE_LOG_ERROR \
- COMPACT_GOOGLE_LOG_EX_ERROR(LogMessage)
-#define COMPACT_GOOGLE_LOG_FATAL \
- COMPACT_GOOGLE_LOG_EX_FATAL(LogMessage)
-#define COMPACT_GOOGLE_LOG_DFATAL \
- COMPACT_GOOGLE_LOG_EX_DFATAL(LogMessage)
+#define COMPACT_GOOGLE_LOG_INFO COMPACT_GOOGLE_LOG_EX_INFO(LogMessage)
+#define COMPACT_GOOGLE_LOG_WARNING COMPACT_GOOGLE_LOG_EX_WARNING(LogMessage)
+#define COMPACT_GOOGLE_LOG_ERROR COMPACT_GOOGLE_LOG_EX_ERROR(LogMessage)
+#define COMPACT_GOOGLE_LOG_FATAL COMPACT_GOOGLE_LOG_EX_FATAL(LogMessage)
+#define COMPACT_GOOGLE_LOG_DFATAL COMPACT_GOOGLE_LOG_EX_DFATAL(LogMessage)
+#define COMPACT_GOOGLE_LOG_DCHECK COMPACT_GOOGLE_LOG_EX_DCHECK(LogMessage)
#if defined(OS_WIN)
// wingdi.h defines ERROR to be 0. When we call LOG(ERROR), it gets
@@ -817,17 +815,15 @@
#if DCHECK_IS_ON()
-#define COMPACT_GOOGLE_LOG_EX_DCHECK(ClassName, ...) \
- COMPACT_GOOGLE_LOG_EX_FATAL(ClassName, ##__VA_ARGS__)
-#define COMPACT_GOOGLE_LOG_DCHECK COMPACT_GOOGLE_LOG_FATAL
+#if defined(SYZYASAN)
+BASE_EXPORT extern LogSeverity LOG_DCHECK;
+#else
const LogSeverity LOG_DCHECK = LOG_FATAL;
+#endif
#else // DCHECK_IS_ON()
-// These are just dummy values.
-#define COMPACT_GOOGLE_LOG_EX_DCHECK(ClassName, ...) \
- COMPACT_GOOGLE_LOG_EX_INFO(ClassName , ##__VA_ARGS__)
-#define COMPACT_GOOGLE_LOG_DCHECK COMPACT_GOOGLE_LOG_INFO
+// This is a dummy value, since the DCHECK implementation is a no-op.
const LogSeverity LOG_DCHECK = LOG_INFO;
#endif // DCHECK_IS_ON()
diff --git a/base/logging_unittest.cc b/base/logging_unittest.cc
index 59fdc2b..7418ec4 100644
--- a/base/logging_unittest.cc
+++ b/base/logging_unittest.cc
@@ -8,6 +8,7 @@
#include "base/compiler_specific.h"
#include "base/macros.h"
#include "base/strings/string_piece.h"
+#include "base/test/scoped_feature_list.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
@@ -419,11 +420,11 @@
EXPECT_EQ(0, g_log_sink_call_count);
DCHECK(false);
- EXPECT_EQ(DCHECK_IS_ON() ? 1 : 0, g_log_sink_call_count);
+ EXPECT_EQ(LOG_DCHECK == LOG_FATAL ? 1 : 0, g_log_sink_call_count);
DPCHECK(false);
- EXPECT_EQ(DCHECK_IS_ON() ? 2 : 0, g_log_sink_call_count);
+ EXPECT_EQ(LOG_DCHECK == LOG_FATAL ? 2 : 0, g_log_sink_call_count);
DCHECK_EQ(0, 1);
- EXPECT_EQ(DCHECK_IS_ON() ? 3 : 0, g_log_sink_call_count);
+ EXPECT_EQ(LOG_DCHECK == LOG_FATAL ? 3 : 0, g_log_sink_call_count);
// Test DCHECK on std::nullptr_t
g_log_sink_call_count = 0;
@@ -440,7 +441,7 @@
DCHECK_EQ(Animal::DOG, Animal::DOG);
EXPECT_EQ(0, g_log_sink_call_count);
DCHECK_EQ(Animal::DOG, Animal::CAT);
- EXPECT_EQ(DCHECK_IS_ON() ? 1 : 0, g_log_sink_call_count);
+ EXPECT_EQ(LOG_DCHECK == LOG_FATAL ? 1 : 0, g_log_sink_call_count);
// Test DCHECK on functions and function pointers.
g_log_sink_call_count = 0;
@@ -463,9 +464,9 @@
DCHECK_EQ(mp2, &MemberFunctions::MemberFunction2);
EXPECT_EQ(0, g_log_sink_call_count);
DCHECK_EQ(fp1, fp2);
- EXPECT_EQ(DCHECK_IS_ON() ? 1 : 0, g_log_sink_call_count);
+ EXPECT_EQ(LOG_DCHECK == LOG_FATAL ? 1 : 0, g_log_sink_call_count);
DCHECK_EQ(mp2, &MemberFunctions::MemberFunction1);
- EXPECT_EQ(DCHECK_IS_ON() ? 2 : 0, g_log_sink_call_count);
+ EXPECT_EQ(LOG_DCHECK == LOG_FATAL ? 2 : 0, g_log_sink_call_count);
}
TEST_F(LoggingTest, DcheckReleaseBehavior) {
@@ -557,6 +558,59 @@
}
} // namespace nested_test
+#if DCHECK_IS_ON() && defined(SYZYASAN)
+TEST_F(LoggingTest, AsanConditionalDCheck) {
+ // Verify that DCHECKs default to non-fatal in SyzyASAN builds.
+ // Note that we require only that DCHECK is non-fatal by default, rather
+ // than requiring that it be exactly INFO, ERROR, etc level.
+ EXPECT_LT(LOG_DCHECK, LOG_FATAL);
+ DCHECK(false);
+
+ // Verify that DCHECK* aren't hard-wired to crash on failure.
+ LOG_DCHECK = LOG_INFO;
+ DCHECK(false);
+ DCHECK_EQ(1, 2);
+
+ // Verify that DCHECK does crash if LOG_DCHECK is set to LOG_FATAL.
+ LOG_DCHECK = LOG_FATAL;
+
+ ::testing::StrictMock<MockLogAssertHandler> handler;
+ EXPECT_CALL(handler, HandleLogAssert(_, _, _, _)).Times(2);
+ {
+ logging::ScopedLogAssertHandler scoped_handler_b(base::Bind(
+ &MockLogAssertHandler::HandleLogAssert, base::Unretained(&handler)));
+ DCHECK(false);
+ DCHECK_EQ(1, 2);
+ }
+}
+
+TEST_F(LoggingTest, AsanConditionalDCheckFeature) {
+ // Enable fatal DCHECKs, so that preconditions in
+ // Initialize FeatureList with and without DcheckIsFatal, and verify the
+ // value of LOG_DCHECK. Note that we don't require that DCHECK take a
+ // specific value when the feature is off, only that it is non-fatal.
+
+ {
+ base::test::ScopedFeatureList feature_list;
+ feature_list.InitFromCommandLine("DcheckIsFatal", "");
+ EXPECT_EQ(LOG_DCHECK, LOG_FATAL);
+ }
+
+ {
+ base::test::ScopedFeatureList feature_list;
+ feature_list.InitFromCommandLine("", "DcheckIsFatal");
+ EXPECT_LT(LOG_DCHECK, LOG_FATAL);
+ }
+
+ // The default case is last, so we leave LOG_DCHECK in the default state.
+ {
+ base::test::ScopedFeatureList feature_list;
+ feature_list.InitFromCommandLine("", "");
+ EXPECT_LT(LOG_DCHECK, LOG_FATAL);
+ }
+}
+#endif // DCHECK_IS_ON() && defined(SYZYASAN)
+
} // namespace
} // namespace logging