[libc++][NFC] Inline the definition of ~basic_ios()

[ayzhao]

Summary

This is causing ODR violations in Chrome that is preventing it from
being built with libc++ debug symbols and ThinLTO[0].

[0]: https://crbug.com/1327706#c13

[ayzhao]

libcxx/include/ios

696–697

I'm not sure if I should specify _LIBCPP_INLINE_VISIBILITY here - does anyone have any ideas?

[jloser]

libcxx/include/ios

696–697

Should we use inline _LIBCPP_INLINE_VISIBILITY instead like the neighboring code currently does?

[jloser]

libcxx/include/ios

696–697

So, _LIBCPP_INLINE_VISIBILITY is a historical predecessor to _LIBCPP_HIDE_FROM_ABI. In newer code, we've been updating to _LIBCPP_HIDE_FROM_ABI.

With that being said, we definitely want inline there and also either _LIBCPP_HIDE_FROM_ABI or _LIBCPP_INLINE_VISIBILITY. There's an argument to be made for both. Using _LIBCPP_HIDE_FROM_ABI is the "new de-facto standard", whereas the latter is locally consistent in this neighborhood of code. I don't feel strongly.

It's a bit surprising this destructor wasn't hidden from being an exported symbol in the ABI!

[ayzhao]

add _LIBCPP_INLINE_VISIBILITY

[philnik]

libcxx/include/ios

696–697

I think nobody would have a problem if we just remove the out-of-line definition and change line 642 to _LIBCPP_HIDE_FROM_ABI virtual ~basic_ios() = default;. I think it should be fine to make it = default, because all copy- and move constructors are deleted.

[ayzhao]

default destructor

[philnik]

LGTM assuming CI passes, but please wait for a second #libc reviewer before landing the patch.

[ldionne]

In D126738#3550536, @philnik wrote:

LGTM assuming CI passes, but please wait for a second #libc reviewer before landing the patch.

I'm pretty sure CI is going to fail during check-cxx-abilist.

As-is, this patch is an ABI break for any user of iostream because it removes std::__1::basic_ios<wchar_t, std::__1::char_traits<wchar_t> >::~basic_ios() and std::__1::basic_ios<char, std::__1::char_traits<char> >::~basic_ios() from the dylib. Furthermore, I believe there was probably an intent of this destructor acting as a key function so that the vtable and RTTI of basic_ios<char|wchar_t> ends up in the dylib, and in the dylib only. I'm not 100% sure about that, but that's the most likely explanation for this strange definition in the first place.

After reading https://crbug.com/1327706#c13, it seems like the issue comes up when enabling libc++'s debug mode, right? If so, then I think this must be tackled via D122941 or something like it, instead.

[ldionne]

(requesting changes until I have more info)

[EricWF]

This comment was removed by EricWF.

[philnik]

In D126738#3556372, @EricWF wrote:

In D126738#3550837, @ldionne wrote:

(requesting changes until I have more info)

I would also like to ensure that we're not inlining the key function. It seems less than ideal to emit the vtable inline in every TU.

Is there a reason -Wweak-vtables isn't enabled?

[ayzhao]

In D126738#3550831, @ldionne wrote:

In D126738#3550536, @philnik wrote:

LGTM assuming CI passes, but please wait for a second #libc reviewer before landing the patch.

I'm pretty sure CI is going to fail during check-cxx-abilist.

As-is, this patch is an ABI break for any user of iostream because it removes std::__1::basic_ios<wchar_t, std::__1::char_traits<wchar_t> >::~basic_ios() and std::__1::basic_ios<char, std::__1::char_traits<char> >::~basic_ios() from the dylib. Furthermore, I believe there was probably an intent of this destructor acting as a key function so that the vtable and RTTI of basic_ios<char|wchar_t> ends up in the dylib, and in the dylib only. I'm not 100% sure about that, but that's the most likely explanation for this strange definition in the first place.

After reading https://crbug.com/1327706#c13, it seems like the issue comes up when enabling libc++'s debug mode, right? If so, then I think this must be tackled via D122941 or something like it, instead.

I spoke with cjdb@ offline and it seems that we can maintain ABI compatibility by defining an empty destructor (i.e. ~basic_ios() {}) instead of using = default: https://godbolt.org/z/r73v1oE6n. WDYT?

[philnik]

I spoke with cjdb@ offline and it seems that we can maintain ABI compatibility by defining an empty destructor (i.e. ~basic_ios() {}) instead of using = default: https://godbolt.org/z/r73v1oE6n. WDYT?

The problem isn't the triviality of basic_ios. The problem is that there isn't a key function anymore, so the vtable and destructor aren't emitted in the dylib anymore.

[ayzhao]

In D126738#3557017, @philnik wrote:

