Merge pull request #35587 from gottesmm/pr-5db2d0b093c814bacaaf559563926c5dec16acc3

[ast] Enable the ASTVerifier behind the enable-ast-verifier flag in no-asserts builds.

Author: gottesmm

CMakeLists.txt

@@ -336,10 +336,6 @@ set(SWIFT_DARWIN_DEPLOYMENT_VERSION_WATCHOS "2.0" CACHE STRING
 # User-configurable debugging options.
 #
 
-option(SWIFT_AST_VERIFIER
-    "Enable the AST verifier in the built compiler, and run it on every compilation"
-    TRUE)
-
 option(SWIFT_SIL_VERIFY_ALL
     "Run SIL verification after each transform when building Swift files in the build process"
     FALSE)

include/swift/Basic/LangOptions.h

@@ -367,6 +367,20 @@ namespace swift {
     // FrontendOptions.
     bool AllowModuleWithCompilerErrors = false;
 
+    /// A helper enum to represent whether or not we customized the default
+    /// ASTVerifier behavior via a frontend flag. By default, we do not
+    /// customize.
+    ///
+    /// NOTE: The default behavior is to run the ASTVerifier only when asserts
+    /// are enabled. This just allows for one to customize that behavior.
+    enum class ASTVerifierOverrideKind {
+      NoOverride = 0,
+      EnableVerifier = 1,
+      DisableVerifier = 2,
+    };
+    ASTVerifierOverrideKind ASTVerifierOverride =
+        ASTVerifierOverrideKind::NoOverride;
+
     /// Sets the target we are building for and updates platform conditions
     /// to match.
     ///

include/swift/Option/FrontendOptions.td

@@ -767,4 +767,20 @@ def bad_file_descriptor_retry_count:
   Separate<["-"], "bad-file-descriptor-retry-count">,
   HelpText<"Number of retrying opening a file if previous open returns a bad "
            "file descriptor error.">;
+
+def enable_ast_verifier : Flag<["-"], "enable-ast-verifier">,
+  HelpText<"Run the AST verifier during compilation. NOTE: This lets the user "
+           "override the default behavior on whether or not the ASTVerifier "
+           "is run. The default behavior is to run the verifier when asserts "
+           "are enabled and not run it when asserts are disabled. NOTE: "
+           "Can not be used if disable-ast-verifier is used as well">;
+
+def disable_ast_verifier : Flag<["-"], "disable-ast-verifier">,
+  HelpText<"Do not run the AST verifier during compilation. NOTE: This lets "
+           "the user override the default behavior on whether or not the "
+           "ASTVerifier is run. The default behavior is to run the verifier "
+           "when asserts are enabled and not run it when asserts are "
+           "disabled. NOTE: Can not be used if enable-ast-verifier is used "
+           "as well">;
+
 } // end let Flags = [FrontendOption, NoDriverOption, HelpHidden]

lib/AST/ASTVerifier.cpp

@@ -3686,15 +3686,36 @@ class Verifier : public ASTWalker {
   };
 } // end anonymous namespace
 
+static bool shouldVerifyGivenContext(const ASTContext &ctx) {
+  using ASTVerifierOverrideKind = LangOptions::ASTVerifierOverrideKind;
+  switch (ctx.LangOpts.ASTVerifierOverride) {
+  case ASTVerifierOverrideKind::EnableVerifier:
+    return true;
+  case ASTVerifierOverrideKind::DisableVerifier:
+    return false;
+  case ASTVerifierOverrideKind::NoOverride:
+#ifndef NDEBUG
+    // asserts. Default behavior is to run.
+    return true;
+#else
+    // no-asserts. Default behavior is not to run the verifier.
+    return false;
+#endif
+  }
+  llvm_unreachable("Covered switch isn't covered?!");
+}
+
 void swift::verify(SourceFile &SF) {
-#if !(defined(NDEBUG) || defined(SWIFT_DISABLE_AST_VERIFIER))
+  if (!shouldVerifyGivenContext(SF.getASTContext()))
+    return;
   Verifier verifier(SF, &SF);
   SF.walk(verifier);
-#endif
 }
 
 bool swift::shouldVerify(const Decl *D, const ASTContext &Context) {
-#if !(defined(NDEBUG) || defined(SWIFT_DISABLE_AST_VERIFIER))
+  if (!shouldVerifyGivenContext(Context))
+    return false;
+
   if (const auto *ED = dyn_cast<ExtensionDecl>(D)) {
     return shouldVerify(ED->getExtendedNominal(), Context);
   }
@@ -3706,15 +3727,13 @@ bool swift::shouldVerify(const Decl *D, const ASTContext &Context) {
   }
 
   return true;
-#else
-  return false;
-#endif
 }
 
 void swift::verify(Decl *D) {
-#if !(defined(NDEBUG) || defined(SWIFT_DISABLE_AST_VERIFIER))
+  if (!shouldVerifyGivenContext(D->getASTContext()))
+    return;
+
   Verifier V = Verifier::forDecl(D);
   D->walk(V);
-#endif
 }
 

lib/AST/CMakeLists.txt

@@ -144,14 +144,3 @@ endif()
 # headers.
 # For more information see the comment at the top of lib/CMakeLists.txt.
 add_dependencies(swiftAST intrinsics_gen clang-tablegen-targets)
