Assertions cleanups, annotate unreachable code

[MisterDA]

This PR sports small cleanups around assertions in the runtime.

• Use a definition of unreachable() to mark unreachable code where it makes sense, and allow C compilers to optimize the code.
• Split conjunctions of CAMLassert(A && B); into CAMLassert(A); CAMLassert(B) to better identify which part of the assertion fails.
• Some CAMLassert can be replaced with static_assert.

(using the unreachable annotation will later be useful to optimize ocamlc too!)

No change entry needed, I think.
cc @NickBarnes

[dustanddreams]

I'd rather see this defined as

Suggested change

	#define unreachable() (abort())
	#define unreachable() (assert(0))

so that, should it ever fire, this gets recognized as an assertion failure rather than a mysterious SIGABRT.

(And yes, given how eager clang and gcc are at arm-wrestling the C specs in order to win at picobenchmarks, I am expecting such unreachable code paths to be reachable when compiled with these compilers at some of their optimization levels).

[MisterDA]

But what if C assertions are disabled with NDEBUG? The code path may then be missing a return statement since there won't be a noreturn function to end it.
Similarly, if we used CAMLassert instead, it would result in an empty statement in non-debug runtimes, triggering the same problem.
If I keep an assertion and add a return statement, I'll probably get warnings about an unreachable return statement after the unreachable annotation…

[dustanddreams]

There is no simple way to solve this without making every compiler combination happy. You probably should not remove the few return following unreachable then.

[MisterDA]

The compilers differ too much in behavior… but in my test they complain about missing returns rather that unreachable code. I've changed my code to use CAMLassert here.

[gasche]

I am nervous about unreachable. If I understand correctly, before we used CAMLassert(0), which means "in debug mode have a clean error, otherwise do nothing", and then we would return something if the type was not void. Now unreachable means "reaching here is undefined behavior".

In the cases where we know for sure that this place is dead code, unreachable is fine. But if there are scenarios of misuse where these places do get reached, typically a programming error in our runtime or in FFI code, then users are going to get weird segfaults instead of "a clean error in debug mode, and some non-crashing fallback otherwise". This could be a degradation, and ensuring that it's fine requires a careful look at the code.

[MisterDA]

I am nervous about unreachable. If I understand correctly, before we used CAMLassert(0), which means "in debug mode have a clean error, otherwise do nothing", and then we would return something if the type was not void. Now unreachable means "reaching here is undefined behavior".

That is my understanding too.

In the cases where we know for sure that this place is dead code, unreachable is fine. But if there are scenarios of misuse where these places do get reached, typically a programming error in our runtime or in FFI code, then users are going to get weird segfaults instead of "a clean error in debug mode, and some non-crashing fallback otherwise". This could be a degradation, and ensuring that it's fine requires a careful look at the code.

These are valid concerns that I'll try to address. I've remembered in the meantime that GCC can warn whenever a switch statement has an index of enumerated type and lacks a case for one or more of the named codes of that enumeration (see -Wswitch, included in -Wall). This alleviates a bit the need for the unreachable annotation, clarifies the code, and adds a bit of type safety (yay!). Now, we need to convince ourselves that values outside of the enumeration space cannot sneak in, and I think that's achievable.

The diff of bigarray.h has changed to extract masks and layout position from the enumerations, but I don't think this changes the effective interface.

[dustanddreams]

These are valid concerns that I'll try to address. I've remembered in the meantime that GCC can warn whenever a switch statement has an index of enumerated type and lacks a case for one or more of the named codes of that enumeration (see -Wswitch, included in -Wall). This alleviates a bit the need for the unreachable annotation, clarifies the code, and adds a bit of type safety (yay!). Now, we need to convince ourselves that values outside of the enumeration space cannot sneak in, and I think that's achievable.

I'm willing to bet the CI will expose at least one compiler which will nevertheless emit warnings for this (its name probably starts with "MS" and ends with "VC").

[MisterDA]

I'm a bit disappointed by my last endeavor. GCC, clang, and MSCV have oh-so-subtly different behaviors regarding unreachable warnings and switch exhaustiveness check. This makes the code somewhat more verbose than I've previously hoped.
I all cases, we don't expect values outside of the possible range of the enumeration, and using default: unreachable() covers that and the compilers' wrath.

[dustanddreams]

Aside the previous remark, I'm satisfied with the changes as they are now.

[shindere]

Antonin Décimo (2024/06/19 01:58 -0700):

In the meantime I'm tempted to leave this as-is an revisit later, if that's fine.

I'll trust you on that, @dustanddreams?