I spoke with cjdb@ offline and it seems that we can maintain ABI compatibility by defining an empty destructor (i.e. ~basic_ios() {}) instead of using = default: https://godbolt.org/z/r73v1oE6n. WDYT?

The problem isn't the triviality of basic_ios. The problem is that there isn't a key function anymore, so the vtable and destructor aren't emitted in the dylib anymore.

Apologies for the dumb question, but could we resolve the issue by moving the definition of the destructor to ios.cpp? The destructor would still be a key function, and this should resolve the ODR issue.

[ayzhao]

In D126738#3560693, @ayzhao wrote:

In D126738#3557017, @philnik wrote:

I spoke with cjdb@ offline and it seems that we can maintain ABI compatibility by defining an empty destructor (i.e. ~basic_ios() {}) instead of using = default: https://godbolt.org/z/r73v1oE6n. WDYT?

The problem isn't the triviality of basic_ios. The problem is that there isn't a key function anymore, so the vtable and destructor aren't emitted in the dylib anymore.

Apologies for the dumb question, but could we resolve the issue by moving the definition of the destructor to ios.cpp? The destructor would still be a key function, and this should resolve the ODR issue.

NVM, I just realized that this isn't possible because of the template parameters.

[philnik]

@ayzhao I think using virtual ~basic_ios() {} should work. I think I'm missing something in how [[gnu::visibility("hidden")]], [[clang::exclude_from_explicit_instantiation]] and = default interact. Sorry for the confusion.

[ayzhao]

Try an empty destructor instead of = default

[ayzhao]

In D126738#3564657, @philnik wrote:

@ayzhao I think using virtual ~basic_ios() {} should work. I think I'm missing something in how [[gnu::visibility("hidden")]], [[clang::exclude_from_explicit_instantiation]] and = default interact. Sorry for the confusion.

Done. Let's see what happens with the CI

[ldionne]

I just landed D122941 -- I think you should check whether this is still an issue for you.

[ayzhao]

In D126738#3564693, @ldionne wrote:

I just landed D122941 -- I think you should check whether this is still an issue for you.

So far, your patch does seem to work, but I'll need to wait to see if our bots are green when we roll libc++. Thanks!

[ldionne]

In D126738#3564733, @ayzhao wrote:

In D126738#3564693, @ldionne wrote:

I just landed D122941 -- I think you should check whether this is still an issue for you.

So far, your patch does seem to work, but I'll need to wait to see if our bots are green when we roll libc++. Thanks!

Any news? If it works, let's abandon this patch to clear the review queue.

[ayzhao]

In D126738#3599952, @ldionne wrote:

In D126738#3564733, @ayzhao wrote:

In D126738#3564693, @ldionne wrote:

I just landed D122941 -- I think you should check whether this is still an issue for you.

So far, your patch does seem to work, but I'll need to wait to see if our bots are green when we roll libc++. Thanks!

Any news? If it works, let's abandon this patch to clear the review queue.

I'll abandon this patch.

libcxx/include/ios

#endif
     _LIBCPP_INLINE_VISIBILITY bool bad() const  {return ios_base::bad();}
 
     _LIBCPP_INLINE_VISIBILITY iostate exceptions() const {return ios_base::exceptions();}
     _LIBCPP_INLINE_VISIBILITY void exceptions(iostate __iostate) {ios_base::exceptions(__iostate);}
 
     // 27.5.4.1 Constructor/destructor:
     _LIBCPP_INLINE_VISIBILITY
     explicit basic_ios(basic_streambuf<char_type,traits_type>* __sb);
-    virtual ~basic_ios();
+    _LIBCPP_INLINE_VISIBILITY virtual ~basic_ios() {};
 
     // 27.5.4.2 Members:
     _LIBCPP_INLINE_VISIBILITY
     basic_ostream<char_type, traits_type>* tie() const;
     _LIBCPP_INLINE_VISIBILITY
     basic_ostream<char_type, traits_type>* tie(basic_ostream<char_type, traits_type>* __tiestr);
 
     _LIBCPP_INLINE_VISIBILITY

 };
 
 template <class _CharT, class _Traits>
 inline _LIBCPP_INLINE_VISIBILITY
 basic_ios<_CharT, _Traits>::basic_ios(basic_streambuf<char_type,traits_type>* __sb)
 {
     init(__sb);
 }
 
 template <class _CharT, class _Traits>
-basic_ios<_CharT, _Traits>::~basic_ios()
-{
-}
-
-template <class _CharT, class _Traits>
 inline _LIBCPP_INLINE_VISIBILITY
 void
 basic_ios<_CharT, _Traits>::init(basic_streambuf<char_type, traits_type>* __sb)
 {
     ios_base::init(__sb);
     __tie_ = nullptr;
     __fill_ = traits_type::eof();
 }