[ast] Enable the ASTVerifier behind the enable-ast-verifier flag in no-asserts builds. #35587
Conversation
…o-asserts builds. This follows the design of how we handled this with sil-verify-all. Specifically, the default behavior is to run only in asserts builds, but one can use the two flags: enable-ast-verifier and disable-ast-verifier to override the default behavior. The reason why this is interesting is that this means that when compiling normally, we will not run the verifier, so we won't have a perf hit. But we can now ask the user to run with this flag (or in a future maybe a re-run in the driver would do this for them), saving us time when screening bugs by avoiding the need to build an asserts compiler to triage if the ASTVerifier would catch the bug.
|
@swift-ci test |
|
@swift-ci test compiler performance |
| "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 " |
There was a problem hiding this comment.
I need to update the text here to say that we take the last of enable_ast_verifier, disable_ast_verifier. I am going to let the testing happen though and then fix this, do a smoke test when I come back to this from finishing some OSSA work.
There was a problem hiding this comment.
Ping to update this help text as you pointed out earlier. :)
| @@ -116,11 +116,11 @@ EnableSpeculativeDevirtualization("enable-spec-devirt", | |||
| llvm::cl::desc("Enable Speculative Devirtualization pass.")); | |||
|
|
|||
| namespace { | |||
| enum EnforceExclusivityMode { | |||
| enum class EnforceExclusivityMode { | |||
There was a problem hiding this comment.
I made this change since EnforceExclusivityMode was injecting None into the global namespace meaning the enum field clashed with Optional::None. We already were always referring to these enum values explicitly, so it makes sense to just an enum class here.
|
Build failed |
|
@swift-ci test macOS platform |
|
Build failed |
|
@swift-ci test macOS platform |
|
I talked with @varungandhi-apple. They are fine with us doing post-commit review on this since there wasn't a perf change at all. |
| } 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?!"); | ||
| } |
There was a problem hiding this comment.
Nit: it would be slightly simpler to change this to:
} else {
assert(A->getOption().matches(OPT_disable_ast_verifier) &&
"<message>");
Opts.ASTVerifierOverride = ASTVerifierOverrideKind::DisableVerifier
}
This follows the design of how we handled this with
sil-verify-all. Specifically, the default behavior is to run only in asserts
builds, but one can use the two flags: enable-ast-verifier and
disable-ast-verifier to override the default behavior.
The reason why this is interesting is that this means that when compiling
normally, we will not run the verifier, so we won't have a perf hit. But we can
now ask the user to run with this flag (or in a future maybe a re-run in the
driver would do this for them), saving us time when screening bugs by avoiding
the need to build an asserts compiler to triage if the ASTVerifier would catch
the bug.