[WIP] core.stdcpp.{exception,typeinfo}: Fix D/C++ symbol conflicts and undefined D vtable entries by kinke · Pull Request #2788 · dlang/druntime

kinke · 2019-09-08T16:30:48Z

Fixes linker errors for the LDC druntime test runner.

Used source refs: libstdc++, libc++, Visual Studio 2017 headers

kinke · 2019-09-08T16:33:26Z

src/core/stdcpp/exception.d

+    version (GenericBaseException)
+    {
+        ///
+        ~this() nothrow;


Not declaring the dtor means it gets an implicit one (base class has one), and that one collides with the C++ one. [For MSVC, everything is defined inline in the C++ headers, so the implicit one does the job.]

kinke · 2019-09-08T16:34:19Z

src/core/stdcpp/exception.d

+        ///
+        ~this() nothrow;
+        ///
+        override const(char)* what() const nothrow;


Required to have the vtable generated on the D side feature the right override in that vtable slot.

kinke · 2019-09-08T16:36:01Z

src/core/stdcpp/typeinfo.d

-        final bool before(const type_info rhs) const;
-        final const(char)* name(__type_info_node* p = &__type_info_root_node) const;
+        final bool before(const type_info rhs) const nothrow;
+        final const(char)* name(__type_info_node* p = &__type_info_root_node) const nothrow;


That's actually the wrong signature for VS 2017 (which takes no params), but I guess DMD has to maintain VS 2010 compatibility or whatever.

We have -extern-std=, so we can version the C++ code according to the C++ version it binds to.

kinke · 2019-09-08T16:36:31Z

src/core/stdcpp/typeinfo.d

    class bad_cast : exception
    {
-        this(const(char)* msg = "bad cast");
+        this(const(char)* msg = "bad cast") nothrow { super(msg); }


Defined inline in C++ header.

kinke · 2019-09-08T16:38:06Z

src/core/stdcpp/typeinfo.d

-        void dtor1();                           // consume destructor slot in vtbl[]
-        void dtor2();                           // consume destructor slot in vtbl[]
+        void dtor1() {}                         // consume destructor slot in vtbl[]
+        void dtor2() {}                         // consume destructor slot in vtbl[]


The vtable on the D side needs dummy definitions to put into the first 2 vtable slots.

O_o ... this is like 12 months out of date. Put an actual destructor.

kinke · 2019-09-08T16:38:52Z

src/core/stdcpp/typeinfo.d

-        //~this();
-        override const(char)* what() const;
+        this() nothrow {}
+        ~this() nothrow;


Again, implicit dtor colliding with C++ one when linking in libstdc++.

kinke · 2019-09-08T17:05:40Z

Due to Phobos including druntime, Phobos would need to be adapted, i.e., linking shared Phobos against lib[std]c++ too.

dlang-bot · 2019-09-08T21:37:02Z

Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + druntime#2788"

Geod24 · 2019-09-08T22:25:36Z

Due to Phobos including druntime, Phobos would need to be adapted, i.e., linking shared Phobos against lib[std]c++ too.

That's a huge issue IMO. We can't link druntime/phobos with the C++ standard library for the sake of those bindings.

kinke · 2019-09-08T22:45:35Z

It only affects the shared libraries and should thus just be an additional dependency to lib[std]c++.{so,dylib}. When using the static libraries, you only need the C++ runtime when using core.stdcpp.{exception,typeinfo} [edit: and possibly other modules in that package when instantiating the templates]. If that's not acceptable (I don't have a strong opinion about this), then core.stdcpp.* should probably be extracted out of druntime.

JohanEngelen · 2019-09-09T17:39:28Z

I also think it is bad if we need to link with the C++ standard library, even if it is only for the shared libraries. In my opinion that is too high a price to pay for the convenience of having C++ bindings in the D std library. Simply moving the bindings into their own separate package seems much more appropriate.

kinke · 2019-09-09T23:27:47Z

Pinging @TurkeyMan.

TurkeyMan · 2019-09-10T00:19:17Z

These are the exact 2 modules that I didn't write... I think the need a complete overhaul, but i'll give this patch a better look when I'm home.
Generally; local methods should be extern(D) so they don't collide with C++, and declare proper ctor/dtor's because they mangle and consume the vtable properly now.

These functions aren't templates, and we don't want druntime to depend on the C++ runtime. All these functions are basically 1-liners, so we should just mirror the C++ implementation, and not extern.
C++ bindings must obviously not cause link errors, or infer a hard dependency on libc++. Care is required, as I've done for all the other modules.

TurkeyMan · 2020-05-07T00:23:31Z

Where's this at now? I can't really see from the CI output what's going on here.
Are there multiple-definition errors? It looks like that's what I'd expect...

kinke · 2020-05-07T00:56:17Z

Had some time to re-look into this and tried the extern(D) way as you suggested. Now the druntime testrunner compiles and links fine for LDC (ldc-developers/ldc#3158), i.e., the D vtables can be populated; no idea what's going on with the failing subset of Mac stdcpp CI jobs here.

See the 2nd paragraph in ldc-developers/ldc#3158 (comment) as to why extern(D) isn't the perfect solution when it comes to virtual functions and trying to avoid a C++ runtime dependency while not conflicting with its symbols. For LDC, @weak extern(C++) might be the solution (ldc-developers/ldc#3424).

TurkeyMan · 2020-05-07T01:08:20Z

Yeah, I was gonna say, weak linkage is the obvious answer, but Walter has rejected it for years.
Perhaps we can push for it again? We need to standardise a weak attribute, it's a PITA being compiler specific!

... and missing D vtable entries.

kinke · 2020-05-07T16:56:53Z

but Walter has rejected it for years

I guess that's solvable by providing him with a finished implementation. ;) - None of my business though.

ibuclaw · 2021-08-25T06:28:14Z

Revived as #3552

kinke commented Sep 8, 2019

View reviewed changes

kinke force-pushed the stdcpp3 branch 2 times, most recently from 0b43ad6 to 96b0394 Compare September 8, 2019 19:42

kinke marked this pull request as ready for review September 8, 2019 21:36

kinke requested review from WalterBright, andralex and wilzbach as code owners September 8, 2019 21:36

kinke changed the title ~~[stable] Fix missing/duplicate symbol definitions in core.stdcpp.{exception,typeinfo}~~ [blocked] [stable] Fix missing/duplicate symbol definitions in core.stdcpp.{exception,typeinfo} Sep 8, 2019

kinke mentioned this pull request Sep 9, 2019

druntime: Include core.stdcpp.* modules ldc-developers/ldc#3158

Merged

kinke force-pushed the stdcpp3 branch from 96b0394 to ccbe442 Compare May 6, 2020 19:12

kinke requested review from Burgos, CyberShadow, DmitryOlshansky, Geod24, MartinNowak, ibuclaw, jmdavis and rainers as code owners May 6, 2020 19:12

kinke requested a review from schveiguy as a code owner May 6, 2020 19:12

dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label May 6, 2020

kinke changed the base branch from stable to master May 6, 2020 19:12

kinke changed the title ~~[blocked] [stable] Fix missing/duplicate symbol definitions in core.stdcpp.{exception,typeinfo}~~ [WIP] core.stdcpp.{exception,typeinfo}: Fix D/C++ symbol conflicts and missing D vtable entries May 6, 2020

dlang-bot added the WIP Work In Progress - not ready for review or pulling label May 6, 2020

kinke force-pushed the stdcpp3 branch 2 times, most recently from 5f1e950 to 665b571 Compare May 6, 2020 19:50

core.stdcpp.{exception,typeinfo}: Fix D/C++ symbol conflicts

a20c647

... and missing D vtable entries.

kinke force-pushed the stdcpp3 branch from 665b571 to a20c647 Compare May 7, 2020 16:54

kinke changed the title ~~[WIP] core.stdcpp.{exception,typeinfo}: Fix D/C++ symbol conflicts and missing D vtable entries~~ [WIP] core.stdcpp.{exception,typeinfo}: Fix D/C++ symbol conflicts and undefined D vtable entries May 17, 2020

dlang-bot added the stalled label May 27, 2021

dlang-bot added the Needs Rebase needs a `git rebase` performed label Aug 22, 2021

ibuclaw mentioned this pull request Aug 25, 2021

core.stdcpp: Upstream differences between dmd and gdc/ldc druntimes #3552

Merged

ibuclaw closed this Aug 25, 2021

kinke deleted the stdcpp3 branch September 5, 2021 10:59

Uh oh!

Conversation

kinke commented Sep 8, 2019

Uh oh!

kinke Sep 8, 2019

Choose a reason for hiding this comment

Uh oh!

kinke Sep 8, 2019

Choose a reason for hiding this comment

Uh oh!

kinke Sep 8, 2019

Choose a reason for hiding this comment

Uh oh!

TurkeyMan Sep 10, 2019

Choose a reason for hiding this comment

Uh oh!

kinke Sep 8, 2019

Choose a reason for hiding this comment

Uh oh!

kinke Sep 8, 2019

Choose a reason for hiding this comment

Uh oh!

TurkeyMan Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinke Sep 8, 2019

Choose a reason for hiding this comment

Uh oh!

kinke commented Sep 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Sep 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

Geod24 commented Sep 8, 2019

Uh oh!

kinke commented Sep 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JohanEngelen commented Sep 9, 2019

Uh oh!

kinke commented Sep 9, 2019

Uh oh!

TurkeyMan commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TurkeyMan commented May 7, 2020

Uh oh!

kinke commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TurkeyMan commented May 7, 2020

Uh oh!

kinke commented May 7, 2020

Uh oh!

ibuclaw commented Aug 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

TurkeyMan Sep 10, 2019 •

edited

Loading

kinke commented Sep 8, 2019 •

edited

Loading

dlang-bot commented Sep 8, 2019 •

edited

Loading

kinke commented Sep 8, 2019 •

edited

Loading

TurkeyMan commented Sep 10, 2019 •

edited

Loading

kinke commented May 7, 2020 •

edited

Loading