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

Mon Mar 4 12:36:29 PST 2019

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?

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/99010c48/attachment.html>