This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement P0627R6 (Function to mark unreachable code)
ClosedPublic

Authored by philnik on Feb 7 2022, 9:24 AM.

Diff Detail

Event Timeline

philnik created this revision.Feb 7 2022, 9:24 AM
philnik requested review of this revision.Feb 7 2022, 9:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2022, 9:24 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Feb 7 2022, 1:32 PM
Quuxplusone added a subscriber: mstorsjo.
Quuxplusone added inline comments.
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.

This revision now requires changes to proceed.Feb 7 2022, 1:32 PM
mstorsjo added inline comments.Feb 7 2022, 1:49 PM
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).

philnik updated this revision to Diff 406655.Feb 7 2022, 5:11 PM
  • Introduce __unreachable() for previous standard versions and remove _LIBCPP_UNREACHABLE()
ldionne requested changes to this revision.Feb 8 2022, 2:21 PM

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.

This revision now requires changes to proceed.Feb 8 2022, 2:21 PM
philnik marked 3 inline comments as done.Feb 8 2022, 3:06 PM

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

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

philnik updated this revision to Diff 407272.Feb 9 2022, 1:24 PM
  • Address comments
Quuxplusone added inline comments.Feb 9 2022, 2:00 PM
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.
Otherwise, I have no objection to this approach. Looking in <cstdlib>, it turns out that it was already impossible for users to -D_LIBCPP_UNREACHABLE=anything-else, so we're not even breaking any unsupported-but-working thing any user might be doing. And the call sites don't need std:: because it takes no arguments, so ADL isn't a problem.

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?

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

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.

How do you expect the libc++-specific test to look? [...] https://godbolt.org/z/GvbnG76nW

// 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 [...]

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.

philnik added a comment.EditedFeb 9 2022, 2:19 PM

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

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

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.

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

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

philnik marked 3 inline comments as done.Feb 10 2022, 11:08 AM
philnik added inline comments.
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/?

philnik updated this revision to Diff 407611.Feb 10 2022, 11:08 AM
philnik marked 2 inline comments as done.
  • Fix CI

Thanks for working on this!

libcxx/include/__utility/unreachable.h
11

Please update utils/generate_feature_test_macro_components.py to provide the __cpp_lib_unreachable feature-test macro.

13

std::abort requires #include <cstdlib>.

philnik updated this revision to Diff 407949.Feb 11 2022, 10:41 AM
philnik marked 3 inline comments as done.
  • Address comments
philnik updated this revision to Diff 408054.Feb 11 2022, 2:24 PM
  • rename to __libcpp_unreachable
  • Fix modules
philnik updated this revision to Diff 408059.Feb 11 2022, 2:40 PM
  • Add test
  • Don't rename __unreachable_sentinel
Quuxplusone added 1 blocking reviewer(s): ldionne.

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

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.

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.

ldionne accepted this revision.Feb 14 2022, 8:24 AM

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.

This revision is now accepted and ready to land.Feb 14 2022, 8:24 AM
Mordante added inline comments.Feb 14 2022, 9:25 AM
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.
Adding something like https://godbolt.org/z/fvzoonv5v will result in a diagnostic.
ADDITIONAL_COMPILE_FLAGS: -Wunreachable-code will add the required compiler flag to the test.

philnik updated this revision to Diff 408454.Feb 14 2022, 9:26 AM
philnik marked 2 inline comments as done.
  • Address comments
ldionne accepted this revision.Feb 14 2022, 9:59 AM
ldionne added inline comments.
libcxx/test/std/utilities/utility/utility.unreachable/unreachable.compile.pass.cpp
17

Either way SGTM.

This revision was landed with ongoing or failed builds.Feb 14 2022, 11:53 AM
This revision was automatically updated to reflect the committed changes.