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

[ghost]

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…

[ghost]

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.

[ghost]

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.

[ghost]

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.)

[MisterDA]

I've struggled to show that retaining unreachable() in the interpreter brings more than marginally performance benefits. Maybe a more intensive task, or one that would run even longer would show more impressive results.

I tried with merkletrees/2.ml from the programming language benchmarks, with this patch applied to trunk (disabling unreachable), and the current PR (enabling unreachable) (rebased on the same trunk):

diff --git a/runtime/interp.c b/runtime/interp.c
index 3518d8a4cb..baad591fa5 100644
--- a/runtime/interp.c
+++ b/runtime/interp.c
@@ -1417,13 +1417,9 @@ do_resume: {

 #ifndef THREADED_CODE
     default:
-#ifdef _MSC_VER
-      __assume(0);
-#else
       caml_fatal_error("bad opcode (%"
                            ARCH_INTNAT_PRINTF_FORMAT "x)",
                            (intnat) *(pc-1));
-#endif
     }
   }
 #endif

trunk>hyperfine "runtime\ocamlrun.exe .\ocaml.exe -I stdlib ..\..\merkletrees-2.ml 20"
Benchmark 1: runtime\ocamlrun.exe .\ocaml.exe -I stdlib ..\..\merkletrees-2.ml 20
  Time (mean ± σ):     211.071 s ±  2.631 s    [User: 209.878 s, System: 0.744 s]
  Range (min … max):   207.575 s … 214.750 s    10 runs

assertions>hyperfine "runtime\ocamlrun.exe .\ocaml.exe -I stdlib ..\..\merkletrees-2.ml 20"
Benchmark 1: runtime\ocamlrun.exe .\ocaml.exe -I stdlib ..\..\merkletrees-2.ml 20
  Time (mean ± σ):     207.112 s ±  3.019 s    [User: 205.886 s, System: 0.642 s]
  Range (min … max):   203.065 s … 211.719 s    10 runs

The benchmarks were done on a x64 4-cores Windows 11 VM using MSVC 19.41.33923.

[MisterDA]

In the latest revision, I've introduced two new macros: CAMLtrap(), and unreachable() (for before C23).
We may use CAMLtrap() to quickly halt the program execution if it reaches an invalid path. Using CAMLtrap() rather than CAMLassert improves the intent of the code, and works even if assertions are disabled.
We may use unreachable() to tag code paths as unreachable and allow the C compiler to optimize code. Currently, the unreachable() annotation is only used in the interpreter if the C compiler doesn't support the labels-as-values extension, to allow the C compiler to somewhat optimize jumps, at the expense of stopping cleanly if it encounters an invalid opcode (this is the current behavior).

[MisterDA]

The PR doesn't change the behavior of the interpreter if it encounters an invalid opcode. I think that's better reexamined in another PR or issue.

[dra27]

Shouldn't this be here? (cf. L1140)

Suggested change

	break;
	break;
	default: CAMLtrap();

[MisterDA]

I've thought about this a bit more, and the bigarray layout is represented by a single bit (#define CAML_BA_LAYOUT_MASK 0x100). There are no invalid values here, if the compiler warns, we can mark it as unreachable (if ever we start using -Wswitch-default or -Wswitch-enum, but they can make the unnecessarily verbose).

[MisterDA]

so I've removed the default case.

[MisterDA]

Ah, the default case is needed otherwise the compiler thinks there are code paths leaving some variables uninitialized. I'll add default: unreachable(); for switches on the bigarray layout.

[MisterDA]

I've rebased this PR and added a fix for an incorrect assertion I've found using cppcheck in the meantime. Let me know if there's more you'd like to see or unsee here.

[gasche]

I had yet yet yet another look at this PR.

The changes are mostly in the default branches of switches that we believe are exhaustive in practice. (Plus one change on the landing code for unsupported bytecodes.)

In some of those default switches you call CAMLtrap(), which is intended to trap, and on some others you call unreachable(), which is intended to be undefined-behavior. The fact that we do two different things rubs me the wrong way. I would prefer if we used a single macro, which is intended to mean "we believe that this point cannot be reached" and which traps. It would be simpler and safer. Is there a good reason not to do this?

(I think that unreachable() would be a perfect name for this macro, because I think that this is what a macro called unreachable() should do. Obviously the designers of C compilers have favored a different design with __builtin_unreachable as they favor performance over correctness. If using unreachable() to mean trap() is shocking to you, maybe impossible()?)

[MisterDA]

I had yet yet yet another look at this PR.

Thanks a lot for your time and energy!

The changes are mostly in the default branches of switches that we believe are exhaustive in practice. (Plus one change on the landing code for unsupported bytecodes.)

In some of those default switches you call CAMLtrap(), which is intended to trap, and on some others you call unreachable(), which is intended to be undefined-behavior. The fact that we do two different things rubs me the wrong way. I would prefer if we used a single macro, which is intended to mean "we believe that this point cannot be reached" and which traps. It would be simpler and safer. Is there a good reason not to do this?

I'm finally using unreachable only when switching on the layout of the bigarray, which is defined by a single bit. I think it makes sense to use unreachable in the default case of this branch, but I'm ready to admit that I may have been overzealous and revert to the previous if (bigarray_has_c_layout) ... else .... Switching to trapping here would introduce a third useless branch. This is IMO an advantage of using unreachable and trap annotations independently.

