Let DCHECK in non-official-release build be opt-in with dcheck_always…

…_on=1 only

- Remove DCHECK in non-official-release build by default
- Gyp variable dcheck_always_on=1 (existing) forces to enable DCHECK
  in release build
- Remove flag --enable-dcheck

Other effects/notes:
- Now allow "buildtype=Official dcheck_always_on=1" (which will
  enable DCHECK in official build) combination.
- Gyp variable logging_like_official_build no longer has an effect
- Leave DCHECK_IS_ON() unchanged. May deal with it in a later change
  if needed.

This won't affect bots which use dcheck_always_on=1.

BUG=350462
TEST=LoggingTest.Dcheck
R=thakis@chromium.org
TBR=darin,sehr (command line changes in components/nacl and mojo)

Review URL: https://codereview.chromium.org/189603007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@255987 0039d316-1c4b-4281-b951-d872f2087c98

base/base_switches.cc

@@ -20,9 +20,6 @@ const char kDisableBreakpad[]               = "disable-breakpad";
 // generated internally.
 const char kEnableCrashReporter[]           = "enable-crash-reporter";
 
-// Enable DCHECKs in release mode.
-const char kEnableDCHECK[]                  = "enable-dcheck";
-
 // Generates full memory crash dump.
 const char kFullMemoryCrashReport[]         = "full-memory-crash-report";
 

base/base_switches.h

