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

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

I'll be looking more into this today. Something weird is happening with `__refstring`.



================
Comment at: src/include/refstring.h:69
 
-inline
+_LIBCPP_REFSTRING_INLINE_VISIBILITY
 __libcpp_refstring::__libcpp_refstring(const char* msg) {
----------------
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?


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

https://reviews.llvm.org/D58203