[libc++] Inline stdexcept constructors, destructors, and assignment operators when using MSVC ABI

[thomasanderson]

Summary

MSVC doesn't provide definitions of functions like
std::logic_error::logic_error(const logic_error&), so they are implicitly generated by
the compiler as inline functions. The inline-ness in the libc++ implementations
need to match those of MSVC, otherwise we get an ODR violation leading to a
duplicate symbol error when trying to link.

[EricWF]

hanks for looping me in. I'm working on a similar problem but with libstdc++.

I think the approach of creating specific visibility macros for the exception / runtime bits than need to be shared makes sense,
though the additional complexity is unfortunate.

include/__config

1003

Can we choose a more descriptive name, starting with _LIBCPP_ABI_?

The macro should also be documented in docs/DesignDocs/VisibilityMacros.rst.

src/include/refstring.h

69

These changes seem wrong. And I don't understand why the changes are needed in relation to MSVC.

[thomasanderson]

In D58203#1397191, @EricWF wrote:

hanks for looping me in. I'm working on a similar problem but with libstdc++.

I think the approach of creating specific visibility macros for the exception / runtime bits than need to be shared makes sense,
though the additional complexity is unfortunate.

The visibility macro is only used in one place. maybe we could just do something like:

class
#if defined(_LIBCPP_ABI_INLINE_STDEXCEPT_DEFINITIONS)
_LIBCPP_TYPE_VIS
#else
_LIBCPP_HIDDEN
#endif
__libcpp_refstring

wdyt?

src/include/refstring.h

69

This is now needed eg. for the inline definition of logic_error::logic_error(), which must now construct a __libcpp_refstring.

If I leave "inline" in, I get link errors:

lld-link: error: undefined symbol: "public: __cdecl std::__1::__libcpp_refstring::__libcpp_refstring(char const *)" (??0__libcpp_refstring@__1@std@@QEAA@PEBD@Z)
>>> referenced by obj/buildtools/third_party/libc++/libc++/string.obj

include/refstring.h gets included in stdexcept.cpp. Actually, that's the only place it gets included, so maybe we could get rid of the inline altogether since we can't have multiple definitions?

[EricWF]

include/stdexcept

83

We should declare the same set of symbols regardless of _LIBCPP_ABI_INLINE_STDEXCEPT_DEFINITIONS.

And when we want inline definitions, instead of defining it inside the class, let's put an inline definition just below the class and guard that.

src/include/refstring.h

69

I don't understand that error at all.

The constructor it's saying is undefined is marked inline. If it is ODR-used in a translation unit, then MSVC needs to emit a definition for it in that TU.

Am I missing something?

[thomasanderson]

include/stdexcept

83

Done. A new visibility macro is required since we presumably want _LIBCPP_INLINE_VISIBILITY when providing the inline definitions and not otherwise. I tested that it works without _LIBCPP_INLINE_VISIBILITY, but I would think that we still want it.

src/include/refstring.h

69

I understand why this is necessary now. When the inline keyword is used, the compiler only generates code for that function if that function is actually used. Since I removed the definitions of runtime_error etc in stdexcept.cpp, nothing actually uses __libcpp_refstring so the code doesn't get generated.

[thakis]

This was made obsolete by D57425, right?

[thomasanderson]

In D58203#1403173, @thakis wrote:

This was made obsolete by D57425, right?

Rather, this CL makes that one obsolete

[thakis]

But D57425 landed and this one didn't :-) Is the plan to land this and revert the other one then? It sounded to me like our libc++/win bot is currently happy, what does this change do that the other one misses?

[thomasanderson]

In D58203#1403211, @thakis wrote:

But D57425 landed and this one didn't :-) Is the plan to land this and revert the other one then?

No revert necessary. It was broken initially, then D57425 added a partial fix, now this CL adds a full fix.

At the time I wrote D57425, there was just a duplicate symbol error. But then a change landed in lld that revealed a different linker error which this CL now fixes.

It sounded to me like our libc++/win bot is currently happy, what does this change do that the other one misses?