@@ -14,7 +14,6 @@ namespace switches {
 extern const char kDebugOnStart[];
 extern const char kDisableBreakpad[];
 extern const char kEnableCrashReporter[];
-extern const char kEnableDCHECK[];
 extern const char kFullMemoryCrashReport[];
 extern const char kNoErrorDialogs[];
 extern const char kProfilerTiming[];

base/i18n/streaming_utf8_validator.cc

@@ -15,10 +15,7 @@ namespace base {
 namespace {
 
 uint8 StateTableLookup(uint8 offset) {
-  // Skip the bounds check on non-debug builds so that it isn't necessary to set
-  // LOGGING_IS_OFFICIAL_BUILD just to do a performance test.
-  if (logging::DEBUG_MODE)
-    DCHECK_LT(offset, internal::kUtf8ValidatorTablesSize);
+  DCHECK_LT(offset, internal::kUtf8ValidatorTablesSize);
   return internal::kUtf8ValidatorTables[offset];
 }
 

base/logging.cc

@@ -65,12 +65,6 @@ typedef pthread_mutex_t* MutexHandle;
 
 namespace logging {
 
-DcheckState g_dcheck_state = DISABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS;
-
-void set_dcheck_state(DcheckState state) {
-  g_dcheck_state = state;
-}
-
 namespace {
 
 VlogInfo* g_vlog_info = NULL;
@@ -358,15 +352,13 @@ LoggingSettings::LoggingSettings()
     : logging_dest(LOG_DEFAULT),
       log_file(NULL),
       lock_log(LOCK_LOG_FILE),
-      delete_old(APPEND_TO_OLD_LOG_FILE),
-      dcheck_state(DISABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS) {}
+      delete_old(APPEND_TO_OLD_LOG_FILE) {}
 
 bool BaseInitLoggingImpl(const LoggingSettings& settings) {
 #if defined(OS_NACL)
   // Can log only to the system debug log.
   CHECK_EQ(settings.logging_dest & ~LOG_TO_SYSTEM_DEBUG_LOG, 0);
 #endif
-  g_dcheck_state = settings.dcheck_state;
   CommandLine* command_line = CommandLine::ForCurrentProcess();
   // Don't bother initializing g_vlog_info unless we use one of the
   // vlog switches.

base/logging.h

@@ -181,19 +181,13 @@ enum LogLockingState { LOCK_LOG_FILE, DONT_LOCK_LOG_FILE };
 // Defaults to APPEND_TO_OLD_LOG_FILE.
 enum OldFileDeletionState { DELETE_OLD_LOG_FILE, APPEND_TO_OLD_LOG_FILE };
 
-enum DcheckState {
-  DISABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS,
-  ENABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS
-};
-
 struct BASE_EXPORT LoggingSettings {
   // The defaults values are:
   //
   //  logging_dest: LOG_DEFAULT
   //  log_file:     NULL
   //  lock_log:     LOCK_LOG_FILE
   //  delete_old:   APPEND_TO_OLD_LOG_FILE
-  //  dcheck_state: DISABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS
   LoggingSettings();
 
   LoggingDestination logging_dest;
@@ -203,8 +197,6 @@ struct BASE_EXPORT LoggingSettings {
   const PathChar* log_file;
   LogLockingState lock_log;
   OldFileDeletionState delete_old;
-
-  DcheckState dcheck_state;
 };
 
 // Define different names for the BaseInitLoggingImpl() function depending on
@@ -465,20 +457,6 @@ const LogSeverity LOG_0 = LOG_ERROR;
 #define PLOG_IF(severity, condition) \
   LAZY_STREAM(PLOG_STREAM(severity), LOG_IS_ON(severity) && (condition))
 
-#if !defined(NDEBUG)
-// Debug builds always include DCHECK and DLOG.
-#undef LOGGING_IS_OFFICIAL_BUILD
-#define LOGGING_IS_OFFICIAL_BUILD 0
-#elif defined(OFFICIAL_BUILD)
-// Official release builds always disable and remove DCHECK and DLOG.
-#undef LOGGING_IS_OFFICIAL_BUILD
-#define LOGGING_IS_OFFICIAL_BUILD 1
-#elif !defined(LOGGING_IS_OFFICIAL_BUILD)
-// Unless otherwise specified, unofficial release builds include
-// DCHECK and DLOG.
-#define LOGGING_IS_OFFICIAL_BUILD 0
-#endif
-
 // The actual stream used isn't important.
 #define EAT_STREAM_PARAMETERS                                           \
   true ? (void) 0 : ::logging::LogMessageVoidify() & LOG_STREAM(FATAL)
@@ -490,10 +468,10 @@ const LogSeverity LOG_0 = LOG_ERROR;
 // We make sure CHECK et al. always evaluates their arguments, as
 // doing CHECK(FunctionWithSideEffect()) is a common idiom.
 
-#if LOGGING_IS_OFFICIAL_BUILD
+#if defined(OFFICIAL_BUILD) && defined(NDEBUG)
 
 // Make all CHECK functions discard their log strings to reduce code
-// bloat for official builds.
+// bloat for official release builds.
 
 // TODO(akalin): This would be more valuable if there were some way to
 // remove BreakDebugger() from the backtrace, perhaps by turning it
@@ -590,21 +568,15 @@ DEFINE_CHECK_OP_IMPL(GT, > )
 #define CHECK_GE(val1, val2) CHECK_OP(GE, >=, val1, val2)
 #define CHECK_GT(val1, val2) CHECK_OP(GT, > , val1, val2)
 
-#if LOGGING_IS_OFFICIAL_BUILD
-// In order to have optimized code for official builds, remove DLOGs and
-// DCHECKs.
-#define ENABLE_DLOG 0
-#define ENABLE_DCHECK 0
-
-#elif defined(NDEBUG)
-// Otherwise, if we're a release build, remove DLOGs but not DCHECKs
-// (since those can still be turned on via a command-line flag).
+#if defined(NDEBUG)
 #define ENABLE_DLOG 0
-#define ENABLE_DCHECK 1
-
 #else
-// Otherwise, we're a debug build so enable DLOGs and DCHECKs.
 #define ENABLE_DLOG 1
+#endif
+
+#if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON)
+#define ENABLE_DCHECK 0
+#else
 #define ENABLE_DCHECK 1
 #endif
 
@@ -672,44 +644,12 @@ enum { DEBUG_MODE = ENABLE_DLOG };
 
 #if ENABLE_DCHECK
 
-#if defined(NDEBUG)
-
-BASE_EXPORT extern DcheckState g_dcheck_state;
-BASE_EXPORT void set_dcheck_state(DcheckState state);
-
-#if defined(DCHECK_ALWAYS_ON)
-
-#define DCHECK_IS_ON() true
-#define COMPACT_GOOGLE_LOG_EX_DCHECK(ClassName, ...) \
-  COMPACT_GOOGLE_LOG_EX_FATAL(ClassName , ##__VA_ARGS__)
-#define COMPACT_GOOGLE_LOG_DCHECK COMPACT_GOOGLE_LOG_FATAL
-const LogSeverity LOG_DCHECK = LOG_FATAL;
-
-#else
-
-#define COMPACT_GOOGLE_LOG_EX_DCHECK(ClassName, ...) \
-  COMPACT_GOOGLE_LOG_EX_ERROR_REPORT(ClassName , ##__VA_ARGS__)
-#define COMPACT_GOOGLE_LOG_DCHECK COMPACT_GOOGLE_LOG_ERROR_REPORT
-const LogSeverity LOG_DCHECK = LOG_ERROR_REPORT;
-
-#define DCHECK_IS_ON() \
-    UNLIKELY(::logging::g_dcheck_state == \
-             ::logging::ENABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS) && \
-    LOG_IS_ON(DCHECK)
-
-#endif  // defined(DCHECK_ALWAYS_ON)
-
-#else  // defined(NDEBUG)
-
-// On a regular debug build, we want to have DCHECKs enabled.
 #define COMPACT_GOOGLE_LOG_EX_DCHECK(ClassName, ...) \
   COMPACT_GOOGLE_LOG_EX_FATAL(ClassName , ##__VA_ARGS__)
 #define COMPACT_GOOGLE_LOG_DCHECK COMPACT_GOOGLE_LOG_FATAL
 const LogSeverity LOG_DCHECK = LOG_FATAL;
 #define DCHECK_IS_ON() true
 
-#endif  // defined(NDEBUG)
-
 #else  // ENABLE_DCHECK
 
 // These are just dummy values since DCHECK_IS_ON() is always false in

base/logging_unittest.cc

@@ -17,11 +17,11 @@ using ::testing::Return;
 // Needs to be global since log assert handlers can't maintain state.
 int log_sink_call_count = 0;
 
-#if !LOGGING_IS_OFFICIAL_BUILD
+#if !defined(OFFICIAL_BUILD) || defined(DCHECK_ALWAYS_ON) || !defined(NDEBUG)
 void LogSink(const std::string& str) {
   ++log_sink_call_count;
 }
-#endif  // !LOGGING_IS_OFFICIAL_BUILD
+#endif
 
 // Class to make sure any manipulations we do to the min log level are
 // contained (i.e., do not affect other unit tests).
@@ -169,7 +169,7 @@ TEST_F(LoggingTest, LoggingIsLazy) {
 }
 
 // Official builds have CHECKs directly call BreakDebugger.
-#if !LOGGING_IS_OFFICIAL_BUILD
+#if !defined(OFFICIAL_BUILD)
 
 TEST_F(LoggingTest, CheckStreamsAreLazy) {
   MockLogSource mock_log_source, uncalled_mock_log_source;
@@ -204,40 +204,34 @@ TEST_F(LoggingTest, DebugLoggingReleaseBehavior) {
 TEST_F(LoggingTest, DcheckStreamsAreLazy) {
   MockLogSource mock_log_source;
   EXPECT_CALL(mock_log_source, Log()).Times(0);
-#if !LOGGING_IS_OFFICIAL_BUILD && defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON)
-  // Unofficial release build without dcheck enabled.
-  set_dcheck_state(DISABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS);
-  DCHECK(mock_log_source.Log()) << mock_log_source.Log();
-  DPCHECK(mock_log_source.Log()) << mock_log_source.Log();
-  DCHECK_EQ(0, 0) << mock_log_source.Log();
-  DCHECK_EQ(mock_log_source.Log(), static_cast<const char*>(NULL))
-      << mock_log_source.Log();
-#endif
+  if (DCHECK_IS_ON()) {
+    DCHECK(true) << mock_log_source.Log();
+    DCHECK_EQ(0, 0) << mock_log_source.Log();
+  } else {
+    DCHECK(mock_log_source.Log()) << mock_log_source.Log();
+    DPCHECK(mock_log_source.Log()) << mock_log_source.Log();
+    DCHECK_EQ(0, 0) << mock_log_source.Log();
+    DCHECK_EQ(mock_log_source.Log(), static_cast<const char*>(NULL))
+        << mock_log_source.Log();
+  }
 }
 
 TEST_F(LoggingTest, Dcheck) {
-#if LOGGING_IS_OFFICIAL_BUILD
-  // Official build.
+#if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON)
+  // Release build.
   EXPECT_FALSE(DCHECK_IS_ON());
   EXPECT_FALSE(DLOG_IS_ON(DCHECK));
-#elif defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON)
-  // Unofficial release build.
-  set_dcheck_state(ENABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS);
-  SetLogReportHandler(&LogSink);
-  EXPECT_TRUE(DCHECK_IS_ON());
-  EXPECT_FALSE(DLOG_IS_ON(DCHECK));
 #elif defined(NDEBUG) && defined(DCHECK_ALWAYS_ON)
-  // Unofficial release build with real DCHECKS.
-  set_dcheck_state(ENABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS);
+  // Release build with real DCHECKS.
   SetLogAssertHandler(&LogSink);
   EXPECT_TRUE(DCHECK_IS_ON());
   EXPECT_FALSE(DLOG_IS_ON(DCHECK));
 #else
-  // Unofficial debug build.
+  // Debug build.
   SetLogAssertHandler(&LogSink);
   EXPECT_TRUE(DCHECK_IS_ON());
   EXPECT_TRUE(DLOG_IS_ON(DCHECK));
-#endif  // defined(LOGGING_IS_OFFICIAL_BUILD)
+#endif
 
   EXPECT_EQ(0, log_sink_call_count);
   DCHECK(false);

build/common.gypi

@@ -264,15 +264,9 @@
       # on compile-only bots).
       'fastbuild%': 0,
 
-      # Set to 1 to enable dcheck in release without having to use the flag.
+      # Set to 1 to enable dcheck in release.
       'dcheck_always_on%': 0,
 
-      # Set to 1 to make a build that logs like an official build, but is not
-      # necessarily an official build, ie DCHECK and DLOG are disabled and
-      # removed completely in release builds, to minimize binary footprint.
-      # Note: this setting is ignored if buildtype=="Official".
-      'logging_like_official_build%': 0,
-
       # Set to 1 to make a build that disables unshipped tracing events.
       # Note: this setting is ignored if buildtype=="Official".
       'tracing_like_official_build%': 0,
@@ -923,7 +917,6 @@
     'image_loader_extension%': '<(image_loader_extension)',
     'fastbuild%': '<(fastbuild)',
     'dcheck_always_on%': '<(dcheck_always_on)',
-    'logging_like_official_build%': '<(logging_like_official_build)',
     'tracing_like_official_build%': '<(tracing_like_official_build)',
     'python_ver%': '<(python_ver)',
     'arm_version%': '<(arm_version)',
@@ -2364,9 +2357,6 @@
       ['dcheck_always_on!=0', {
         'defines': ['DCHECK_ALWAYS_ON=1'],
       }],  # dcheck_always_on!=0
-      ['logging_like_official_build!=0', {
-        'defines': ['LOGGING_IS_OFFICIAL_BUILD=1'],
-      }],  # logging_like_official_build!=0
       ['tracing_like_official_build!=0', {
         'defines': ['TRACING_IS_OFFICIAL_BUILD=1'],
       }],  # tracing_like_official_build!=0

chrome/common/chrome_switches.cc

@@ -1305,8 +1305,7 @@ const char kSigninProcess[]                 = "signin-process";
 const char kSilentDebuggerExtensionAPI[]    = "silent-debugger-extension-api";
 
 // Changes the DCHECKS to dump memory and continue instead of displaying error
-// dialog. This is valid only in Release mode when --enable-dcheck is
-// specified.
+// dialog. This is valid only in Release mode when gyp dcheck_always_on=1.
 const char kSilentDumpOnDCHECK[]            = "silent-dump-on-dcheck";
 
 // Causes Chrome to launch without opening any windows by default. Useful if

chrome/common/logging_chrome.cc

@@ -243,17 +243,11 @@ void RedirectChromeLogging(const CommandLine& command_line) {
   // Always force a new symlink when redirecting.
   base::FilePath target_path = SetUpSymlinkIfNeeded(log_path, true);
 
-  logging::DcheckState dcheck_state =
-      command_line.HasSwitch(switches::kEnableDCHECK) ?
-      logging::ENABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS :
-      logging::DISABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS;
-
   // ChromeOS always logs through the symlink, so it shouldn't be
   // deleted if it already exists.
   logging::LoggingSettings settings;
   settings.logging_dest = DetermineLogMode(command_line);
   settings.log_file = log_path.value().c_str();
-  settings.dcheck_state = dcheck_state;
   if (!logging::InitLogging(settings)) {
     DLOG(ERROR) << "Unable to initialize logging to " << log_path.value();
     RemoveSymlinkAndLog(log_path, target_path);
@@ -303,17 +297,11 @@ void InitChromeLogging(const CommandLine& command_line,
     log_locking_state = DONT_LOCK_LOG_FILE;
   }
 
-  logging::DcheckState dcheck_state =
-      command_line.HasSwitch(switches::kEnableDCHECK) ?
-      logging::ENABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS :
-      logging::DISABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS;
-
   logging::LoggingSettings settings;
   settings.logging_dest = logging_dest;
   settings.log_file = log_path.value().c_str();
   settings.lock_log = log_locking_state;
   settings.delete_old = delete_old_log_file;
-  settings.dcheck_state = dcheck_state;
   bool success = logging::InitLogging(settings);
 
 #if defined(OS_CHROMEOS)
@@ -371,8 +359,7 @@ void InitChromeLogging(const CommandLine& command_line,
 #endif
 
 #ifdef NDEBUG
-  if (command_line.HasSwitch(switches::kSilentDumpOnDCHECK) &&
-      command_line.HasSwitch(switches::kEnableDCHECK)) {
+  if (command_line.HasSwitch(switches::kSilentDumpOnDCHECK) && DCHECK_IS_ON()) {
 #if defined(OS_WIN)
     logging::SetLogReportHandler(DumpProcessAssertHandler);
 #endif

chrome/test/automation/proxy_launcher.cc

@@ -122,8 +122,6 @@ ProxyLauncher::ProxyLauncher()
                             switches::kFullMemoryCrashReport)),
       show_error_dialogs_(CommandLine::ForCurrentProcess()->HasSwitch(
                               switches::kEnableErrorDialogs)),
-      enable_dcheck_(CommandLine::ForCurrentProcess()->HasSwitch(
-                         switches::kEnableDCHECK)),
       silent_dump_on_dcheck_(CommandLine::ForCurrentProcess()->HasSwitch(
                                  switches::kSilentDumpOnDCHECK)),
       disable_breakpad_(CommandLine::ForCurrentProcess()->HasSwitch(
@@ -428,8 +426,6 @@ void ProxyLauncher::PrepareTestCommandline(CommandLine* command_line,
     command_line->AppendSwitch(switches::kNoSandbox);
   if (full_memory_dump_)
     command_line->AppendSwitch(switches::kFullMemoryCrashReport);
-  if (enable_dcheck_)
-    command_line->AppendSwitch(switches::kEnableDCHECK);
   if (silent_dump_on_dcheck_)
     command_line->AppendSwitch(switches::kSilentDumpOnDCHECK);
   if (disable_breakpad_)

chrome/test/automation/proxy_launcher.h

@@ -186,9 +186,6 @@ class ProxyLauncher {
   // If true, a user is paying attention to the test, so show error dialogs.
   bool show_error_dialogs_;
 
-  // Enable dchecks in release mode.
-  bool enable_dcheck_;
-
   // Dump process memory on dcheck without crashing.
   bool silent_dump_on_dcheck_;