[libcxx-commits] [PATCH] D147860: [libcxxabi] [test] Mark code following an assert(false) as unreachable

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxxabi/test/cxa_vec_new_overflow_PR41395.pass.cpp:21
 
-void *dummy_alloc(size_t) { assert(false && "should not be called"); }
+void *dummy_alloc(size_t) { assert(false && "should not be called"); __builtin_unreachable(); }
 void dummy_dealloc(void*) { assert(false && "should not be called"); }
----------------
mstorsjo wrote:
> mstorsjo wrote:
> > EricWF wrote:
> > > mstorsjo wrote:
> > > > phosek wrote:
> > > > > 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.
> > > > I guess that's a reasonable fix too, I'll try that.
> > > If the compiler isn't figuring out that it's unreachable, are we sure assertions are enabled?
> > 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.)
> @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.
@phosek - Can you follow up here; which alternative do you prefer here - since the asserts overall are essential for the test anyway?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147860/new/

https://reviews.llvm.org/D147860