<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 4, 2019 at 12:36 PM Casey Carter <<a href="mailto:cartec69@gmail.com">cartec69@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto">Disclaimer: Microsoft employee.<div dir="auto"><br></div><div dir="auto">It seems you're jumping head first into a pit of ODR violations. Even if you can work out the symbol issues, I think MSVC and libc++ have different representations for the exception types. What's the original motivation for linking libc++ with MSVC's standard library? </div></div></blockquote><div><br></div><div>I'm switching Windows Chromium to build with libc++. We're not really using the full MSVC standard library, just the ABI parts in place of libc++abi because the latter only implements the Itanium C++ ABI, and not the Microsoft ABI which we use on Windows (and we need to use in order to interoperate correctly with COM among other things). </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 4, 2019, 12:00 Tom Anderson via Phabricator via libcxx-commits <<a href="mailto:libcxx-commits@lists.llvm.org" target="_blank">libcxx-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">thomasanderson marked an inline comment as done.<br>
thomasanderson added a comment.<br>
<br>
In D58203#1416975 <<a href="https://reviews.llvm.org/D58203#1416975" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D58203#1416975</a>>, @EricWF wrote:<br>
<br>
> Looking at the rational for this change again, I'm not sure I understand:<br>
><br>
> > 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.<br>
><br>
> 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?<br>
<br>
<br>
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. <br>
Here's the MSVC header: <a href="https://paste.googleplex.com/6494922167287808?raw" rel="noreferrer noreferrer" target="_blank">https://paste.googleplex.com/6494922167287808?raw</a><br>
<br>
> <br>
> <br>
>> 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.<br>
> <br>
> This suggests that MSVC provided definitions are provided as non-inline. If we're getting duplicate definitions, can't we simply omit our definitions?<br>
<br>
The duplicate symbol error occurs when statically linking libc++ and MSVC (with use_component_build=false). Errors like this:<br>
<br>
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)<br>
<br>
Omitting our definitions doesn't fix the issue, because we'd get a different linker error:<br>
<br>
ld.lld: error: undefined symbol: std::runtime_error::runtime_error(char const*)<br>
>>> referenced by locale.cpp<br>
>>> clang_x64/obj/buildtools/third_party/libc++/libc++/locale.o:(std::__1::__throw_runtime_error(char const*))<br>
<br>
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 (<a href="https://reviews.llvm.org/D57324" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D57324</a> and <a href="https://reviews.llvm.org/D58180" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D58180</a>) 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.<br>
<br>
<br>
<br>
================<br>
Comment at: src/include/refstring.h:69<br>
<br>
-inline<br>
+_LIBCPP_REFSTRING_INLINE_VISIBILITY<br>
__libcpp_refstring::__libcpp_refstring(const char* msg) {<br>
----------------<br>
EricWF wrote:<br>
> thomasanderson wrote:<br>
> > EricWF wrote:<br>
> > > thomasanderson wrote:<br>
> > > > EricWF wrote:<br>
> > > > > These changes seem wrong. And I don't understand why the changes are needed in relation to MSVC.<br>
> > > > This is now needed eg. for the inline definition of logic_error::logic_error(), which must now construct a __libcpp_refstring.<br>
> > > > <br>
> > > > If I leave "inline" in, I get link errors:<br>
> > > > lld-link: error: undefined symbol: "public: __cdecl std::__1::__libcpp_refstring::__libcpp_refstring(char const *)" (??0__libcpp_refstring@__1@std@@QEAA@PEBD@Z)<br>
> > > > >>> referenced by obj/buildtools/third_party/libc++/libc++/string.obj<br>
> > > > <br>
> > > > 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?<br>
> > > I don't understand that error at all.<br>
> > > <br>
> > > 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.<br>
> > > <br>
> > > Am I missing something?<br>
> > 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.<br>
> I still don't understand. If the functions are unused, why are we encountering an undefined symbol error?<br>
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:<br>
<br>
* Compile stdexcept.cpp: `runtime_error()` implicitly calls `__libcpp_refstring()`, so the compiler defines both `runtime_error()` and `__libcpp_refstring()` in the object file.<br>
* Compile string.cpp: `runtime_error()` is left undefined in the object file.<br>
* Linking succeeds because the undefined `runtime_error()` in string.o is available in stdexcept.o. <br>
<br>
Now with this change (but without the `_LIBCPP_REFSTRING_INLINE` change), here's what happens:<br>
* 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.<br>
* Compile string.cpp: `runtime_error()` is inlined and depends on `__libcpp_refstring()`, and the latter is added as an undefined symbol.<br>
* Linking fails because string.o contains an undefined symbol `__libcpp_refstring()`.<br>
<br>
Now with this change with the `_LIBCPP_REFSTRING_INLINE` change:<br>
* 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.<br>
* Compile string.cpp: `runtime_error()` is inlined and depends on `__libcpp_refstring()`, and the latter is added as an undefined symbol.<br>
* Linking succeeds because the undefined `__libcpp_refstring()` in string.o is available in stdexcept.o.<br>
<br>
I'd recommend removing the `inline` entirely rather than adding `_LIBCPP_REFSTRING_INLINE` if this sgty<br>
<br>
<br>
CHANGES SINCE LAST ACTION<br>
<a href="https://reviews.llvm.org/D58203/new/" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D58203/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D58203" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D58203</a><br>
<br>
<br>
<br>
_______________________________________________<br>
libcxx-commits mailing list<br>
<a href="mailto:libcxx-commits@lists.llvm.org" rel="noreferrer" target="_blank">libcxx-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/libcxx-commits" rel="noreferrer noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/libcxx-commits</a><br>
</blockquote></div>
</blockquote></div></div>