Page MenuHomePhabricator

[Libcxx] Add <source_location> header.
AcceptedPublic

Authored by jyknight on Feb 27 2022, 11:03 AM.

Details

Reviewers
ldionne
philnik
Group Reviewers
Restricted Project
Summary

This requires the __builtin_source_location() builtin, as implemented
by GCC and Clang.

Diff Detail

Event Timeline

jyknight requested review of this revision.Feb 27 2022, 11:03 AM
jyknight created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2022, 11:03 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

FWIW, you can maybe check if there is any test (or anything else) to take from https://reviews.llvm.org/D80376 (my failed attempt to implement this, but without clang's built-in).

philnik requested changes to this revision.Feb 27 2022, 11:42 AM
philnik added a subscriber: philnik.

Looks like you have to run ninja libcxx-generate-files.

libcxx/include/source_location
41
44–45
64–75
78
libcxx/test/std/language.support/support.srcloc/general.pass.cpp
17–20

This should be something along the lines of // UNSUPPORTED: clang-13, depending on when __builtin_source_location was implemented.

This revision now requires changes to proceed.Feb 27 2022, 11:42 AM

You also have to add the header to libcxx/include/modules.modulemap and libcxx/include/CMakeLists.txt.

libcxx/include/version
361–364

You should edit libcxx/utils/generate_feature_test_macro_components.py. This is a generated file.

+1 to all @philnik's comments. Looks pretty decent otherwise; let's see what buildkite thinks of it!

libcxx/test/std/language.support/support.srcloc/general.pass.cpp
12–15
44

