[libcxx-commits] [PATCH] D153709: [libc++] Implement stringbuf members of P0408R7 (Efficient Access to basic_stringbuf's Buffer)

[Hans Wennborg via Phabricator via libcxx-commits]

hans added a subscriber: ldionne.
hans added inline comments.


================
Comment at: libcxx/include/sstream:337
+#else
+    _LIBCPP_HIDE_FROM_ABI string_type str() const & { return str(__str_.get_allocator()); }
+
----------------
pfusik wrote:
> hans wrote:
> > pfusik wrote:
> > > hans wrote:
> > > > This is causing link errors for us on Windows. (https://crbug.com/1463881)
> > > > 
> > > > The problem is that `sstream` has an explicit instantiation declaration of `basic_stringbuf<char>` in the `sstream` header, but when compiling the instantiation in `ios.instantiations.cpp`, `_LIBCPP_BUILDING_LIBRARY` is defined and so this `string_type str() const &` member function doesn't get defined.
> > > > 
> > > > (Apologies for the late report; we were blocked on https://reviews.llvm.org/D151953#4472195 until yesterday.)
> > > `str() const &` is `_LIBCPP_HIDE_FROM_ABI` so I think it should NOT be linked.
> > > Why is it `__declspec(dllimport)` ?
> > The explicit instantiation decl uses `_LIBCPP_EXTERN_TEMPLATE_TYPE_VIS` which is dllimport: https://github.com/llvm/llvm-project/blob/main/libcxx/include/__config#L564
> > 
> > I think the idea is that _LIBCPP_HIDE_FROM_ABI should exclude the method from the explicit instantiation but that attribute doesn't seem to be really working with dllimport: https://godbolt.org/z/qc3zfvdaa
> Is this issue specific to the `str() const &` overload?
> How do the other `_LIBCPP_HIDE_FROM_ABI` members work? Are they `dllimport` too?
Yes, we're only seeing this issue for `str() const &` (I suppose the `const &&` overload has the same problem, but we're not calling it.)

I believe the other members are also `dllimport`, so in effect the `_LIBCPP_HIDE_FROM_ABI` isn't working, but it still builds because the members (at least the ones that get called in our builds) do get defined in the dll.

What makes these specific methods problematic is that they're excluded when `_LIBCPP_BUILDING_LIBRARY` is set. I suppose that's technically an ODR violation, but I can see why it's needed for compatibility.

I suppose we can try to make exclude_from_explicit_instantiation work better with dllimport explicit instantiations, but that means the code would only work with versions of Clang which has that fix.

We could also stop using exclude_from_explicit_instantiation on Windows and go back to _LIBCPP_ALWAYS_INLINE. I guess that was more brittle which is why the attribute was added in the first place though. 

Maybe @ldionne has ideas here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153709