Add a flag to make CHECK failures non-fatal for debugging.

[zygoloid]

toolchain/autoupdate_testdata.py --allow-check-fail can now be used to perform an autoupdate even if some CARBON_CHECKs 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.

[jonmeow]

LGTM, just suggesting if you want to address the TODO, I think it may be a smaller change than you're expecting (maybe just needs a little familiarity)

[jonmeow]

Had you considered --non-fatal-checks to match the --config?

[zygoloid]

Yeah, I've gone back and forth on this. I found this flag name more intuitive when running autoupdate_testdata, but using a single name also seems nice, and this name doesn't work so well for the config. If you have a strong opinion I'm happy to defer to that.

[jonmeow]

I have a slight leaning towards the same name because it's niche, so my instinct is keeping the same name in spots would make it clearer that they're closely related. I wouldn't call it a strong opinion though.

Perhaps a different name would help -- maybe no-abort-on-check (or disable-check-abort, or similar) or log[ging]-checks? Mirroring something similar to the #ifdef... I don't know if any of those names sound better to you in the various spots.

[zygoloid]

A slight leaning is more than I have, so I've gone with non-fatal-checks throughout.

[jonmeow]

BTW, LG with the argparse changes, sorry about my * vs + mixup

[chandlerc]

I don't really want to block this landing, but I do feel very nervous about it.

Specifically, I'm very nervous about something that is only sometimes no-return. For example, won't this build mode be flaky with -Wuninitialized warnings? Without this config, a CHECK can be used to clearly and forcibly cut off a control flow path, but with this build that isn't true.

This is debug only, and so I'm fine if we want to experiment with it and see if its useful. But I think this is even more brittle than it is described. Mostly, I don't want us to start slowly accumulating changes to work around when CHECK isn't actually no-return, as I don't think source level changes like that will be sustainable long-term.

[jonmeow]

For example, won't this build mode be flaky with -Wuninitialized warnings?

I think zygoloid is taking advantage of a very peculiar trick: --per_file_copt=common/check_internal.cpp@-DCARBON_NON_FATAL_CHECKS

This means only the implementation of CheckFail sees the behavior change. Users of CHECK won't see it, meaning uninitialized warnings shouldn't be affected.

It's weird and potentially risky, and yes brittle, but when it's mainly for debug (and also default-off) I'm kind of okay with that if it helps zygoloid.

I think it's better than the alternative mentioned of adding another CHECK variant that explicitly isn't noreturn and then auditing current CHECK calls to migrate. That to me has a higher cost, because it creates CHECK more variants that are intended to work (but untested).

[zygoloid]

I'm very nervous about something that is only sometimes no-return. For example, won't this build mode be flaky with -Wuninitialized warnings? Without this config, a CHECK can be used to clearly and forcibly cut off a control flow path, but with this build that isn't true.

CARBON_CHECK can't be used to cut off control flow to the following code. We have compile-time checking that the condition of a CARBON_CHECK is neither a constant false nor a constant true, so the code after the CARBON_CHECK is always reachable regardless of build mode. And for CARBON_FATAL, this PR injects a call to std::abort() after the check failure in the build modes where CheckFail can return.

So this should not in any way affect flow-sensitive reachability. It does affect path-sensitive reachability, but I'm assuming that that (a) won't affect diagnostics, and (b) doesn't really matter outside -c opt.