Add static asserts to gsl::narrow for template parameter pairs that can never work

[B3ret]

I just ran into this with some templated code.
Basically some pairs of template parameters in gsl::narrow will almost always fail at runtime, e.g., gsl::narrow<float, double> and gsl::narrow<double, int>.

I know it is a clear progammer's mistake to do this, but maybe we can static assert, or at least warn about pairs that are very unlikely to be correct?

[e00E]

Allowing narrow casts from floating point to integer is a footgun that should be forbidden.

Like the OP said, it makes little sense when the source type allows non integer values and the target type does not. Here the narrowing is not a sufficient condition to describe all invalid values so a narrow cast is the wrong tool.

Furthermore it is undefined behavior to cast a floating point value to an integer that it is out of bounds for like with gsl::narrow<uint8_t, float>(256.0) [0]. Even when throwing, this is what gsl::narrow does internally so it invokes UB.

[0] https://github.com/e00E/cpp-clamp-cast

[dmitrykobets-msft]

Hi, apologies for the delay on this issue -- this is a helpful finding and will be brought up in the next maintainers' sync.

[hsutter]

Editors call: Good point, thank you.

Regarding the use of UB internally: It's okay and if anyone is worried about it the use of UB is benign on the platforms we target (e.g., they don't involve hitting any hardware trap representations for these types). However, it might be good to put a [[gsl::suppress]] or a comment to indicate we're deliberately violating the Guidelines rule to avoid UB.

Regarding static-asserting certain types that nearly always fail round-trip: We think the static_asserts will defeat the purpose of narrow because there are still values for which it is defined.

[e00E]

@hsutter

Regarding the use of UB internally: It's okay and if anyone is worried about it the use of UB is benign on the platforms we target (e.g., they don't involve hitting any hardware trap representations for these types). However, it might be good to put a [[gsl::suppress]] or a comment to indicate we're deliberately violating the Guidelines rule to avoid UB.

(I must have missed this reply before, just randomly checked up on this issue and saw it.)

This is a dangerous argument to make. It is not sufficient to argue that this behavior works in practice because the hardware does the right thing. If the compiler proves you are invoking UB then it is allowed to make the program do anything. Whether the assembly you expect to be emitted does the right thing does not matter. What matters is what happens in the C++ abstract machine. There is no benign UB.

Please correct me if I am wrong about this. I did not want to spend the time now but if we disagree I will find more sources in favor of my point.

I agree that at the moment with existing compilers this code is fine. This isn't good enough. At any point, a standard compliant compiler could become smarter and exploit this property, turning previously working code into a security vulnerability.

[RalfJung]

Regarding the use of UB internally: It's okay and if anyone is worried about it the use of UB is benign on the platforms we target

So... you don't target clang then? Because clang, via its LLVM backend, will use the assumption that a float-to-int cast fits the target type to, for example, optimize away bounds checks. (Here's an example of that happening in Rust before we fixed this UB.) That's a perfectly confirming C++ compiler.

There is no benign UB.

💯
Nobody who is serious about memory safety can talk about "benign UB". There is no such thing.

[carsonRadtke]

I'll raise this issue in the next internal GSL sync. I'd agree with y'all that this behavior: https://godbolt.org/z/4Tr1fe9xG is undesirable.