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

Mon Mar 4 12:49:53 PST 2019

On Mon, Mar 4, 2019 at 12:36 PM Casey Carter <cartec69 at gmail.com> wrote:

> Disclaimer: Microsoft employee.
>
> 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?
>

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).

> On Mon, Mar 4, 2019, 12:00 Tom Anderson via Phabricator via libcxx-commits
> <libcxx-commits at lists.llvm.org> wrote:
>
>> 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
>>
>>
>>
>> _______________________________________________
>> libcxx-commits mailing list
>> libcxx-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/libcxx-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20190304/1c6ccabd/attachment-0001.html>