By convention (something about freestanding support): int main(int, char**) {
and return 0; at the end. See any other test file for comparison.

Thanks for working on this! Some additional comments.

libcxx/include/source_location
12

Please update the source_location status in docs/Cxx2aStatusPaperStatus.csv. Look like this implements both both P1208 and LWG-3396.

29

This header needs to be included in all library header, it provides all important macros like _LIBCPP_BEGIN_NAMESPACE_STD.

56

The __ptr argument differs from the wording in the Standard, can you add comment why this is done?

libcxx/test/std/language.support/support.srcloc/general.pass.cpp
17–20

Since the clang part is under review it should be also unsupported in clang-13 and apple-clang-12.
All our supported platforms are tested in the precommit CI so it's possible to see which other platforms fail.

jyknight updated this revision to Diff 412090.Mar 1 2022, 7:03 AM
jyknight marked 12 inline comments as done.

Update per review comments.

Looks like you have to run ninja libcxx-generate-files.

Done.

You also have to add the header to libcxx/include/modules.modulemap and libcxx/include/CMakeLists.txt.

Done.

libcxx/include/source_location
31–32

I'd like a reviewer to opine on this question here.

libcxx/test/std/language.support/support.srcloc/general.pass.cpp
17–20

Well, it's not implemented until the patch this depends on. So it'll be implemented as of clang-15 -- but current builds of clang-15 don't have that support. Should I just put "// UNSUPPORTED: clang-13, clang-14"? Will that cause problems with test-bots? I'll submit the other patch first -- but do libcxx bots run with older builds of unreleased head clang? Guess I'll just try and see what happens...

philnik added inline comments.Mar 1 2022, 7:12 AM
libcxx/include/source_location
31–32

Including the header should be fine.

libcxx/test/std/language.support/support.srcloc/general.pass.cpp
17–20

CI doesn't run a trunk clang, so it should just be // UNSUPPORTED: clang-13 clang-14 and the others that don't work.

So, in doing further testing, I've noticed a problem. Currently, clang cannot compile code that calls a consteval function in a default argument. (see https://github.com/llvm/llvm-project/issues/48230 and https://reviews.llvm.org/D119646)

E.g., clang -std=c++20 -fsyntax-only consteval.cc given:

consteval int foo() { return 1; }
int bar(int N = foo()) { return N; }
int main() { return bar(); }

raises "error: cannot take address of consteval function 'foo' outside of an immediate invocation", incorrectly.

The same issue occurs with the consteval std::source_location::current() -- which makes it effectively impossible to use std::source_location::current() as it's intended to be used, in a default-argument.

Quuxplusone added inline comments.Mar 1 2022, 7:28 AM
libcxx/include/source_location
31–32

IMHO #include <cstdint> is fine (but please move it up with the others and alphabetize).
(If @ldionne disagrees, go with what he says.)

44–45

Nit: s/REQUIRED/required/ (the emphasis is unnecessary, and potentially briefly confusing in that REQUIRED has a special meaning in tests). I might reword this comment as

// The names source_location::__impl, _M_file_name, _M_function_name, _M_line, and _M_column
// are hard-coded in the compiler and must not be changed here.

(Note for users of git grep: These names are not hard-coded in Clang yet; they're part of D120159 which has not yet landed.)

58–59

My kneejerk reaction here was "this indentation is messed up," but then my eyes refocused. ;)
I suggest reflowing as

static consteval
source_location current(const void *__ptr = __builtin_source_location()) noexcept {

Using const void * instead of decltype(__builtin_source_location()) helps readability and doesn't seem to hurt anything since you're already doing the cast on line 61. In fact, at that point I'd remove the comment on line 60 because now the cast isn't unusual-looking at all.

(2) Should we use _LIBCPP_HIDE_FROM_ABI here, even though it's consteval? I don't know.

libcxx/utils/generate_feature_test_macro_components.py
636–642

Please git grep source_location ../libcxx/docs/ and see whether you can check off any relevant papers or issues. (Also of course git grep source_location ../libcxx in general.)

philnik added inline comments.Mar 1 2022, 8:21 AM
libcxx/include/source_location
58–59

(2) Should we use _LIBCPP_HIDE_FROM_ABI here, even though it's consteval? I don't know.

I don't think so. A consteval function can never be part of any ABI, so there is no need to mark it as _LIBCPP_HIDE_FOM_ABI.

EricWF added a subscriber: EricWF.Mar 1 2022, 9:32 AM
EricWF added inline comments.
libcxx/include/source_location
58–59

I don't think using const void* improves readability. Why do you say that?

@Quuxplusone Going forward the project should take a "format and forget about it" approach to whitespace changes. What's correct is what clang-format spits out. I don't believe reviewing whitespace changes is a good use of engineering effort.

65

When does this constructor get called?

I assume it's in the spec?

Mordante added inline comments.Mar 1 2022, 10:09 AM
libcxx/test/std/language.support/support.srcloc/general.pass.cpp
17–20

Well, it's not implemented until the patch this depends on. So it'll be implemented as of clang-15 -- but current builds of clang-15 don't have that support. Should I just put "// UNSUPPORTED: clang-13, clang-14"? Will that cause problems with test-bots? I'll submit the other patch first -- but do libcxx bots run with older builds of unreleased head clang? Guess I'll just try and see what happens...

The CI uses nightly Ubuntu builds, but the Docker file isn't automatically updated. This means:

  • we first need to land the clang patch
  • wait a short while before it's available in Ubuntu
  • ask @ldionne to update the Docker image
  • wait a while until the new image is used in the CI

This isn't ideal, but we've used this approach for other patches with dependencies on the CI running the latest version of our Docker file.

ldionne requested changes to this revision.Mar 1 2022, 10:29 AM

Does this mean we'll only implement source_location with Clang? Or does GCC provide a similar builtin? If so, are both compatible?

libcxx/include/source_location
31–32

Yeah, I'd rather use uint_least32_t and include the header than use the more arcane __UINT_LEAST32_TYPE__.

58–59

We've had numerous discussions about clang-format in the past several months. Please consult the Discord archive for details, but long story short, the current state of things is that we're going to first agree on a clang-format configuration that is suitable, and then slowly move towards it for new code. It hasn't happened yet.

Regarding decltype(__builtin_source_location()), you could also just use auto* instead.

libcxx/test/std/language.support/support.srcloc/general.pass.cpp
17–20

@Mordante is correct -- basically we'll need to wait a bit after the Clang patch lands for the Docker images to be refreshed and then all CI will be running with a Clang that supports the builtin. In the meantime, only the Bootstraping build bot would is relevant, since it's the only one that's going to run this test.

33

I'd like us to test the return type of all these functions. We usually do something like std::same_as<uint_least32_t> auto line = empty.line();.

This revision now requires changes to proceed.Mar 1 2022, 10:29 AM
EricWF added inline comments.Mar 1 2022, 10:47 AM
libcxx/include/source_location
58–59

Fantastic. Let's do that. Because reviewing whitespace is in nobodys best interest.
And if we can't agree on a clang-format configuration, then we can't agree on whitespace, and we shouldn't bog any review down changing it.

jyknight updated this revision to Diff 412279.Mar 1 2022, 3:25 PM
jyknight marked 14 inline comments as done.

Update for review comments.

Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 3:25 PM
jyknight added inline comments.Mar 1 2022, 3:25 PM
libcxx/include/source_location
58–59

Can't use auto* in the argument type -- that'd make this into a function template.

I don't want to use a const void * argument, because constexprs are not allowed to cast back _from_ void*, even if the underlying object is a T and you're casting to T*. (However, note that I did add a hack to Clang to support libstdc++ -- which DOES have that invalid cast that GCC doesn't diagnose, but I wouldn't want to introduce the same issue into libc++.)

I can move the decltype into a separate using __bsl_ty = decltype(__builtin_source_location()); line to improve readability.

65

Not sure what you mean. Yes, a default constructor is required by the spec.

libcxx/test/std/language.support/support.srcloc/general.pass.cpp
17–20

The GCC bot should be running the test already.

Are you sure the bootstrapping build is doing this? Because if so, it should've failed (as clang-15 isn't marked unsupported), but it didn't.

libcxx/utils/generate_feature_test_macro_components.py
636–642

Did that already in the preceding update.

Does this mean we'll only implement source_location with Clang? Or does GCC provide a similar builtin? If so, are both compatible?

No, GCC implements the same builtin with the same semantics, modulo the return type.

ldionne accepted this revision.Mar 2 2022, 6:18 AM

LGTM. I might make a minor pass on the test to make it look more like our other tests (in particular try to avoid duplication of constexpr and non-constexpr tests), but overall this LGTM.

Thanks a whole lot for working on this, we really needed someone to step up and implement this with the help of the compiler.

libcxx/test/std/language.support/support.srcloc/general.pass.cpp
17–20

Yes, I am sure the bootstrapping build is running with the just-built Clang. Is it possible that CI ran alongside with the parent commit too? I think that's how it works.

philnik accepted this revision.Mar 5 2022, 5:13 PM

LGTM % comments.

libcxx/include/source_location
67–72

I'd like to see __ptr_ != nullptr

libcxx/test/std/language.support/support.srcloc/general.pass.cpp
65

Could you == 0 or cast one to string_view for comparison? The current code looks a lot like they should not be equal. Ditto below.

This revision is now accepted and ready to land.Mar 5 2022, 5:13 PM
Quuxplusone added inline comments.Mar 5 2022, 5:37 PM
libcxx/test/std/language.support/support.srcloc/general.pass.cpp
66

Too bad we can't std::string_view(local.function_name()).contains("main") until C++23. :(

+1 for assert(strcmp(local.file_name(), "ss") == 0) above (and lines 72,73,79,80...) and for assert(strstr(local.function_name(), "main") != nullptr) here.

@jyknight is there a reason you don't work on this currently?

I mentioned on Chat, but I see I didn't mention here: I'm not sure there's much point in committing this to libc++ until https://github.com/llvm/llvm-project/issues/48230 has been fixed in Clang, so that it can be properly used.

Given https://github.com/llvm/llvm-project/issues/48230 is closed, maybe we could move forward on this direction.

@jyknight Do you plan to finish this PR? If not I'd be happy to complete it.