Page MenuHomePhabricator

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

Authored by thomasanderson on Feb 13 2019, 1:00 PM.

Details

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.

Diff Detail

Event Timeline

thomasanderson created this revision.Feb 13 2019, 1:00 PM
EricWF requested changes to this revision.Feb 13 2019, 2:20 PM

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.

This revision now requires changes to proceed.Feb 13 2019, 2:20 PM
thomasanderson marked 2 inline comments as done.
thomasanderson marked an inline comment as not done.

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 added inline comments.Feb 14 2019, 12:25 AM
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 marked 3 inline comments as done.
thomasanderson added inline comments.
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.

This was made obsolete by D57425, right?

This was made obsolete by D57425, right?

Rather, this CL makes that one obsolete

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?

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.

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.

EricWF added a comment.Mar 4 2019, 6:44 AM

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 added a comment.Mar 4 2019, 8:05 AM

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 edited the summary of this revision. (Show Details)Mar 4 2019, 11:13 AM
thomasanderson marked an inline comment as done.Mar 4 2019, 12:00 PM

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