sqlite: Re-enable ENABLE_SQLITE_API_ARMOR outside fuzzers.

https://crrev.com/c/1315959 and https://crrev.com/c/1341921 made it
possible to use SQLITE_DEBUG for SQLite fuzzers, which has uncovered
bugs. Unfortunately, the CLs unintentionally removed
ENABLE_SQLITE_API_ARMOR from non-fuzzing builds, reducing our ability to
catch API misuse.

This CL re-instates ENABLE_SQLITE_API_ARMOR for non-fuzzing builds, for
the reasons described above. It also removes
-Wno-implicit-function-declaration in return for a less intrusive
workaround in sqlite3_shim.c.

Bug: 900910
Change-Id: I6327fdcee173c384da0b3b62c1414b7b6126473f
Reviewed-on: https://chromium-review.googlesource.com/c/1345649
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Max Moroz <mmoroz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610362}

third_party/sqlite/BUILD.gn

@@ -174,13 +174,22 @@ config("chromium_sqlite3_compile_options") {
     }
   }
 
-  if (use_fuzzing_engine && use_sanitizer_coverage &&
-      (is_debug || dcheck_always_on)) {
-    # Enable SQLite's assert() macros.
-    defines += [ "SQLITE_DEBUG" ]
+  if (is_debug || dcheck_always_on) {
+    if (use_fuzzing_engine && use_sanitizer_coverage) {
+      # Enable SQLite's assert() macros.
+      #
+      # TODO(pwnall): Fix all the bugs preventing us from enabling this flag for
+      #               all DCHECK builds. See https://crbug.com/907371 and
+      #               https://crrev.com/c/1343529.
+      defines += [ "SQLITE_DEBUG" ]
+    }
 
     # Check preconditions when SQLite APIs are called. See
     # https://sqlite.org/compile.html#enable_api_armor
+    #
+    # fuzzing builds have this disabled because the fuzzers are guaranteed to
+    # use the API correctly, and removing the checks opens up the possibility
+    # that the fuzzers will get more code coverage.
     defines += [ "SQLITE_ENABLE_API_ARMOR" ]
   }
 }
@@ -195,10 +204,11 @@ config("sqlite_warnings") {
     # sqlite3Fts3InitTok).
     cflags += [ "-Wno-unused-function" ]
 
-    if (use_fuzzing_engine && use_sanitizer_coverage &&
-        (is_debug || dcheck_always_on)) {
+    if (is_debug || dcheck_always_on) {
       cflags += [
-        "-Wno-implicit-function-declaration",
+        # SQLite uses assert(!"description") to express
+        # NOT_REACHED() << "description". This is considered an implicit
+        # conversion from char[] to bool, and triggers a warning.
         "-Wno-string-conversion",
       ]
     }

third_party/sqlite/sqlite3_shim.c

@@ -36,4 +36,17 @@
 
 #endif  // defined(__linux__)
 
+// For unfortunately complex reasons, Chrome has release builds where
+// DCHECK_IS_ON() (so we want SQLITE_DEBUG to be on) but NDEBUG is also defined.
+// This causes declarations for mutex-checking functions used by SQLITE_DEBUG
+// code (sqlite3_mutex_held, sqlite3_mutex_notheld) to be omitted, resulting in
+// warnings.
+//
+// The easiest solution for now is to undefine NDEBUG when SQLITE_DEBUG is
+// defined. The #undef only takes effect for the SQLite implementation (included
+// below), and does not impact any dependency.
+#if defined(SQLITE_DEBUG) && defined(NDEBUG)
+#undef NDEBUG
+#endif  // defined(SQLITE_DEBUG) && defined(NDEBUG)
+
 #include "third_party/sqlite/amalgamation/sqlite3.c"