New assertions support

[tbkka]

This adds three new assertion macros:

• ASSERT - always compiled in, always checked
• CONDITIONAL_ASSERT - always compiled in, checked whenever the -compiler-assertions flag is provided
• DEBUG_ASSERT - only compiled into debug builds, always checked when compiled in (functionally the same as Standard C assert)

The new -compiler-assertions flag is recognized by both swift-frontend and swiftc.

The goal is to eventually replace every use of assert in the compiler with one of the above:

• Most assertions will eventually use ASSERT (most assertions should always be present and checked, even in release builds)
• Expensive assertions can use CONDITIONAL_ASSERT to be suppressed by default
• A few very expensive and/or brittle assertions can use DEBUG_ASSERT to be compiled out of release builds

(Of course, we have a lot of flaky assertions today, so in the near-term, CONDITIONAL_ASSERT will likely be used far more heavily than is really appropriate until we can work through those.)

This should:

• Improve quality by catching errors earlier,
• Accelerate compiler triage and debugging by providing more accurate crash dumps by default, and
• Allow compiler engineers and end users alike to add -compiler-assertions to get more accurate failure diagnostics with any compiler

[tbkka]

@swift-ci Please test

[tbkka]

Note that this is the first piece of this proposed change. I wanted to leave it this way initially to get feedback on the overall design.

• Some initial benchmarking shows that globally replacing assert with CONDITIONAL_ASSERT impacts the performance of a release-built compiler by less than 1%. (That is, testing a global to skip every assertion is no more than 1% slower than compiling the assertions entirely out.) I think that's a small enough penalty to make this a viable approach.

• Next step: Add #include "swift/Basic/Assertions.h" to almost every .cpp file in the entire project. (This is purely mechanical and I have it already done in my local tree.)

• Next step after that: Replace assert with new assertion macros in large chunks of the project. (This is less mechanical and will probably need to be done in stages.)

• One tricky point: The runtime and other libraries may need to stick with assert for a while, and any code used by those libraries, until we can figure out the right way to extend this approach to those.

[tbkka]

Other notes:

• I am not proposing this for 6.0. (Though it may make sense to pull over the new headers just to ensure that any new assertions that get accidentally cherry-picked don't break anything.)

• I've marked this Draft while I solicit feedback on the overall approach. I want to make sure folks are comfortable with this before we start globally replacing assertion statements.

[nate-chandler]

It would be handy to have more options than (a) trap on first assertion failure (b) ignore all assertions, considering that discovering which assertions are incorrect will be a gradual process, and the first assertion that fails may not be related to the problem directly being solved.

For example:

• print assertions that fail (and backtraces) but keep running
• trap on the Nth assertion
• break on assertion failures but keep running (LLVM_BUILTIN_DEBUGTRAP)
• break on the Nth assertion failure

[tbkka]

All of that could be handled with future changes to this function, if we made the function declaration to not be [[noreturn]].

[tbkka]

I added two very basic controls as an experiment:

-Xllvm -assert-help           Dumps a list of assertion control options the first time an assertion is hit
-Xllvm -assert-continue    Assertion failures are printed but don't abort()

If this general approach (using -Xllvm) works for folks, then this seems pretty easy to expand with whatever other controls we might like. Another possibility would be to provide an interactive mode similar to what @al45tair did for backtracing: An assertion would then print a prompt and wait for you to press a key. You could choose to continue, drop into a debugger, or abort(). After a timeout, the default handling would proceed.

[mikeash]

Two sub-variants of the above could integrate better with complex builds: emit a warning on failure, and emit an error on failure (both continuing execution afterwards). But obviously does not have to be in the first version of this!

[tbkka]

Yes, I plan to keep the first version pretty basic: There's a lot of potentially-disruptive work to merge this, and I want to be sure folks are comfortable with me roto-tilling all the assertions in the project. Once that's done, there are many ways to improve how assertions actually get reported and processed at runtime.

[beccadax]

It might also be good to have a selective continue option, like:

-Xllvm -assert-work-around=TypeCheckAttr.cpp:181

This would provide a way to work around a specific assertion that's been investigated and found to be spurious for a particular project.

[al45tair]

I wonder if we could make the assertion macros preserve register state. One of the problems we have, especially on Darwin where abort() isn't a system call and does a tonne of extra work, is that the crash dump outputs from these kinds of things tend to be generated after all the registers have already been overwritten, which makes retrospective debugging harder than necessary.

I have some ideas on this front; I'll ping you on Slack.

[mikeash]

This looks great! I'm excited to get this in.

[tbkka]

@swift-ci Please test

[tbkka]

2 weeks with no-one expressing concerns with the overall approach here -- just lots of good ideas about how to refine the assertion reporting and handling, all of which can be pursued incrementally with future changes. So I'll go ahead and land this and proceed with the next step (including the new header into basically everywhere).

[tbkka]

Oh yeah: @mikeash I'm still not sure how/if to use this in the runtime. My goal right now is the compiler proper. If you think this would be good in the runtime too, we should talk about how to make that work. (Will require dropping the assertion failure handler into the runtime lib and figuring out an appropriate way to control the assertion behavior.)

[mikeash]

I'd love to see this in the runtime too.

[al45tair]

Agreed. The only thing to watch out for is that there are some cases (demangling being one such) where it makes sense to assert always in the runtime, but it doesn't make sense when the code is being invoked from LLDB (we don't want the debugger to crash with an assertion failure — it often speculatively tries things, that being the nature of the beast).

[beccadax]

Sorry for the late review; didn't see this when it was new.

Bottom line: I like this idea! We've needed to rethink compiler assertions for a while now.

[beccadax]

It might also be good to have a selective continue option, like:

-Xllvm -assert-work-around=TypeCheckAttr.cpp:181

This would provide a way to work around a specific assertion that's been investigated and found to be spurious for a particular project.