[shindere]

Miod Vallat (2024/06/19 02:02 -0700):

@dustanddreams commented on this pull request. > @@ -1418,7 +1418,7 @@ do_resume: { #ifndef THREADED_CODE default: #ifdef _MSC_VER - __assume(0); + unreachable(); That's fine.

Ah sorry, didn't see this.

[shindere]

Ah, this now needs to be rebased to resolve the conflict in Changes.

Feel free to ping me when done.

[MisterDA]

Rebase is done. Maybe @gasche still has comments?

[gasche]

My comment remains that if some of those unreachable() are in fact reachable in case of programming bugs by the user, then I would prefer something that fails cleanly than nasty undefined behavior, especially in debug mode -- but why not all the time?

I only looked at the new version briefly but my impression is that it suffers from the same problem. For example:

• you use explicit casts for bigarray masks (and that's nice), but unless I am very C-confused this does not bring any additional dynamic safety, and a programming error in the runtime or in the user FFI code could still bring an unexpected value there?
• in the bytecode interpreter you point out one place where UB-undefined may bring performance benefits, but if I understand correctly this is the place that handles unknown opcodes; it occurs from time to time that the interpreter is fed an unknown opcode (for example when we add new bytecode instructions and forget to update the magic number), and a user-friendly error in this situations is very helpful. To me this calls for a "this is a cold place that is very rarely reached" annotation, not a "do whatever you want" annotation.

[MisterDA]

My comment remains that if some of those unreachable() are in fact reachable in case of programming bugs by the user, then I would prefer something that fails cleanly than nasty undefined behavior, especially in debug mode -- but why not all the time?

For debug mode, there's a possible alternative implementation where we trap the program if it reaches an unreachable annotation.

I only looked at the new version briefly but my impression is that it suffers from the same problem.

You're right, but...

• you use explicit casts for bigarray masks (and that's nice), but unless I am very C-confused this does not bring any additional dynamic safety, and a programming error in the runtime or in the user FFI code could still bring an unexpected value there?

I can convince myself that the runtime currently cannot fall in the default cases. It's true that it's true that this doesn't prevent us from future errors. As for user FFI code, to me this example shares a lot in common with converting the sum type ocaml representation to and from a C array of integers (think socket flags, or caml_convert_flag_list). We usually don't bound-check them and just access the array at the offset represented by the tag. We cannot protect against a C array being shorter than the type exposed on the OCaml side, or having wrong values. Here too I think we could forget about these programming errors.

• in the bytecode interpreter you point out one place where UB-undefined may bring performance benefits, but if I understand correctly this is the place that handles unknown opcodes; it occurs from time to time that the interpreter is fed an unknown opcode (for example when we add new bytecode instructions and forget to update the magic number), and a user-friendly error in this situations is very helpful. To me this calls for a "this is a cold place that is very rarely reached" annotation, not a "do whatever you want" annotation.

On the other hand the error case for the interpreter seems to be only handled for the non-threaded code interpreter (debug mode or MSVC). Am I reading the code wrong? What happens when (threaded-code) ocamlrun encounters a bad opcode at the moment?

Adding unreachable annotations in switch cases is supposed to help the compiler optimize the jumps. I'll try to provide some benchmarks later if we still need more motivation.

[gasche]

Here too I think we could forget about these programming errors.

I see no benefit to making our system less usable/debuggable in this way. (In particular I very much doubt that there is any measurable performance benefit -- in the bigarray stuff.) Why do you want to do this? My impression (but I may be wrong and I'm not judging in any way) is that you think that unreachable() is the standard/common/good-practice thing to do in C. But we would benefit more from a clean fatal-error than from undefined behavior.

In terms of gcc intrisics, I think we want __builtin_trap rather than __builtin_unreachable.

Adding unreachable annotations in switch cases is supposed to help the compiler optimize the jumps. I'll try to provide some benchmarks later if we still need more motivation.

Again, this could be done with a hint that says "this code path is unlikely" and then follows with a fatal error, we don't need to introduce more undefined behaviors in our programs to get code-generation benefits.

[MisterDA]

Thank you for the pointers, that was really helpful. I've changed the definition of CAMLunreachable to trap if the compiler supports it, or use CAMLassert(0) if not (the current state).

[gasche]

This change may have a negative performance impact, I'm not sure. Have you tested it?

[gasche]

(The test that I have in mind: use a dumb microbenchmark that does something compute-heavy for long enough, and compare the performance using ocamlrun before and after the change. If you don't see any noticeable performance difference then we should be fine.)