(I think that unreachable() would be a perfect name for this macro, because I think that this is what a macro called unreachable() should do. Obviously the designers of C compilers have favored a different design with __builtin_unreachable as they favor performance over correctness. If using unreachable() to mean trap() is shocking to you, maybe impossible()?)

This would be fine by me. C23 defines unreachable as a macro, so we can safely override it.
If you're still not convinced we could use both, I'll revert to if/else branches in the bigarray layout switch, and define unreachable as trapping (in debug and normal mode).

I've also experimented with -Wswitch-default (forces to add a default case) and -Wswitch-enum (forces to handle explicitly all enum cases) but both forces us to add verbose code, often not that useful.

[gasche]

Switching to trapping here would introduce a third useless branch. This is IMO an advantage of using unreachable and trap annotations independently.

It is the job of the compiler to check that the branch is useless and remove it. If they get it wrong, at worst the program is slightly slower. It should not be the job of the author to decide between "helpful macro that will print a user message if I get it wrong" and "shoot myself in the foot", unless you can demonstrate that (1) compilers do get it wrong, and (2) the performance benefits in this specific case matter and justify the loss of safety.

I was okay with if-then-else branches in the bigarray machinery, and I am also okay with switches that are slightly more verbose but also more explicit. But I don't want, as the author/reviewer/maintainer/evolver of this code, to have to think hard about which of the two macros I should use, unless you can demonstrate clear benefits to imposing this complexity on us.

[gasche]

I tried entering the following code in goldbolt:

enum caml_ba_layout {
  CAML_BA_C_LAYOUT = 0,           /* Row major, indices start at 0 */
  CAML_BA_FORTRAN_LAYOUT = 0x100, /* Column major, indices start at 1 */
};
#define CAML_BA_LAYOUT_SHIFT 8    /* Bit offset of layout flag */
#define CAML_BA_LAYOUT_MASK 0x100 /* Mask for layout in flags field */

#define layout(v) (enum caml_ba_layout)(v & CAML_BA_LAYOUT_MASK)

extern void f(void);
extern void g(void);

/* Type your code here, or load an example. */
void test(int n) {
    switch (layout(n)) {
        case CAML_BA_C_LAYOUT: return (f());
        case CAML_BA_FORTRAN_LAYOUT: return (g());
        default: __builtin_trap();
    }
}

With GCC 14.1, with -O0 I get code generated for the default: case, but starting at -O1 it is removed and there is just one branch between the two possible options.

• with -O0
• with -O2

[MisterDA]

This makes sense. Thanks for your comments. Now unreachable() traps the program. I did not think the compiler would eliminate the branch, and I should have tested this.

[gasche]

If I understand correctly, in C++ unreachable() already exists and means "undefined behavior", and if CAML_INTERNALS is set and you include caml/misc.h in your code then it gets redefined below to "trap". This seems like a potential surprise that could be avoided by calling this CAMLunreachable instead of unreachable or something. I wonder if people have opinion on whether we should do this or keep the current unreachable() proposal. (I'm not too sure what would be the assumptions of C/C++ programmers writing INTERNALS-only OCaml FFI code.)

Otherwise the rest of the PR looks fine to me now, thanks!

[MisterDA]

I agree that CAMLunreachable is an extra safer step for C++ compatibility.

[gasche]

Approved again. Thanks for the masterful history rewriting.

[MisterDA]

Thanks for your insights and your perseverance ;)

[nojb]

@MisterDA Unfortunately, this PR breaks compilation on Ubuntu 20.04:

nojebar@PERVERSESHEAF:~/ocaml$ make
make coldstart
make[1]: Entering directory '/home/nojebar/ocaml'
  CC runtime/bigarray.b.o
runtime/bigarray.c: In function ‘caml_ba_compare’:
runtime/bigarray.c:389:1: error: control reaches end of non-void function [-Werror=return-type]
  389 | }
      | ^
runtime/bigarray.c: In function ‘caml_ba_get_N’:
runtime/bigarray.c:759:1: error: control reaches end of non-void function [-Werror=return-type]
  759 | }
      | ^
runtime/bigarray.c: In function ‘caml_ba_slice’:
runtime/bigarray.c:1137:9: error: ‘sub_dims’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
 1137 |   res = caml_ba_alloc(b->flags | CAML_BA_SUBARRAY,
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1138 |                       b->num_dims - num_inds, sub_data, sub_dims);
      |                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
runtime/bigarray.c:1135:12: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
 1135 |     offset * caml_ba_element_size[b->flags & CAML_BA_KIND_MASK];
      |     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
runtime/bigarray.c: In function ‘caml_ba_sub’:
runtime/bigarray.c:1213:9: error: ‘mul’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
 1213 |     ofs * mul * caml_ba_element_size[b->flags & CAML_BA_KIND_MASK];
      |     ~~~~^~~~~
runtime/bigarray.c:1209:47: error: ‘changed_dim’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
 1209 |   if (ofs < 0 || len < 0 || ofs + len > b->dim[changed_dim])
      |                                               ^
cc1: all warnings being treated as errors
make[1]: *** [Makefile:1534: runtime/bigarray.b.o] Error 1
make[1]: Leaving directory '/home/nojebar/ocaml'
make: *** [Makefile:843: world.opt] Error 2

The compiler is:

nojebar@PERVERSESHEAF:~/ocaml$ gcc --version
gcc (Ubuntu 9.4.0-1ubuntu1~20.04.2) 9.4.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.