src.opts.mk: Default LLVM_ASSERTIONS off

[kbowling]

Summary

This noticeably slows down the compiler execution.

It seems like it should be toggled on in the import branches for test and exp-run and default off otherwise.

[jrtc27]

Or, like WITNESS or MALLOC_PRODUCTION, on in HEAD and off in stable branches?

[kbowling]

In D50388#1149945, @jrtc27 wrote:

Or, like WITNESS or MALLOC_PRODUCTION, on in HEAD and off in stable branches?

My thinking here is few of us are LLVM developers, and it is not clear this is a benefit to apply broadly. I am less certain on where MALLOC_PRODUCTION should fall, if the thought is it catches src errors than that is a favor for keeping it on in HEAD. WITNESS is directly relevant to HEAD development and the pkgbase now makes it easy to opt out.

[kbowling]

I'm planning to commit this soon unless a serious objection is raised.

[emaste]

I have no objection to this general idea but defer to @dim. I do think that there's value in having LLVM_ASSERTIONS enabled in head and disabled in stable branches in the coverage it provides in the window before the compiler makes it to the stable branch.

Oh, LLVM_ASSERTIONS support arrived in 147d7b567f03f7e327b9bd2fa5bec6077022aeeb with the note:

For head/, this will remain eternally default-on to maintain the status quo.
For stable/ branches, it should be flipped to default-off to maintain the
status quo.

Adding @kevans to reviewers list

[kevans]

I don't really object to it defaulting off, but I think I'd want the cluster ports builders to keep it on (so we'll need to poke pkgmgr@). We have discovered llvm bugs in the past from random ports tripping assertions, and there's some value in still being able to discover that via exp-runs and pkg-fallout.

[dim]

The idea behind this being on in -CURRENT is to catch more errors. The same really holds for WITNESS and other assertions, but you could argue that those are more for the core of the system, while llvm-project is contributed software. Note that we caught many assertions since they are enabled by default, so they definitely have value.

In D50388#1155400, @kevans wrote:

I don't really object to it defaulting off, but I think I'd want the cluster ports builders to keep it on (so we'll need to poke pkgmgr@). We have discovered llvm bugs in the past from random ports tripping assertions, and there's some value in still being able to discover that via exp-runs and pkg-fallout.

For fixing bugs in the toolchain, that is indeed extremely useful. When assertions are off, sometimes the compiler or linker will simply segfault, but in many other cases there might be bad code silently emitted, or some other nastiness.

That said, if people are really so annoyed by this, it may make sense to turn them off by default. But you could then also say that for MALLOC_PRODUCTION, for example. Why make everybody's memory allocations slow, while you really only intend to debug the core system? :)

[kevans]

In D50388#1155608, @dim wrote:

In D50388#1155400, @kevans wrote:

I don't really object to it defaulting off, but I think I'd want the cluster ports builders to keep it on (so we'll need to poke pkgmgr@). We have discovered llvm bugs in the past from random ports tripping assertions, and there's some value in still being able to discover that via exp-runs and pkg-fallout.

For fixing bugs in the toolchain, that is indeed extremely useful. When assertions are off, sometimes the compiler or linker will simply segfault, but in many other cases there might be bad code silently emitted, or some other nastiness.

That said, if people are really so annoyed by this, it may make sense to turn them off by default. But you could then also say that for MALLOC_PRODUCTION, for example. Why make everybody's memory allocations slow, while you really only intend to debug the core system? :)

I'd argue a little harder for MALLOC_PRODUCTION staying as-is since that's more broadly applicable. Though, we have src.conf knobs for both of these exactly so that folks can just flip them easily for their own use-cases without having to constantly argue defaults, and the current defaults reflect generally what we do with debug features in the kernel (as you noted).

[dim]

Okay, let's change the default then. It would be nice if you can put a reminder in the commit message that says you have to turn on LLVM_ASSERTIONS whenever you want to report an issue with the toolchain itself.