[libcxx-commits] [PATCH] D111644: [libc++] LWG3480: make (recursive)_directory_iterator C++20 ranges

Thu Oct 14 13:56:42 PDT 2021

ldionne added inline comments.

================
Comment at: libcxx/include/filesystem:3024
+#if !defined(_LIBCPP_HAS_NO_RANGES)
+template<>
+inline constexpr bool _VSTD::ranges::enable_borrowed_range<_VSTD_FS::directory_iterator> = true;
----------------
Quuxplusone wrote:
> ldionne wrote:
> > jloser wrote:
> > > jloser wrote:
> > > > Quuxplusone wrote:
> > > > > Nitpick: `template <>` for consistency. (FWIW, I personally write `template<>` consistently in my own code. But libc++ more often uses `template <>`; and more importantly, lines 3026, 3029, 3031 already use `template <>`.)
> > > > > 
> > > > > Also, do you need `_LIBCPP_TEMPLATE_VIS` throughout? I think so, but I'm not sure.
> > > > Fixed the spacing in `template <>` here, in the synopsis, and the tests.
> > > > 
> > > > As for the `_LIBCPP_TEMPLATE_VIS`, I don't know. I added it to the class, but can we get away with adding it just to these specializations? @ldionne 
> > > FWIW `span` doesn't have `_LIBCPP_TEMPLATE_VIS` on the similar variable template specializations for `span`, so I removed it for now unless @ldionne tells me I need it.
> > `_LIBCPP_TEMPLATE_VIS` makes the vague linkage objects of a type visible. Here we're talking about a variable so you definitely don't need it. In fact, we have a pre-existing issue that non of our variable templates are visibility-annotated, which leads to a bunch of weak symbols leaking from user programs when they use those variables. That was reported to me internally by our dynamic linker folks, who are intimately aware of these sorts of issues because the dynamic linker is the one having to do the job of de-duplicating the symbols.
> > 
> > Well, I guess in theory the more correct behavior is to give default visibility to those variable templates such that they have the same address if used in two different dylibs, however in practice I believe it's better to reduce the number of weak symbols since anyone relying on the address of these variables templates for their correctness is doing something really wrong. To illustrate the situation, consider this:
> > 
> > ```
> > cat <<EOF | clang++ -xc++ - -std=c++20 -fvisibility=hidden -shared -o liba.dylib && nm -omg liba.dylib | c++filt
> > #include <type_traits>
> > __attribute__((visibility("default")))
> > void const* a() { return &std::is_integral_v<int>; }
> > EOF
> > 
> > cat <<EOF | clang++ -xc++ - -std=c++20 -fvisibility=hidden -shared -o libb.dylib && nm -omg libb.dylib | c++filt
> > #include <type_traits>
> > __attribute__((visibility("default")))
> > void const* b() { return &std::is_integral_v<int>; }
> > EOF
> > 
> > cat <<EOF | clang++ -xc++ - -std=c++20 -L . -la -lb -o test && ./test
> > #include <cassert>
> > void const* a();
> > void const* b();
> > int main() { assert(a() == b()); }
> > EOF
> > ```
> > 
> > The above will fail because the two dylibs were compiled with `-fvisibility=hidden`. Since we don't annotate `std::is_integral_v` with any visibility annotation, it gets the visibility set by `-fvisibility=hidden`, so it is hidden. Both `liba.dylib` and `libb.dylib` get a definition of `std::is_integral_v` (that is the case regardless of `-fvisibility=hidden`). However, when we run the `test` program, the dynamic linker doesn't see that both `liba.dylib` and `libb.dylib` are using the same `std::is_integral_v`, because it has hidden visibility in both of them (technically only one of the two needs hidden visibility). As a result, both `a()` and `b()` return addresses to different objects and the assertion fails.
> > 
> > Now, if you go ahead and remove `-fvisibility=hidden` from the command lines above, suddenly `liba.dylib` and `libb.dylib` start exporting `std::is_integral_v` and the dynamic linker de-duplicates those at load time. And the assertion passes.
> > 
> > So, do we want this to work (knowing there is a load time and symbol table size impact)? In my opinion, the pragmatic thing is to say that it shouldn't work, in which case we'd go ahead and mark those inline variables as `_LIBCPP_HIDE_FROM_ABI`.
> > 
> > However, since we don't do that right now, I'd suggest you hold off on doing it and we can apply it throughout the library. Actually if someone wanted to do that, it would definitely solve one of my problems right now :-)
> > 
> If I understand your program correctly, then //yes//, we not only "want" it to work, but it's mandated by the Standard to work. When someone takes the address of `std::is_integral_v<int>` from multiple TUs, they must see the same address from both TUs, because the Standard says so (via the `inline` keyword).
> 
> Arguably, as soon as the user passes `-fvisibility=hidden -shared`, all bets are off: is that what we're claiming here?
> 
> Re `_LIBCPP_TEMPLATE_VIS`, ignore me; I think I was looking at something like `less<void>` when I said that. Which //is// a full specialization, but of a //type//, not a //variable//.
We're not talking about two different TUs here, we're talking about two different shared libraries. But that's orthogonal to the issue we're discussing because what should work across TUs should also work across dylib boundaries.

However, I think you made me change my mind here. I was going to quote http://eel.is/c++draft/constraints to say how users are not allowed to depend on the address of those variable templates, however it appears that we only prohibit that for functions in namespace `std`, not for variables. So indeed, it seems like users are allowed to expect that the above will work, and so throwing `_LIBCPP_HIDE_FROM_ABI` onto all those variable templates would make us non-conforming. Ugh, that's a nasty problem cause I don't think the intent was ever for the address of those variable templates to be relevant.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111644