The Windows bots will not be happy without this change (or this other change https://github.com/googlei18n/libaddressinput/pull/172). I rolled libc++ into Chrome today because I know it's going to be a couple days still before all dependencies get updated, and I wanted to make sure there were no libc++-related issues in the roll in the meantime.

[ldionne]

Question: Why do we define those (almost trivial) functions in the dylib? It seems like those should always be in the headers?

I understand we can't remove them from the dylib now, of course.

[thomasanderson]

Friendly ping

[EricWF]

I'll be looking more into this today. Something weird is happening with __refstring.

src/include/refstring.h

69

I still don't understand. If the functions are unused, why are we encountering an undefined symbol error?

[EricWF]

Looking at the rational for this change again, I'm not sure I understand:

MSVC doesn't provide definitions of functions like std::logic_error::logic_error(const char*), so they are implicitly generated by the compiler as inline function.

There shouldn't be any compiler magic here. Nothing should be implicitly generated. Could you dig up the MSVC source code that declares and defines these types?

The inline-ness in the libc++ implementations need to match those of MSVC, otherwise we get an ODR violation leading to a duplicate symbol error when trying to link.

This suggests that MSVC provided definitions are provided as non-inline. If we're getting duplicate definitions, can't we simply omit our definitions?

[thomasanderson]

In D58203#1416975, @EricWF wrote:

Looking at the rational for this change again, I'm not sure I understand:

MSVC doesn't provide definitions of functions like std::logic_error::logic_error(const char*), so they are implicitly generated by the compiler as inline function.

There shouldn't be any compiler magic here. Nothing should be implicitly generated. Could you dig up the MSVC source code that declares and defines these types?

Sorry, that was supposed to be the copy constructor. I mean the copy constructor, destructor, and operator= are implicitly generated since they're not explicitly defined.
Here's the MSVC header: https://paste.googleplex.com/6494922167287808?raw

The inline-ness in the libc++ implementations need to match those of MSVC, otherwise we get an ODR violation leading to a duplicate symbol error when trying to link.

This suggests that MSVC provided definitions are provided as non-inline. If we're getting duplicate definitions, can't we simply omit our definitions?

The duplicate symbol error occurs when statically linking libc++ and MSVC (with use_component_build=false). Errors like this:

lld-link: error: duplicate symbol: "public: __cdecl std::runtime_error::runtime_error(class std::runtime_error const &)" (??0runtime_error@std@@QEAA@AEBV01@@Z) in obj/buildtools/third_party/libc++/libc++/stdexcept.obj and in libcpmt.lib(special_math.obj)

Omitting our definitions doesn't fix the issue, because we'd get a different linker error:

ld.lld: error: undefined symbol: std::runtime_error::runtime_error(char const*)
>>> referenced by locale.cpp
>>>               clang_x64/obj/buildtools/third_party/libc++/libc++/locale.o:(std::__1::__throw_runtime_error(char const*))

You might be wondering how it's possible for runtime_error() to be undefined by MSVC when we don't define it, but defined when we do. Nico could answer this better than I could, but basically a recent change to lld (https://reviews.llvm.org/D57324 and https://reviews.llvm.org/D58180) makes duplicate symbol checking more strict. For the first error, lld would happily add duplicated inline and non-inline symbols before, but now it complains. For the second error, the symbol in special_math.obj is unavailable to us (I guess since it's inline?). The change in lld is the entire reason for this CL; the Windows/libc++ build worked before it.

src/include/refstring.h

69

The symbols are used (eg by string.cpp), but no longer by stdexcept.cpp, which is the only file that #include's refstring.h. Here's what was happening before:

• Compile stdexcept.cpp: runtime_error() implicitly calls __libcpp_refstring(), so the compiler defines both runtime_error() and __libcpp_refstring() in the object file.
• Compile string.cpp: runtime_error() is left undefined in the object file.
• Linking succeeds because the undefined runtime_error() in string.o is available in stdexcept.o.

Now with this change (but without the _LIBCPP_REFSTRING_INLINE change), here's what happens:

• Compile stdexcept.cpp: runtime_error() is not defined in the source file any more, so runtime_error() is not added to the symbol table. __libcpp_refstring() was defined inline, but is not used by anything, so it's not added to the symbol table either.
• Compile string.cpp: runtime_error() is inlined and depends on __libcpp_refstring(), and the latter is added as an undefined symbol.
• Linking fails because string.o contains an undefined symbol __libcpp_refstring().

Now with this change with the _LIBCPP_REFSTRING_INLINE change:

• Compile stdexcept.cpp: runtime_error() is not defined in the source file any more, so runtime_error() is not added to the symbol table. __libcpp_refstring() was not defined inline, so is added to the symbol table even though it's not used.
• Compile string.cpp: runtime_error() is inlined and depends on __libcpp_refstring(), and the latter is added as an undefined symbol.
• Linking succeeds because the undefined __libcpp_refstring() in string.o is available in stdexcept.o.

I'd recommend removing the inline entirely rather than adding _LIBCPP_REFSTRING_INLINE if this sgty

[thomasanderson]

Fix by EricWF: https://reviews.llvm.org/D58945

include/__config

 #  define _LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION
 #endif
 
+#if defined(_LIBCPP_ABI_MICROSOFT) && !defined(_LIBCPP_NO_VCRUNTIME)
+#  define _LIBCPP_INLINE_STDEXCEPT
+#  define _LIBCPP_REFSTRING_TYPE_VISIBILITY _LIBCPP_TYPE_VIS
+#  define _LIBCPP_REFSTRING_INLINE_VISIBILITY
+#else
+#  define _LIBCPP_REFSTRING_TYPE_VISIBILITY _LIBCPP_HIDDEN
+#  define _LIBCPP_REFSTRING_INLINE_VISIBILITY inline
+#endif
+
 #if defined(__APPLE__)
 #  if !defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && \
       defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__)

include/stdexcept

 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-class _LIBCPP_HIDDEN __libcpp_refstring
+class _LIBCPP_REFSTRING_TYPE_VISIBILITY __libcpp_refstring
 {
     const char* __imp_;
 

     _VSTD::__libcpp_refstring __imp_;
 public:
     explicit logic_error(const string&);
+#if defined(_LIBCPP_INLINE_STDEXCEPT)
+    _LIBCPP_INLINE_VISIBILITY explicit logic_error(const char* __msg) : __imp_(__msg) {}
+#else
     explicit logic_error(const char*);
 
     logic_error(const logic_error&) _NOEXCEPT;
     logic_error& operator=(const logic_error&) _NOEXCEPT;
 
     virtual ~logic_error() _NOEXCEPT;
+#endif
 
     virtual const char* what() const _NOEXCEPT;
 };

     _VSTD::__libcpp_refstring __imp_;
 public:
     explicit runtime_error(const string&);
+#if defined(_LIBCPP_INLINE_STDEXCEPT)
+    _LIBCPP_INLINE_VISIBILITY explicit runtime_error(const char* __msg) : __imp_(__msg) {}
+#else
     explicit runtime_error(const char*);
 
     runtime_error(const runtime_error&) _NOEXCEPT;
     runtime_error& operator=(const runtime_error&) _NOEXCEPT;
 
     virtual ~runtime_error() _NOEXCEPT;
+#endif
 
     virtual const char* what() const _NOEXCEPT;
 };

     _LIBCPP_INLINE_VISIBILITY explicit domain_error(const string& __s) : logic_error(__s) {}
     _LIBCPP_INLINE_VISIBILITY explicit domain_error(const char* __s)   : logic_error(__s) {}
 
