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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

LGTM as-is with nitpicks comments about formatting. I suggest we tackle visibility as a global commit.



================
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;
----------------
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 :-)



================
Comment at: libcxx/test/std/input.output/filesystems/fs.filesystem.synopsis/enable_borrowed_range.compile.pass.cpp:14-17
+//template <>
+//inline constexpr bool ranges::enable_borrowed_range<filesystem::directory_iterator> = true;
+//template <>
+//inline constexpr bool ranges::enable_borrowed_range<filesystem::recursive_directory_iterator> = true;
----------------
Here and elsewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111644