Page MenuHomePhabricator

[libc++] Make _VSTD and alias for std
ClosedPublic

Authored by philnik on Jan 20 2022, 10:47 AM.

Details

Summary

There is no practical difference between _VSTD and std so we should just remove _VSTD. This is the first step.

Diff Detail

Event Timeline

philnik created this revision.Jan 20 2022, 10:47 AM
philnik requested review of this revision.Jan 20 2022, 10:47 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
dim added a subscriber: dim.Jan 20 2022, 11:18 AM

How do you mean "no practical difference"? _VSTD is defined as std::_LIBCPP_ABI_NAMESPACE, and in turn, _LIBCPP_ABI_NAMESPACE is configurable by the user, or defaults to mangling the ABI version in it. So this would be a huge change, for unknown benefit?

How do you mean "no practical difference"? _VSTD is defined as std::_LIBCPP_ABI_NAMESPACE, and in turn, _LIBCPP_ABI_NAMESPACE is configurable by the user, or defaults to mangling the ABI version in it. So this would be a huge change, for unknown benefit?

This patch isn't changing in which namespace the symbols are. Everything is still in std::_LIBCPP_ABI_NAMESPACE. _VSTD is only used when accessing anything in std::_LIBCPP_ABI_NAMESPACE and since _LIBCPP_ABI_NAMESPACE is always an inline namespace all the symbols are also accessible in std.

How can there be no practical difference if one is a configurable 'macro' and other isn't?

Hello Eric, everyone else,

We have projects that are getting great mileage from this macro right now, we'd be sad to see it go. 😰

If you look at https://github.com/ogiroux/freestanding you'll get the gist of what we're doing with it: we don't just change the inline namespace name, we also change the std:: namespace name using the macro. This was never the intended use, I realize, but it allows us to make progress with e.g. side-by-side co-existence for a large & important subset. In other words, the presence of this macro helped make libc++ adaptable the way needed it to be, and may slowly transform us into contributors even (hence netting you great mileage too).

So our view is that we would not like to see this change made. Sorry.

Olivier

How can there be no practical difference if one is a configurable 'macro' and other isn't?

Hello Eric, everyone else,

We have projects that are getting great mileage from this macro right now, we'd be sad to see it go. 😰

If you look at https://github.com/ogiroux/freestanding you'll get the gist of what we're doing with it: we don't just change the inline namespace name, we also change the std:: namespace name using the macro. This was never the intended use, I realize, but it allows us to make progress with e.g. side-by-side co-existence for a large & important subset. In other words, the presence of this macro helped make libc++ adaptable the way needed it to be, and may slowly transform us into contributors even (hence netting you great mileage too).

So our view is that we would not like to see this change made. Sorry.

Olivier

