[libcxxabi] [test] Mark code following an assert(false) as unreachable

[mstorsjo]

Summary

With current versions of mingw-w64 headers, code following
assert(false) isn't considered unreachable - thus mark it
as such with __builtin_unreachable(), to avoid warnings (treated as
errors) for a missing return statement.

The root cause does get fixed further upstream in mingw-w64 in
https://github.com/mingw-w64/mingw-w64/commit/1690994f515910a31b9fb7c7bd3a52d4ba987abe
though.

[phosek]

libcxxabi/test/cxa_vec_new_overflow_PR41395.pass.cpp

18–19

Should we make the same change to all other dummy functions for consistency?

21

I wonder if we should instead use __bultin_trap() to ensure that the code doesn't continue executing even with assertions disabled? This was also discussed in https://discourse.llvm.org/t/llvm-unreachable-is-widely-misused/60587.

[mstorsjo]

libcxxabi/test/cxa_vec_new_overflow_PR41395.pass.cpp

18–19

I guess that could be good, especially wrt the arguments about uses of unreachable that you mentioned.

21

I guess that's a reasonable fix too, I'll try that.

[EricWF]

libcxxabi/test/cxa_vec_new_overflow_PR41395.pass.cpp

21

If the compiler isn't figuring out that it's unreachable, are we sure assertions are enabled?

[mstorsjo]

libcxxabi/test/cxa_vec_new_overflow_PR41395.pass.cpp

21

Yes, assertions are enabled. The reason is well understood here - assert(x) expands to (!!(x) || _assert(...)), https://github.com/mingw-w64/mingw-w64/blob/v10.0.0/mingw-w64-headers/crt/assert.h#L52-L53, where _assert() is a function that prints a message and aborts. The issue was that _assert() was lacking a noreturn attribute. With the noreturn attribute in place, the compiler can figure it out as expected. This is fixed upstream in https://github.com/mingw-w64/mingw-w64/commit/1690994f515910a31b9fb7c7bd3a52d4ba987abe now, but I'm working to get this testsuite running in the CI environment - and for this issue, I think it's nicer to work around the issue in the test rather than marking the whole test as UNSUPPORTED or XFAIL. (XFAIL is tricky also when we later will update the CI environment to a newer toolchain that does have the issue fixed.)

[mstorsjo]

libcxxabi/test/cxa_vec_new_overflow_PR41395.pass.cpp

21

@phosek - Tested, __builtin_trap() does work for fixing the issue too. But on second thought, I think there's not much point in doing that - the whole test becomes essentially pointless if asserts actually were to be disabled. Therefore I think __builtin_unreachable() is more straightforward for just telling the compiler to not care about that part of the codebase. Or alternatively just sticking in a dummy return nullptr; there - that doesn't seem to trigger any warnings/errors about the code being unreachable in cases where the assert is evaluated correctly.

[EricWF]

LGTM after addressing @phosek comment about the other delete functions.

Also please leave a comment explaining why assert isn't catching this.

[EricWF]

Oh, and of course, after the CI is passing. Please re-upload a new patch so that it merges.

[mstorsjo]

In D147860#4258788, @EricWF wrote:

LGTM after addressing @phosek comment about the other delete functions.

Also please leave a comment explaining why assert isn't catching this.

Sure, I can do that. As we’ve concluded that this test doesn’t make much sense, I think there’s little point in adding a similar marker in the other functions, as the only point of it is to silence the issue of missing a return. I’ll wait for @phosek to complete the discussion anyway, and I can still be persuaded to change it if he still wants it that way.

In D147860#4258793, @EricWF wrote:

Oh, and of course, after the CI is passing. Please re-upload a new patch so that it merges.

Sure, I can do that - although the only issues in CI seem to be the same sanitizers test configs that are (or were?) failing in all jobs.

[mstorsjo]

libcxxabi/test/cxa_vec_new_overflow_PR41395.pass.cpp

21

@phosek - Can you follow up here; which alternative do you prefer here - since the asserts overall are essential for the test anyway?

[phosek]

libcxxabi/test/cxa_vec_new_overflow_PR41395.pass.cpp

21

I think that __builtin_unreachable(); followed by a return nullptr; is fine.

[mstorsjo]

libcxxabi/test/cxa_vec_new_overflow_PR41395.pass.cpp

21

Ok, but if adding a dummy return nullptr; we could just as well omit the __builtin_unreachable()?

[phosek]

libcxxabi/test/cxa_vec_new_overflow_PR41395.pass.cpp

21

True, let's go with just return nullptr; then.

[mstorsjo]

Fix the warning by just adding a return nullptr;.

libcxxabi/test/cxa_vec_new_overflow_PR41395.pass.cpp

 
 // PR41395 isn't fixed until the dylib shipped with macOS 10.15
 // XFAIL: stdlib=apple-libc++ && target={{.+}}-apple-macosx10.{{9|10|11|12|13|14}}
 
 #include "cxxabi.h"
 #include <new>
 #include <cassert>
 
 void dummy_ctor(void*) { assert(false && "should not be called"); }
 void dummy_dtor(void*) { assert(false && "should not be called"); }
 
-void *dummy_alloc(size_t) { assert(false && "should not be called"); }
+void *dummy_alloc(size_t) { assert(false && "should not be called"); return nullptr; }
 void dummy_dealloc(void*) { assert(false && "should not be called"); }
 void dummy_dealloc_sized(void*, size_t) { assert(false && "should not be called"); }
 
 
 bool check_mul_overflows(size_t x, size_t y) {
   size_t tmp = x * y;
   if (tmp / x != y)
     return true;