> Ideally we'd have a fuzzer that runs CheckedNumeric ops with fuzzed numbers under ubsan, to get some confidence that this code works. And then our compiler friends can no longer make fun of the irony of having undefined behavior in our safe_math code :-)

There was a conscious tradeoff here that I should explain. Basically, I lump undefined behavior into two classes:

Strong - Execution logic depends in some way on the result of an operation with undefined behavior (e.g. triggering signed overflow and branching on or otherwise using the result).
Weak - Undefined behavior is triggered, but no program logic depends on it and no resulting values are ever used (e.g. triggering signed overflow, but never using the value because the overflow was detected via well-defined behavior).

Originally I wrote this code allowing weak but not strong undefined behavior, which is an invariant I'm fairly certain still holds. I took this approach because it allowed me to remove numerous branches in the most common code paths.

Since we started running ubsan I've been approving patches that remove several of these occurrences of weak undefined behavior. However, I am a bit concerned that we're just hurting performance and increasing code complexity while not actually fixing any real bugs. Because I'm not aware of any compilers that would break the code on weak undefined behavior, and in fact I would argue that such a thing would represent a compiler bug.

Here's an alternative proposal: If my assumptions about weak undefined behavior are reasonable and we can rely on the compiler doing what we expect in these cases--which the current test coverage verifies--how about we just keep the optimizations and annotate these locations to silence the tools?

From a language lawyer perspective, "weak undefined behavior" isn't. This is similar to "benign races" (https://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong): Compilers optimize in creative and unpredictable ways (see e.g.  bug 669642  where some undefined behavior was harmless in practice until last week, when it broke exclusively on chrome/android/clang).

From a pragmatic perspective, we have all kinds of undefined behavior in chrome, most of it unintentionally. But some things are intentional, we for example build with -fno-strict-aliasing which disables some optimizations that will only break code with undefined behavior (but casting memory is impossible without undefined behavior, bit_cast<>() is not always convenient to use, and the union aliasing technique is C-only and even there not fully standardized afaik, so doing the Right Thing there depends on the standard deciding on what the Right Thing is), we deref 0 to force crashes in a few places, etc.

So I think what we want to do is use UBSan to find undefined behavior, start with a small list of undefined behavior, and then in priority order add more and more checks as we clean up the codebase. If we find checks that require unreasonable fixes, we could decide to not turn them on (and then I suppose give feedback to the C++ standards folks in some form).

It sounds like you're claiming that the double->float casts are in that category. It's possible that that's true. http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html lists only fsanitize=float-cast-overflow which covers both float->int (which we know can cause miscompiles) and double->float, so I think we'd want to split that flag in two in ubsan, and then turn on only one of the two (at least for now) and collect data to make a case that maybe double->float shouldn't be undefined or something.

But just leaving undefined behavior and hoping for the best guarantees that that code will break one day.

You kinda danced around my question, and the comparisons to race conditions aren't aren't really valid because all of the examples are clearly not benign, and are in fact relying on the result of undefined behavior.

So, back to my question, I agree that we should not have any code that relies directly on undefined behavior. However, we keep adding additional code and complexity here where we do not actually rely directly on the result of undefined behavior. Taking the case of the conversion from an out-of-bounds float to an integral, what's the harm if the value is never used because well-defined behavior has determined it would be invalid? Why can't we just use the simpler, more performant approach of allowing the operation unconditionally, annotating it, and ensuring we have test coverage? If the compiler removes the undefined conversion when it detects it--which seems like the most extreme thing it could do--then isn't that all the better, since we were never going to use that value anyway?

> Taking the case of the conversion from an out-of-bounds float to an integral, what's the harm if the value is never used because well-defined behavior has determined it would be invalid?

The harm in that case was that the compiler figured that since an out-of-bounds float is undefined behavior it can assume that the result will fix in a byte (if the integral was an uint8_t), and then it packed that byte together with the is_valid byte into an uint16_t, and when the out-of-bounds float got written to that uint16_t, the bits of the float larger than 8 byte spilled into the 8 byte of the is_valid field, which broke things.

For double->float conversions we can't think of anything like this yet, but something similarly abstruse could happen later. I'd wager that we don't have a ton of CheckedNumeric<float>, so I'm not sure handling this correctly would be very expensive. (We likely have a ton of static_cast<float>(my_double) and all of that is technically wrong but works in practice -- maybe doubles happen to be smaller than FLT_MAX in practice, or compilers don't exploit this type of undefined behavior yet).

> The harm in that case was that the compiler figured that since an out-of-bounds float is undefined behavior it can assume that the result will fix in a byte (if the integral was an uint8_t), and then it packed that byte together with the is_valid byte into an uint16_t, and when the out-of-bounds float got written to that uint16_t, the bits of the float larger than 8 byte spilled into the 8 byte of the is_valid field, which broke things.

All of the inappropriate spillage is why compiler devs never get invited to the good parties.

Is this done?

I don't know. I fixed the obvious floating point thing, but you cited "a bunch" in the title, and I'm not sure where that measurement comes from.

Running base_unittests with fsanitize=float-cast-overflow enabled. I'll try again and see how it goes.

Gonna assume I cleaned this up sufficiently.