_VSTD was never intended to be changed other than by changing _LIBCPP_ABI_NAMESPACE. The question I'm basically asking here is if there are projects that rely on the macro that don't seem to be abandoned and if there is a way around it for these projects. For example I think in the project linked it should be possible to change _LIBCPP_BEGIN_NAMESPACE_STD and using std in the headers should still work (https://godbolt.org/z/c7TTb1f9Y). Note that changing _LIBCPP_BEGIN_NAMESPACE_STD is just as unsupported as changing _VSTD.

@philnik, I suggest opening a discussion thread on the Discord if you still want to pursue this direction at all.
D117811 duplicates D55517 (December 2018), which was abandoned, and I don't think we have any "new information" since then.
_VSTD and std currently are not synonymous — there are places you can grep for where we deliberately use std:: not _VSTD:: — and this seems like it would erase that deliberate difference. Whether the deliberate difference is useful, I don't know. But at the very least, you could propose a preliminary PR that removes just the difference (i.e. change just those few existing places to use _VSTD:: instead of std::), and let someone explain why even that would be a bad idea. It could be a learning experience for all of us. :)

@philnik, I suggest opening a discussion thread on the Discord if you still want to pursue this direction at all.
D117811 duplicates D55517 (December 2018), which was abandoned, and I don't think we have any "new information" since then.
_VSTD and std currently are not synonymous — there are places you can grep for where we deliberately use std:: not _VSTD:: — and this seems like it would erase that deliberate difference. Whether the deliberate difference is useful, I don't know. But at the very least, you could propose a preliminary PR that removes just the difference (i.e. change just those few existing places to use _VSTD:: instead of std::), and let someone explain why even that would be a bad idea. It could be a learning experience for all of us. :)

I am aware that this is a duplicate of D55517. I chose to create a new PR, since D55517 hasn't been updated in a few years. I agree that we don't have any new information, but I don't see any information in D55517 that would stop us from removing _VSTD.

ldionne accepted this revision as: ldionne.Jan 21 2022, 9:31 AM
ldionne added a reviewer: Restricted Project.

I think there is some misunderstanding around _VSTD::, _LIBCPP_ABI_NAMESPACE, and the notion that libc++ uses an unversioned namespace on purpose in a few places (notably libc++abi). Let me try to clear this up. First:

  • _VSTD:: expands to std::<abi-namespace>, and can't be configured.
  • _LIBCPP_ABI_NAMESPACE expands to <abi-namespace>, and is configurable.

However, because of inline namespaces, typing std::<abi-namespace>::foo and std::foo is strictly equivalent. Therefore, _VSTD::foo and std::foo are strictly equivalent for anything that was defined inside <abi-namespace>. There are some places, e.g. libc++abi, where we define types and functions inside namespace std { } without using <abi-namespace>. As far as I can tell, that was to be compatible with libstdc++ exceptions when we made the migration from libstdc++ to libc++ on Apple platforms ~10 years ago. Because these types and functions are not defined in std::<abi-namespace>, _VSTD cannot be used for qualifying them, because that would try to access them inside the <abi-namespace>. However, the other way around does work, i.e. using std:: in place of _VSTD::.

In particular, this patch does not propose removing the ability to customize the ABI namespace by changing _LIBCPP_ABI_NAMESPACE. That would still be supported, and if you customize <abi-namespace>, we'd still be using the correct namespace when doing std::foo, whether foo is a versioned symbol or not.

My main rationale for being in favour of this change is that we don't even use _VSTD:: consistently, and so any claims to be using _VSTD:: in fancy ways is guaranteed not to work without changes (and I suspect pretty major ones). In other words, I think we're cargo-culting the utility of this macro, and I would like to see it go away. I'm adding the libcxx vendors group to gather attention. If you can bring new information to the table (e.g. if you can show us reasonable uses of _VSTD that I am not aware of), then we can totally have this discussion, and we could even perhaps support that use case properly. However, I would really like to avoid making progress on this just based on fear and uncertainty.

However, my main concern is about breaking in-flight changes and downstream diffs by making such a wide ranging patch (I would have the same concern for e.g. clang-format on the whole code base at once). Instead, I would like that as a first step, we redefine _VSTD:: to being just std::, and start using std:: instead of _VSTD:: in new code we write. We could then also move from _VSTD:: to std:: in patches that e.g. modularize files or otherwise touch a lot of lines anyways. I'm accepting personally to show support for this. Let's give folks the opportunity to show concrete evidence of _VSTD needing to stay, and if that doesn't happen, we'll move forward.

Just to clarify, the step 1 I'm requesting instead of this PR is to just change this:

#define _VSTD std

I've mentioned this before, but NVIDIA defines _VSTD to something else in our product branch. This diff, here, implies that we will need a matching & opposite diff to further separate us from upstream.

I'm not necessarily saying you have to do something convenient for NVIDIA, but know how it's not ideal.

I've mentioned this before, but NVIDIA defines _VSTD to something else in our product branch. This diff, here, implies that we will need a matching & opposite diff to further separate us from upstream.

I'm not necessarily saying you have to do something convenient for NVIDIA, but know how it's not ideal.

I would like to understand what your use case is so we can either support it properly, or not at all. The current situation where we're cargo culting the usefulness of _VSTD based on some vendors saying "don't make this change, it'll break us" is not good. I would like a concrete and actionable description of how you are using it so we can either decide "okay, that is a supported use case of libc++ and we're going to commit to supporting that properly", or to the contrary "this is twisted and we don't commit to supporting that".

EricWF added a subscriber: EricWF.Jan 21 2022, 11:24 AM

I fully support this change. I've wanted to kill _VSTD for a while. This will also reduce the memory usage of clang when compiling libc++ because it no longer needs to expand the macro and build nodes for the additional inline namespace specifier. Not sure how much this is worth, but it's something.

philnik updated this revision to Diff 402053.Jan 21 2022, 11:26 AM

Only change _VSTD

The build failure looks to me like a gcc bug. Can anybody confirm this? Or is it implementation divergence?

dim added a comment.Jan 21 2022, 3:43 PM

The build failure looks to me like a gcc bug. Can anybody confirm this? Or is it implementation divergence?

As far as I can see, it checked whether clang-format made any corrections, and it apparently did:

+ git-clang-format --binary /usr/bin/clang-format --diff --extensions ,h,hh,hpp,hxx,c,cc,cxx,cpp HEAD~1 -- libcxx/benchmarks libcxx/include libcxx/src libcxx/test libcxxabi/fuzz libcxxabi/include libcxxabi/src libcxxabi/test

+ tee /home/libcxx-builder/.buildkite-agent/builds/c70c66b3b3e9-1/llvm-project/libcxx-ci/build/check-format/clang-format.patch
...
+ grep -q '^--- a' /home/libcxx-builder/.buildkite-agent/builds/c70c66b3b3e9-1/llvm-project/libcxx-ci/build/check-format/clang-format.patch
🚨 Error: The command exited with status 1

E.g., the clang-format produced diff was non-empty? It seems that it wanted to indent some of the preprocessor statements...

@dim clang-format is currently not enforced. I mean the gcc build failure.

wmaxey added a subscriber: wmaxey.Jan 21 2022, 5:20 PM

As @__simt__ has stated, we do make use of this macro, but are open to the idea of an alternative that does provide similar configurability.

This macro is used to differentiate between two namespaces depending on how the project is included/used. It would be possible for us to move away from some such a mechanism, but ideally rather than deleting it, I would like to see it be replaced with something that allows us to assume the namespace we need to search isn't necessarily std::

As @__simt__ has stated, we do make use of this macro, but are open to the idea of an alternative that does provide similar configurability.

This macro is used to differentiate between two namespaces depending on how the project is included/used. It would be possible for us to move away from some such a mechanism, but ideally rather than deleting it, I would like to see it be replaced with something that allows us to assume the namespace we need to search isn't necessarily std::

We know that people change it, but we don't know how it is used. Could you provide a specific example that uses _VSTD? If we don't know how _VSTD is used we can't determine how to officially support these use cases or see if it is necessary for these use-cases to even have _VSTD. Would it be possible to just change _LIBCPP_BEGIN_NAMESPACE_STD? I don't think we'll remove that. It would probably also be possible to make that configurable by the user more easily.

Mordante added inline comments.Jan 23 2022, 9:19 AM
libcxx/include/__config
822

Please add some information regarding this change to the release notes.

philnik updated this revision to Diff 404283.Jan 29 2022, 9:15 AM
  • Add gcc workarounds
  • Add release note
philnik marked an inline comment as done.Jan 29 2022, 9:15 AM

Gentle ping @__simt__ @wmaxey. I genuinely want to understand whether/how we can support your use case properly and avoid creating needless pain for you folks, but I also want us to stop pretending that this macro has any non-accidental use in its current form. I'll approve and ask to ship this on Feb 3rd if there is no answer.

libcxx/docs/ReleaseNotes.rst
111
Mordante added inline comments.Feb 1 2022, 11:04 AM
libcxx/include/__format/format_arg.h
141

Why is this required? Is this the Add gcc workarounds? This deviates from the previous assertion that there was no difference between using std:: and std::__1::. The requirement to sometimes use std::_LIBCPP_ABI_NAMESPACE instead of std:: feels error prone to me.

philnik added inline comments.Feb 1 2022, 11:09 AM
libcxx/include/__format/format_arg.h
141

Yes, these are the GCC workarounds. I think this is a GCC bug, but nobody has confirmed it yet, so I didn't open an issue. It seems GCC doesn't handle friends properly with inline namespaces.

Quuxplusone requested changes to this revision.Feb 1 2022, 11:27 AM
Quuxplusone added inline comments.
libcxx/include/__format/format_arg.h
141

I think it should work to just remove _VSTD:: from this line, because make_format_args is already in the same namespace as basic_format_arg, isn't it? (And ADL applies to calls, not friend declarations.) Thus:

template <class _Ctx, class... _Args>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_AVAILABILITY_FORMAT
friend __format_arg_store<_Ctx, _Args...> make_format_args(const _Args&...);

(Drive-by fixed indentation.) If this way works, which I believe it should, I'd very much prefer to do it (unless @Mordante is planning some future change that will explain why we don't want to do this even though it works).