-
-set(swift_ast_verifier_flag)
-if(SWIFT_AST_VERIFIER)
-  set(swift_ast_verifier_flag " -USWIFT_DISABLE_AST_VERIFIER")
-else()
-  set(swift_ast_verifier_flag " -DSWIFT_DISABLE_AST_VERIFIER=1")
-endif()
-
-set_property(SOURCE ASTVerifier.cpp APPEND_STRING PROPERTY COMPILE_FLAGS
-  "${swift_ast_verifier_flag}")
-

lib/Frontend/CompilerInvocation.cpp

@@ -689,6 +689,21 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
     Opts.AllowModuleWithCompilerErrors = true;
   }
 
+  if (auto A =
+          Args.getLastArg(OPT_enable_ast_verifier, OPT_disable_ast_verifier)) {
+    using ASTVerifierOverrideKind = LangOptions::ASTVerifierOverrideKind;
+    if (A->getOption().matches(OPT_enable_ast_verifier)) {
+      Opts.ASTVerifierOverride = ASTVerifierOverrideKind::EnableVerifier;
+    } else if (A->getOption().matches(OPT_disable_ast_verifier)) {
+      Opts.ASTVerifierOverride = ASTVerifierOverrideKind::DisableVerifier;
+    } else {
+      // This is an assert since getLastArg should not have let us get here if
+      // we did not have one of enable/disable specified.
+      llvm_unreachable(
+          "Should have found one of enable/disable ast verifier?!");
+    }
+  }
+
   return HadError || UnsupportedOS || UnsupportedArch;
 }
 

test/lit.site.cfg.in

@@ -83,9 +83,6 @@ if "@SWIFT_STDLIB_ASSERTIONS@" == "TRUE":
 else:
     config.available_features.add('swift_stdlib_no_asserts')
 
-if "@SWIFT_AST_VERIFIER@" == "TRUE":
-    config.available_features.add('swift_ast_verifier')
-
 if "@SWIFT_OPTIMIZED@" == "TRUE":
     config.available_features.add("optimized_stdlib")
 

tools/sil-opt/SILOpt.cpp

@@ -116,11 +116,11 @@ EnableSpeculativeDevirtualization("enable-spec-devirt",
                   llvm::cl::desc("Enable Speculative Devirtualization pass."));
 
 namespace {
-enum EnforceExclusivityMode {
+enum class EnforceExclusivityMode {
   Unchecked, // static only
   Checked,   // static and dynamic
   DynamicOnly,
-  None
+  None,
 };
 } // end anonymous namespace
 
@@ -203,6 +203,16 @@ SILVerifyNone("sil-verify-none",
               llvm::cl::init(false),
               llvm::cl::desc("Completely disable SIL verification"));
 
+/// Customize the default behavior
+static llvm::cl::opt<bool> EnableASTVerifier(
+    "enable-ast-verifier", llvm::cl::Hidden, llvm::cl::init(false),
+    llvm::cl::desc("Override the default behavior and Enable the ASTVerifier"));
+
+static llvm::cl::opt<bool> DisableASTVerifier(
+    "disable-ast-verifier", llvm::cl::Hidden, llvm::cl::init(false),
+    llvm::cl::desc(
+        "Override the default behavior and force disable the ASTVerifier"));
+
 static llvm::cl::opt<bool>
 RemoveRuntimeAsserts("remove-runtime-asserts",
                      llvm::cl::Hidden,
@@ -310,6 +320,22 @@ static void runCommandLineSelectedPasses(SILModule *Module,
     Module->verify();
 }
 
+namespace {
+using ASTVerifierOverrideKind = LangOptions::ASTVerifierOverrideKind;
+} // end anonymous namespace
+
+static Optional<ASTVerifierOverrideKind> getASTOverrideKind() {
+  assert(!(EnableASTVerifier && DisableASTVerifier) &&
+         "Can only set one of EnableASTVerifier/DisableASTVerifier?!");
+  if (EnableASTVerifier)
+    return ASTVerifierOverrideKind::EnableVerifier;
+
+  if (DisableASTVerifier)
+    return ASTVerifierOverrideKind::DisableVerifier;
+
+  return None;
+}
+
 // This function isn't referenced outside its translation unit, but it
 // can't use the "static" keyword because its address is used for
 // getMainExecutable (since some platforms don't support taking the
@@ -362,7 +388,9 @@ int main(int argc, char **argv) {
   Invocation.getLangOptions().DisableAvailabilityChecking = true;
   Invocation.getLangOptions().EnableAccessControl = false;
   Invocation.getLangOptions().EnableObjCAttrRequiresFoundation = false;
-  
+  if (auto overrideKind = getASTOverrideKind()) {
+    Invocation.getLangOptions().ASTVerifierOverride = *overrideKind;
+  }
   Invocation.getLangOptions().EnableExperimentalConcurrency =
     EnableExperimentalConcurrency;
 

validation-test/SIL/crashers_fixed/017-swift-decl-walk.sil

@@ -1,3 +1,3 @@
-// RUN: not %target-sil-opt %s
-// REQUIRES: swift_ast_verifier
+// RUN: not %target-sil-opt -enable-ast-verifier %s
+
 import Swift func g(@opened(Any

validation-test/compiler_crashers_fixed/28653-child-source-range-not-contained-within-its-parent.swift

@@ -5,7 +5,6 @@
 // See https://swift.org/LICENSE.txt for license information
 // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
 
-// RUN: not %target-swift-frontend %s -emit-ir
-// REQUIRES: swift_ast_verifier
+// RUN: not %target-swift-frontend -enable-ast-verifier %s -emit-ir
 
 switch{case.b(u){