+#if !defined(_LIBCPP_INLINE_STDEXCEPT)
     virtual ~domain_error() _NOEXCEPT;
+#endif
 };
 
 class _LIBCPP_EXCEPTION_ABI invalid_argument

     _LIBCPP_INLINE_VISIBILITY explicit invalid_argument(const string& __s) : logic_error(__s) {}
     _LIBCPP_INLINE_VISIBILITY explicit invalid_argument(const char* __s)   : logic_error(__s) {}
 
+#if !defined(_LIBCPP_INLINE_STDEXCEPT)
     virtual ~invalid_argument() _NOEXCEPT;
+#endif
 };
 
 class _LIBCPP_EXCEPTION_ABI length_error

     _LIBCPP_INLINE_VISIBILITY explicit length_error(const string& __s) : logic_error(__s) {}
     _LIBCPP_INLINE_VISIBILITY explicit length_error(const char* __s)   : logic_error(__s) {}
 
+#if !defined(_LIBCPP_INLINE_STDEXCEPT)
     virtual ~length_error() _NOEXCEPT;
+#endif
 };
 
 class _LIBCPP_EXCEPTION_ABI out_of_range

     _LIBCPP_INLINE_VISIBILITY explicit out_of_range(const string& __s) : logic_error(__s) {}
     _LIBCPP_INLINE_VISIBILITY explicit out_of_range(const char* __s)   : logic_error(__s) {}
 
