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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/sstream:337
+#else
+    _LIBCPP_HIDE_FROM_ABI string_type str() const & { return str(__str_.get_allocator()); }
+
----------------
hans wrote:
> pfusik wrote:
> > ldionne wrote:
> > > hans wrote:
> > > > 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?
> > > This seems like https://github.com/llvm/llvm-project/issues/40363. I never had time to look into it, and until now it seemed like it wasn't hard-blocking anyone.
> > > 
> > > The medium term fix is to fix https://github.com/llvm/llvm-project/issues/40363 and be done with it.
> > > The short term fix could be this:
> > > 
> > > ```
> > > // TODO(LLVM 19): Remove this once we drop support for Clang 16, which had this bug: https://github.com/llvm/llvm-project/issues/40363
> > > #ifdef _WIN32
> > > _LIBCPP_ALWAYS_INLINE string_type str() const & { return str(__str_.get_allocator()); }
> > > #else
> > > _LIBCPP_HIDE_FROM_ABI string_type str() const & { return str(__str_.get_allocator()); }
> > > #endif
> > > ```
> > > 
> > > WDYT?
> > > 
> > > Would someone with Windows access be willing to take on the fix for Clang? I can review it, I'm just really under water until the release. I don't think it's too hard to fix, the attribute was really simple to implement. It would be nice to fix this for LLVM 17, then we can remove the workaround fairly soon.
> > I'm fine with both the short and medium term fixes and can do both.
> Both of these sound good to me too. I just confirmed the short-term one fixes our build, and I'm happy to help with the medium-term one too. (Thanks for finding the bug Louis, I had completely forgotten that we already had it on file.)
Linking this thread to D155713.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153709