Ditto below.

Also, if this works, please land this should-be-NFC change (and below) in their own separate commit. Let's have changing-the-#define _VSTD be a one-line commit, so it's really really trivial to revert if needed.

This revision now requires changes to proceed.Feb 1 2022, 11:27 AM
Mordante requested changes to this revision.Feb 1 2022, 11:30 AM

Request changes to figure out which compiler is wrong.

libcxx/include/__format/format_arg.h
141

Please make sure it's a GCC bug and not a Clang bug, feel free to discuss it on Discord.
I'm not happy when it's a GCC bug and we need to add _LIBCPP_ABI_NAMESPACE. This is error prone and only caught by the CI. (At least I expect most of us using Clang as compiler.)

Mordante added inline comments.Feb 1 2022, 11:32 AM
libcxx/include/__format/format_arg.h
141

It seems our messages crossed @Quuxplusone, I'll have a look at removing the _VSTD from the friend declarations.

Mordante added inline comments.Feb 1 2022, 12:01 PM
libcxx/include/__format/format_arg.h
141

It works for me locally so created D118719. In that case we don't need to verify which compiler is right.

philnik added inline comments.Feb 1 2022, 12:06 PM
libcxx/include/__format/format_arg.h
141

Based on http://eel.is/c++draft/basic.namespace#namespace.def.general-6 I would say that this is a GCC bug. Name lookup is the same even if "there are declarations of that name in the enclosing namespace". I don't know if name lookup is done with friend declarations, but I guess it must be.

philnik retitled this revision from [libc++] Remove _VSTD to [libc++] Make _VSTD and alias for std.Feb 1 2022, 1:36 PM
philnik edited the summary of this revision. (Show Details)

It turns out that the release is being cut today instead of Feb 4th, like I thought. @philnik please update this and ping me, we'll ship it today.

If some vendors want to weigh in against this change, now's the time. Once this is shipped and we've cut LLVM 14, we'll start removing instances of _VSTD:: in new code and in code that we're touching anyway.

philnik updated this revision to Diff 405087.Feb 1 2022, 1:40 PM
philnik marked an inline comment as done.
  • Update release note
ldionne accepted this revision as: Restricted Project.Feb 1 2022, 1:41 PM

Ship it!

This revision was not accepted when it landed; it landed in state Needs Review.Feb 1 2022, 1:43 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
wmaxey added a comment.Feb 1 2022, 3:14 PM

I appreciate getting some time to evaluate the impact. After some investigation we have no issue with this change.

Gentle ping @__simt__ @wmaxey. I genuinely want to understand whether/how we can support your use case properly and avoid creating needless pain for you folks, but I also want us to stop pretending that this macro has any non-accidental use in its current form. I'll approve and ask to ship this on Feb 3rd if there is no answer.