+#if !defined(_LIBCPP_INLINE_STDEXCEPT)
     virtual ~out_of_range() _NOEXCEPT;
+#endif
 };
 
 class _LIBCPP_EXCEPTION_ABI range_error

     _LIBCPP_INLINE_VISIBILITY explicit range_error(const string& __s) : runtime_error(__s) {}
     _LIBCPP_INLINE_VISIBILITY explicit range_error(const char* __s)   : runtime_error(__s) {}
 
+#if !defined(_LIBCPP_INLINE_STDEXCEPT)
     virtual ~range_error() _NOEXCEPT;
+#endif
 };
 
 class _LIBCPP_EXCEPTION_ABI overflow_error

     _LIBCPP_INLINE_VISIBILITY explicit overflow_error(const string& __s) : runtime_error(__s) {}
     _LIBCPP_INLINE_VISIBILITY explicit overflow_error(const char* __s)   : runtime_error(__s) {}
 
+#if !defined(_LIBCPP_INLINE_STDEXCEPT)
     virtual ~overflow_error() _NOEXCEPT;
+#endif
 };
 
 class _LIBCPP_EXCEPTION_ABI underflow_error

     _LIBCPP_INLINE_VISIBILITY explicit underflow_error(const string& __s) : runtime_error(__s) {}
     _LIBCPP_INLINE_VISIBILITY explicit underflow_error(const char* __s)   : runtime_error(__s) {}
 
+#if !defined(_LIBCPP_INLINE_STDEXCEPT)
     virtual ~underflow_error() _NOEXCEPT;
+#endif
 };
 
 }  // std

src/include/refstring.h

 
 using namespace __refstring_imp;
 
-inline
+_LIBCPP_REFSTRING_INLINE_VISIBILITY
 __libcpp_refstring::__libcpp_refstring(const char* msg) {
     std::size_t len = strlen(msg);
     _Rep_base* rep = static_cast<_Rep_base *>(::operator new(sizeof(*rep) + len + 1));

     __imp_ = data;
 }
 
-inline
+_LIBCPP_REFSTRING_INLINE_VISIBILITY
 __libcpp_refstring::__libcpp_refstring(const __libcpp_refstring &s) _NOEXCEPT
     : __imp_(s.__imp_)
 {

         __libcpp_atomic_add(&rep_from_data(__imp_)->count, 1);
 }
 
-inline
+_LIBCPP_REFSTRING_INLINE_VISIBILITY
 __libcpp_refstring& __libcpp_refstring::operator=(__libcpp_refstring const& s) _NOEXCEPT {
     bool adjust_old_count = __uses_refcount();
     struct _Rep_base *old_rep = rep_from_data(__imp_);

     return *this;
 }
 
-inline
+_LIBCPP_REFSTRING_INLINE_VISIBILITY
 __libcpp_refstring::~__libcpp_refstring() {
     if (__uses_refcount()) {
         _Rep_base* rep = rep_from_data(__imp_);

     }
 }
 
-inline
+_LIBCPP_REFSTRING_INLINE_VISIBILITY
 bool __libcpp_refstring::__uses_refcount() const {
 #ifdef __APPLE__
     return __imp_ != get_gcc_empty_string_storage();

src/stdexcept.cpp

 {
 }
 
+#if !defined(_LIBCPP_INLINE_STDEXCEPT)
 logic_error::logic_error(const char* msg) : __imp_(msg)
 {
 }

     __imp_ = le.__imp_;
     return *this;
 }
+#endif
 
 runtime_error::runtime_error(const string& msg) : __imp_(msg.c_str())
 {
 }
 
+#if !defined(_LIBCPP_INLINE_STDEXCEPT)
 runtime_error::runtime_error(const char* msg) : __imp_(msg)
 {
 }

     __imp_ = le.__imp_;
     return *this;
 }
+#endif
 
 #if !defined(_LIBCPPABI_VERSION) && !defined(LIBSTDCXX)
 

     return __imp_.c_str();
 }
 
-#if !defined(_LIBCPP_ABI_MICROSOFT) || defined(_LIBCPP_NO_VCRUNTIME)
+#if !defined(_LIBCPP_INLINE_STDEXCEPT)
 
 logic_error::~logic_error() _NOEXCEPT {}
 domain_error::~domain_error() _NOEXCEPT {}