Define CAMLunreachable in GCC < 10

[MisterDA]

This also reverts commit 034f273.

Tested in Ubuntu 20.04 with GCC 9 in a Docker image.

$ cd ocaml
$ git checkout XXX # this branch or trunk
$ docker run --pull=missing --rm -v .:/mnt gcc:9 sh -c \
    'cd /mnt && git clean -fdX && ./configure --disable-ocamldoc && make -j$(nproc) runtime/ocamlrun'

Reported by @nojb in #13242 (comment).
cc @gasche

[shindere]

Miod Vallat (2024/07/24 01:33 -0700):

@dustanddreams approved this pull request. > @@ -382,7 +382,9 @@ CAMLexport int caml_ba_compare(value v1, value v2) case CAML_BA_CAML_INT: case CAML_BA_NATIVE_INT: DO_INTEGER_COMPARISON(intnat); - default: CAMLunreachable(); + default: /* Should not happen */ + CAMLunreachable(); + return 0; /* Keeps GCC -Wall happy */ Maybe mention `GCC 9` rather than simply `GCC`, so that next time someone wonders whether this line is still needed, that person can check whether gcc 9 is still in use somewhere and able to build the current OCaml codebase.

Thanks for both the PR and the review. Just approved. Good to go once the suggestion above has been taken into account and CI passes.

[gasche]

I would like to understand a bit more what is going on here:

• there are many versions of CAMLunreachable depending on conditional tests, which one is gcc 9 using?
• is the problem that this version always aborts, but gcc 9 does not notice this, or that this version sometimes does not abort?

[gasche]

I had a look myself:

• the definition used for gcc 9 is CAMLassert(0), which indeed does not always abort the program (only in debug mode)
• the reason why gcc 9 uses this definition is that it does not support __has_builtin, but it in fact has __builtin_trap available, it was introduced in gcc 4.3 released in march 2008.

[MisterDA]

GCC 9 doesn't have __builtin_trap, so CAMLunreachable defaults to CAMLassert(0). The problem is that CAMLassert(0) defaults to an empty expression ((void)0) in the default runtime, which doesn't stop the program execution. GCC then errors because either the function ends without a return, or because some values are left uninitialized as the control flows continues out of the switch.
This PR returns to the default behavior before #13242 of using dummy return values in case the compiler doesn't support trap. One of the first iterations of #13242 used abort() in this case.

[MisterDA]

the reason why gcc 9 uses this definition is that it does not support __has_builtin, but it in fact has __builtin_trap available, it was introduced in gcc 4.3 released in march 2008.

oh, missed that, thanks

[gasche]

This suggests the following:

• the specific issue with gcc could be fixed by checking that (gcc >= 4.3), if we are not already assuming this, as an alternative to the __has_builtin(__builtin_trap) check
• or we could check for __builtin_trap at configure-time, which is more portable
• I wonder if there is an alternative fallback case that always interrupts evaluation. One way to do this would be to define CAMLunreachable1(x) instead of CAMLunreachable(), where x is (evaluated and) returned only in the fallback case. Maybe there exists a C-friendly way to do this without asking the user for a dummy value?

[MisterDA]

We're already using __has_builtin(XXX) || defined(__GNUC__) in other places, I think that's the simplest.

[gasche]

Yep, our configure requires (gcc >= 4.9), so it would suffice to check __GNUC__ to enable the __builtin_trap version, as an alternative to the __has_builtin check.

[gasche]

This looks good, do I correctly understand that you tested the new approach on gcc 9?

[gasche]

We may still have an issue with older compilers or compilers that do not have __builtin_trap(), where the current fallback definition is incorrect outside debug mode. Maybe the fallback should be abort() instead?

[gasche]

(Note: I'm fine with merging the PR as-is, which fixes the immediate issue with gcc 9, and letting you cook up a longer-term fix for the fallback case separately.)

[MisterDA]

We may still have an issue with older compilers or compilers that do not have __builtin_trap(), where the current fallback definition is incorrect outside debug mode. Maybe the fallback should be abort() instead?

From LLVM llvm.trap Intrinsic docs:

This intrinsic is lowered to the target dependent trap instruction. If the target does not have a trap instruction, this intrinsic will be lowered to a call of the abort() function.

Older compilers are less of a problem because they tend not to support C11 atomics. For compilers not supporting __builtin_trap() I'm also in favor of using abort().

[gasche]

I merged the current PR because it is well-scoped, and presumably this will make @nojb's life easier.

I would be interested in experiments with abort() instead of CAMLassert(0) for the fallback. (I am also curious to know about how it is when you actually run this code in practice: CAMLassert shows line numbers for example, does abort() also do this in practice? Or maybe we should use assert(0); abort(); to get the best of both words, even in the non-debug runtime?)

[nojb]

I merged the current PR because it is well-scoped, and presumably this will make @nojb's life easier.

Thanks!

[nojb]

I merged the current PR because it is well-scoped, and presumably this will make @nojb's life easier.

Thanks!

(and I confirm that OCaml is again buildable on Ubuntu 20.04)