From 58fba078ee3ac3d477446e2d5c48da933b23a645 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Thu, 23 Jan 2025 12:47:51 -0800 Subject: [PATCH] Add a flag to make `CHECK` failures non-fatal for debugging. (#4835) `toolchain/autoupdate_testdata.py --allow-check-fail` can now be used to perform an autoupdate even if some `CARBON_CHECK`s are failing. What this does will depend on how the toolchain behaves after the `CHECK` failure, and of course there's no guarantees there, but this can be useful if it's easier to debug the `CHECK` failure by looking at the produced SemIR. Internally, this uses `bazel build --config=non-fatal-checks`, which in turn specifies a `--per_file_copt` for `check_internal.cpp`. The intent here is that the rebuild required to enable or disable this mode is as small as reasonably possible. This mode is not compatible with `-c opt`, as it's important that check failure calls are `[[noreturn]]` in `-c opt` mode. --------- Co-authored-by: Jon Ross-Perkins --- .bazelrc | 3 +++ common/check_internal.cpp | 23 ++++++++++++++++------- common/check_internal.h | 32 +++++++++++++++++++++++--------- toolchain/autoupdate_testdata.py | 24 +++++++++++++++++++++--- 4 files changed, 63 insertions(+), 19 deletions(-) diff --git a/.bazelrc b/.bazelrc index f293d39e0254..6eaaca23223b 100644 --- a/.bazelrc +++ b/.bazelrc @@ -28,6 +28,9 @@ build:clang-tidy --action_env=PATH --host_action_env=PATH # not firing in our normal builds. build:clang-tidy --copt=-Wno-unknown-pragmas +# --config=non-fatal-checks makes CHECK failures not terminate compilation. +build:non-fatal-checks --per_file_copt=common/check_internal.cpp@-DCARBON_NON_FATAL_CHECKS + # Provide an alias for controlling the `carbon_*` Bazel rules' configuration. We # enable use of the target config here to make our build and tests more # efficient, see the documentation in //bazel/carbon_rules/BUILD for details. diff --git a/common/check_internal.cpp b/common/check_internal.cpp index 9fc5cd653a15..3beeed25b2e6 100644 --- a/common/check_internal.cpp +++ b/common/check_internal.cpp @@ -9,11 +9,6 @@ namespace Carbon::Internal { -// Prints the buffered message. -static auto PrintAfterStackTrace(void* str) -> void { - llvm::errs() << reinterpret_cast(str); -} - auto CheckFailImpl(const char* kind, const char* file, int line, const char* condition_str, llvm::StringRef extra_message) -> void { @@ -23,15 +18,29 @@ auto CheckFailImpl(const char* kind, const char* file, int line, llvm::StringRef(condition_str).empty() ? "" : ": ", condition_str, extra_message.empty() ? "" : ": ", extra_message); + // This macro is defined by `--config=non-fatal-checks`. +#ifdef CARBON_NON_FATAL_CHECKS +#ifdef NDEBUG +#error "--config=non-fatal-checks is incompatible with -c opt" +#endif + // TODO: It'd be nice to print the LLVM PrettyStackTrace, but LLVM doesn't + // expose functionality to do so. + llvm::sys::PrintStackTrace(llvm::errs()); + + llvm::errs() << message; +#else // Register another signal handler to print the message. This is because we // want it at the bottom of output, after LLVM's builtin stack output, rather // than the top. - llvm::sys::AddSignalHandler(PrintAfterStackTrace, - const_cast(message.c_str())); + llvm::sys::AddSignalHandler( + [](void* str) { llvm::errs() << reinterpret_cast(str); }, + const_cast(message.c_str())); + // It's useful to exit the program with `std::abort()` for integration with // debuggers and other tools. We also assume LLVM's exit handling is // installed, which will stack trace on `std::abort()`. std::abort(); +#endif } } // namespace Carbon::Internal diff --git a/common/check_internal.h b/common/check_internal.h index 97161297ed6f..ff76ed13429a 100644 --- a/common/check_internal.h +++ b/common/check_internal.h @@ -31,7 +31,9 @@ CheckCondition(bool condition) // Implements the check failure message printing. // // This is out-of-line and will arrange to stop the program, print any debugging -// information and this string. +// information and this string. In `!NDEBUG` mode (`dbg` and `fastbuild`), check +// failures can be made non-fatal by a build flag, so this is not `[[noreturn]]` +// in that case. // // This API uses `const char*` C string arguments rather than `llvm::StringRef` // because we know that these are available as C strings and passing them that @@ -39,9 +41,12 @@ CheckCondition(bool condition) // a single pointer argument for each. The runtime cost of re-computing the size // should be minimal. The extra message however might not be compile-time // guaranteed to be a C string so we use a normal `StringRef` there. -[[noreturn]] auto CheckFailImpl(const char* kind, const char* file, int line, - const char* condition_str, - llvm::StringRef extra_message) -> void; +#ifdef NDEBUG +[[noreturn]] +#endif +auto CheckFailImpl(const char* kind, const char* file, int line, + const char* condition_str, llvm::StringRef extra_message) + -> void; // Allow converting format values; the default behaviour is to just pass them // through. @@ -77,8 +82,11 @@ auto ConvertFormatValue(T&& t) { // `llvm::formatv` which handles all of the formatting of output. template -[[noreturn, gnu::cold, clang::noinline]] auto CheckFail(Ts&&... values) - -> void { +#ifdef NDEBUG +[[noreturn]] +#endif +[[gnu::cold, clang::noinline]] auto +CheckFail(Ts&&... values) -> void { if constexpr (llvm::StringRef(FormatStr).empty()) { // Skip the format string rendering if empty. Note that we don't skip it // even if there are no values as we want to have consistent handling of @@ -135,9 +143,10 @@ template (__VA_ARGS__) + +// The CheckFail function itself is noreturn in NDEBUG. +#define CARBON_INTERNAL_FATAL_NORETURN_SUFFIX() void() +#else +#define CARBON_INTERNAL_FATAL_NORETURN_SUFFIX() std::abort() #endif #endif // CARBON_COMMON_CHECK_INTERNAL_H_ diff --git a/toolchain/autoupdate_testdata.py b/toolchain/autoupdate_testdata.py index dd1c174d8731..85b6061d227f 100755 --- a/toolchain/autoupdate_testdata.py +++ b/toolchain/autoupdate_testdata.py @@ -8,6 +8,7 @@ Exceptions. See /LICENSE for license information. SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception """ +import argparse import re import subprocess import sys @@ -16,6 +17,7 @@ from pathlib import Path def main() -> None: bazel = str(Path(__file__).parents[1] / "scripts" / "run_bazel.py") + configs = [] # Use the most recently used build mode, or `fastbuild` if missing # `bazel-bin`. build_mode = "fastbuild" @@ -37,11 +39,26 @@ def main() -> None: else: exit(f"Build mode not found in `bazel-bin` symlink: {link}") + # Parse arguments. + parser = argparse.ArgumentParser(__doc__) + parser.add_argument("--non-fatal-checks", action="store_true") + parser.add_argument("files", nargs="*") + args = parser.parse_args() + + if args.allow_check_fail: + if build_mode == "opt": + exit( + "`--non-fatal-checks` is incompatible with inferred " + "`-c opt` build mode" + ) + configs.append("--config=non-fatal-checks") + argv = [ bazel, "run", "-c", build_mode, + *configs, "--experimental_convenience_symlinks=ignore", "--ui_event_filters=-info,-stdout,-stderr,-finish", "//toolchain/testing:file_test", @@ -50,18 +67,19 @@ def main() -> None: ] # Support specifying tests to update, such as: # ./autoupdate_testdata.py lex/**/* - if len(sys.argv) > 1: + if args.files: repo_root = Path(__file__).parents[1] file_tests = [] # Filter down to just test files. - for f in sys.argv[1:]: + for f in args.files: if f.endswith(".carbon"): path = str(Path(f).resolve().relative_to(repo_root)) if path.count("/testdata/"): file_tests.append(path) if not file_tests: sys.exit( - f"Args do not seem to be test files; for example, {sys.argv[1]}" + "Args do not seem to be test files; for example, " + f"{args.files[0]}" ) argv.append("--file_tests=" + ",".join(file_tests)) # Provide an empty stdin so that the driver tests that read from stdin