[libcxx-commits] [PATCH] D58203: [libc++] Inline stdexcept constructors, destructors, and assignment operators when using MSVC ABI

[Tom Anderson via Phabricator via libcxx-commits]

thomasanderson marked an inline comment as done.
thomasanderson added a comment.

In D58203#1416975 <https://reviews.llvm.org/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 at std@@QEAA at 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.



================
Comment at: src/include/refstring.h:69
 
-inline
+_LIBCPP_REFSTRING_INLINE_VISIBILITY
 __libcpp_refstring::__libcpp_refstring(const char* msg) {
----------------
EricWF wrote:
> thomasanderson wrote:
> > EricWF wrote:
> > > thomasanderson wrote:
> > > > EricWF wrote:
> > > > > These changes seem wrong. And I don't understand why the changes are needed in relation to MSVC.
> > > > 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 at __1@std@@QEAA at 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?
> > > 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?
> > 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.
> I still don't understand. If the functions are unused, why are we encountering an undefined symbol error?
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


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

https://reviews.llvm.org/D58203