Details
- Reviewers
ldionne • Quuxplusone Mordante var-const - Group Reviewers
Restricted Project - Commits
- rG2a8f9a5e95de: [libc++] Implement P0627R6 (Function to mark unreachable code)
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/__utility/unreachable.h | ||
---|---|---|
19 | @ldionne @mstorsjo perhaps: What's the best way to get this to do __assume(false) on MSVC? Should we introduce a __libcpp_unreachable()?... Oh wait, we already have one. In libcxx/include/cstdlib we have: #ifdef __GNUC__ #define _LIBCPP_UNREACHABLE() __builtin_unreachable() #else #define _LIBCPP_UNREACHABLE() _VSTD::abort() #endif Let's do something in this PR to normalize the situation. Perhaps involving __has_builtin, perhaps involving defined(_MSC_VER), but the end result should be that libc++'s std::unreachable is implemented in terms of _LIBCPP_UNREACHABLE. |
libcxx/include/__utility/unreachable.h | ||
---|---|---|
19 | FWIW, in practice, all our cases of "MSVC" in our codebase is clang-cl only, as libcxx isn't usable with MSVC proper - so even in an MSVC/non-GNUC config, it would end up supporting Clang's builtins. (But a __has_builtin check doesn't hurt of course - and might be needed for other non-Clang, non-GCC compilers, if we support any such). |
- Introduce __unreachable() for previous standard versions and remove _LIBCPP_UNREACHABLE()
We should add a test, at least a .compile.pass.cpp test that we're defining the function and that it returns void. Also:
- We should update the feature-test macro
- We should add a release note for this feature (I want to be aggressive about release-noting the features we implement)
- We should also add a libc++ specific test (should be a .fail.cpp) that executes std::unreachable(), which would confirm that we somehow stop the execution of the program when encountered. Basically, I'd like a test that fails if std::unreachable() were implemented as an empty function. This is libc++ specific since technically, executing it is undefined behavior (but we know what we actually do).
libcxx/include/__utility/unreachable.h | ||
---|---|---|
19 | I believe we should use __has_builtin(__builtin_unreachable) here instead -- that would avoid hardcoding compiler knowledge. |
How do you expect the libc++-specific test to look? The compiler won't ignore that there is a __builtin_unreachable in the way and just remove all the code after that. We don't control the behavior in this case. We don't even have to do anything in unreachable(). It can just be empty and marked with [[noreturn]] and it would work pretty much the same. https://godbolt.org/z/GvbnG76nW
libcxx/include/__utility/unreachable.h | ||
---|---|---|
19 | IMHO we should name this function __libcpp_unreachable; just __unreachable seems in danger of stepping on some compiler's builtin or something. | |
libcxx/include/charconv | ||
91 | Can we remove this? If not, let's at least remove the comment (which is wrong now). | |
libcxx/include/fstream | ||
188 | Can we remove this now? (The answer, if "yes", will depend partly on politics: are we OK with breaking users who #include <fstream> and then call malloc or whatever.) | |
libcxx/src/filesystem/filesystem_common.h | ||
13 | As you probably noticed, right now this file uses #include "..." over #include <...>, and doesn't include any detail headers directly. I think there's no particular reason to preserve these invariants, but probably we want to s/""/<>/ in this file before we start mixing <> into it. | |
16–17 | Can we remove this now? |
Sorry, .fail.cpp wasn't accurate. In <test>.pass.cpp:
// XFAIL: * int main(int, char**) { std::unreachable(); return 0; }
When you run this, it will do zsh: illegal hardware instruction ./a.out if __builtin_unreachable() is used, and it will call std::abort() otherwise. In both cases, the executable will return with non-zero. And since the test is XFAILed, it will expect that non-zero and be reported as an expected failure.
If we implemented std::unreachable() as an empty body, it would result in "unexpected pass".
libcxx/test/std/utilities/utility/utility.unreachable/unreachable.compile.pass.cpp | ||
---|---|---|
9 | This needs some standard requirements. I suggest running just the test you're adding locally under various standard modes (build/bin/llvm-lit -sv libcxx/test/<path-to-test> --param std=c++XX) to shake out basic failures and get faster turn around time. |
I don't think that's guaranteed. __builtin_unreachable() just means "assume false"; it doesn't necessarily translate into a ud2 AFAIK, especially if we're talking about something that will work on both GCC and Clang.
Even on Clang at -O0, it seems to just flow off the unreachable end of main into whatever code follows that in memory: https://godbolt.org/z/fz6hh76P5
However, I think we could do a verify.cpp test: namely,
[[noreturn]] void f() { std::unreachable(); }
should not trigger Clang's diagnostic warning: function declared 'noreturn' should not return [-Winvalid-noreturn]. If it does, that's a bug.
It will work even with an empty body. [[noreturn]] already informs the compiler that the function never returns. https://godbolt.org/z/qc97n59Kc. In this example The return value is even 0. There is just no way to verify the behavior. https://godbolt.org/z/fPGEYfEWv returns 1. The test is just to unstable.
libcxx/test/std/utilities/utility/utility.unreachable/unreachable.compile.pass.cpp | ||
---|---|---|
9 | Yeah I sometimes forget. |
Oh right, that's why I was seeing invalid instruction. For some reason I thought Clang was inserting something like a __builtin_trap(). I stand corrected.
However, I think we could do a verify.cpp test: namely,
[[noreturn]] void f() { std::unreachable(); }should not trigger Clang's diagnostic warning: function declared 'noreturn' should not return [-Winvalid-noreturn]. If it does, that's a bug.
Yeah, I think that's the best we can do. At least we'll test that unreachable() is marked as [[noreturn]].
libcxx/include/charconv | ||
---|---|---|
91 | I wouldn't play with the header includes in this PR. We have to go through all headers anyways and remove unused includes. | |
libcxx/include/fstream | ||
188 | Same as above. | |
libcxx/src/filesystem/filesystem_common.h | ||
13 | Would you also be OK with me making a followup PR to clean up all the includes in src/? |
LGTM except for the missing .verify.cpp test. (And the accidental sed casualty __libcpp_unreachable_sentinel, which you're already aware of and fixing.)
Once the .verify.cpp test exists, I'd like @ldionne to approve too, since he had requested changes.
libcxx/src/filesystem/filesystem_common.h | ||
---|---|---|
13 | This is now D119561 | |
libcxx/src/filesystem/operations.cpp | ||
9 | In filesystem_common.h you included the top-level <utility>; here and in locale.cpp you included only the detail header. I'm ambivalent whether we should consistently stick with top-level headers or consistently stick with the most granular detail headers; and in the process of making D119561 I discovered that we've already got a mix (although I think the granular ones are all due to @Mordante in <format>-related code). So, I'm ambivalent what to do here. No action required, I guess. | |
libcxx/test/std/utilities/utility/utility.unreachable/unreachable.compile.pass.cpp | ||
9 |
Please add a .verify.cpp test along these lines. I'm not sure exactly how it should look; please just make sure that if you remove [[noreturn]] from __libcpp_unreachable, and re-ninja cxx, and re-run the test, that the test correctly fails. |
LGTM except for my comment about using a proper .verify.cpp test, which I'd like to see addressed.
libcxx/test/std/utilities/utility/utility.unreachable/unreachable.compile.pass.cpp | ||
---|---|---|
17 | I would like this to be a .verify.cpp test instead, with expected-no-diagnostics. You can look at e.g. libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/default.lazy.verify.cpp for how we do that. Otherwise, we're relying on the fact that warnings are turned into errors in the test suite, and I find that to be slightly brittle. |
libcxx/test/std/utilities/utility/utility.unreachable/unreachable.compile.pass.cpp | ||
---|---|---|
17 | Instead of expected-no-diagnostics we can let clang issue a diagnostic and test for that. |
libcxx/test/std/utilities/utility/utility.unreachable/unreachable.compile.pass.cpp | ||
---|---|---|
17 | Either way SGTM. |
Please update utils/generate_feature_test_macro_components.py to provide the __cpp_lib_unreachable feature-test macro.